Re: [PATCH v5 1/2] tpm: add sysfs exports for all banks of PCR registers

On Fri, 2021-01-15 at 08:55 +0200, Jarkko Sakkinen wrote:

On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:

quoted

On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote:

[...]

quoted

quoted

Please use sysfs_emit() and sysfs_emit_at() for new sysfs files.

Hey these interfaces were added after this patch began life.  But
looking at sysfs_emit_at() I've got to say "aah ... don't you guys
ever read rusty's guide to interfaces?" an interface which takes in
an absolute page position but returns a relative offset to the
position it took in is asking for people to get it wrong.  You
should always be consistent about uses for inputs and
outputs.  Basically the only way you can ever use sysfs_emit_at in
a show routine is as

offset += sysfs_emit_at(buf, offset, ...);

because you always need to track the absolute offset.

It looks like we already have a couple of bugs in the kernel
introduced by this confusion ...  return sysfs_emit() vs return
sysfs_emit_at() being the most tricky ...

How is using sysfs_emit() different from using snprintf() for the
caller, ignoring the added safety measures? I'm new to this API.

Using the sprintX variants you maintain a cursor pointer, so they all
look like

char *cursor = buf;

...

cursor += sprintX(cursor, "...", ...

...

return cursor - buf;

So the input is a relative cursor and the output is the additional
offset.  I'm not claiming it's the best interface but it is hard to get
wrong, just that if we're going to force a new interface we should make
it much better.

with sysfs_emit_at you use an offset "cursor" but it's hard to know
without reading the function how to do it because the return is
relative rather than absolute.  To have an interface it would be hard
to misuse, I think the best way would be to take a pointer to the
offset and adjust it after use, so

sysfs_emit_at(buf, &offset, ...);

That way it returns void so you can't use it in place of 

return sysfs_emit()

And you don't have to worry about whether the return is absolute or
relative because it adjusts the pointer for you.

The whole point about Rusty and interfaces is that if you are going to
invent new interfaces you should make them easy to get right and hard
to misuse.  A function you can't figure out how to use until you read
the source is about 2/10 on the rusty scale:

https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

James