Re: [PATCH 3/3] Documentation: ABI: sysfs-driver-regulator-output

[Zev Weiss]

On Sun, Sep 03, 2023 at 06:04:23AM PDT, Greg Kroah-Hartman wrote:
> On Fri, Sep 01, 2023 at 02:13:23AM -0700, Zev Weiss wrote:
> > Adding Greg re: sysfs ABI design...
> >
> > On Thu, Aug 31, 2023 at 05:14:10AM PDT, Naresh Solanki wrote:
> > > Adds sysfs-driver-regulator-output
> > >
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > > ---
> > > .../ABI/testing/sysfs-driver-regulator-output         | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > > create mode 100644 Documentation/ABI/testing/sysfs-driver-regulator-output
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-regulator-output b/Documentation/ABI/testing/sysfs-driver-regulator-output
> > > new file mode 100644
> > > index 000000000000..f9b0a8f810fa
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-regulator-output
> > > @@ -0,0 +1,11 @@
> > > +What:		/sys/bus/platform/drivers/regulator-output/*/events
> > > +Date:		August 2023
> > > +Description:	Provided regulator events.
> > > +
> > > +		Read provides various events the regulator associated with the
> > > +		driver has encountered. All REGULATOR_EVENT_* are
> > > +		defined in include/uapi/linux/regulator.h
> > > +
> > > +		e.g.
> > > +		cat /sys/bus/platform/drivers/regulator-output/ssb_rssd32/events
> > > +		0x0
> >
> > If we really are going to proceed with a "read with side-effects"
> > (clear-on-read) design, that should absolutely be loudly and clearly
> > documented, since it's very different from how sysfs files typically work
> > and hence a somewhat glaring principle-of-least-surprise violation.  Also,
> > since from the code it looks like it's intended to be used via poll(2), that
> > should be described here as well.
>
> Poll?  Ick, but that can happen.
>
> But yes, reading a sysfs should almost never cause a side affect at all.
>
> But what do you mean by "clear events?"  

I mean that when the sysfs file is read, the state variable whose value 
it exposes is also cleared as a side-effect (so the read operation 
"consumes" the value it returns) -- see the implementation in patch 2 of 
this series (specifically the 'data->events = 0' assignment in 
events_show()):

https://lore.kernel.org/lkml/20230831121412.2359239-2-Naresh.Solanki@xxxxxxxxxxxxx/

(As indicated in my reply to that patch, this seems like a footgun to me 
and I'm hoping we can come up with a better approach.)


Zev