Re: [PATCH] regulator: userspace-consumer: Add regulator event support

[Mark Brown]

On Fri, Aug 04, 2023 at 05:02:02AM -0700, Zev Weiss wrote:
> On Fri, Aug 04, 2023 at 01:59:44AM PDT, Naresh Solanki wrote:
> > On Fri, 4 Aug 2023 at 02:15, Zev Weiss <zev@xxxxxxxxxxxxxxxxx> wrote:
> > > On Thu, Aug 03, 2023 at 04:12:25AM PDT, Naresh Solanki wrote:

> > > >Add sysfs attribute to track regulator events received from regulator
> > > >notifier block handler.

> > > >+      mutex_lock(&events_lock);
> > > >+      e = data->events;
> > > >+      data->events = 0;

> > > ...particularly this bit -- a read operation on a read-only file (and
> > > especially one with 0644 permissions) having side-effects (clearing the
> > > value it accesses) seems on the face of it like fairly surprising
> > > behavior.  Is this a pattern that's used elsewhere in any other sysfs
> > > files?

> > These are regulator events & are valid when it occurs.
> > Userspace application is intended to consume them as soon as the
> > event is notified by kernel sysfs_notify.

> Sure, but that doesn't really address what I was concerned about -- as
> written this is a read operation on a read-only file (0444, not 0644 as I
> mistakenly wrote above) that nevertheless alters the state of an internal
> kernel data structure.  Can you point to any other sysfs attributes that
> behave like that?  I can't think of one offhand, and I'd be reluctant to
> establish the precedent.

The whole userspace consumer interface is a kludge so I'm not super
concerned about what's effectively clear on read interrupts, ideally
it'd be a file reporting the current status but we don't have a way to
read the current status of everything...

> Would a uevent-based mechanism maybe be a better fit for the problem you're
> trying to solve?

uevents would definitely be good to have, and much better than polling
for apps that can use them, but they don't preclude a read interface.

> > > However, it looks like this would expose the values of all the
> > > REGULATOR_EVENT_* constants as a userspace-visible ABI -- is that
> > > something we really want to do?

> > Yes.

> Given that the REGULATOR_EVENT_* constants are defined in headers under
> include/linux and not include/uapi, it doesn't seem like they were intended
> to be used as part of a userspace-visible interface.  If they're going to
> be, I think they should be moved to the uapi directory so that applications
> can use the proper definitions from the kernel instead of manually
> replicating it on their own (but I suspect we should probably find a
> different approach instead).

This is a concern.  We should probably indirect via strings at least,
but that probably implies a file per event at least.  Due to that I'll
drop this patch for this release.  Sorrt for doing that this late, it's
not ideal - like I said in the other thread I lost this thread under a
bunch of others in my inbox.

Attachment: signature.asc
Description: PGP signature