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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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:
> > On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote:
[...]
> > > 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