Messages in this thread | | | Date | Thu, 24 Aug 2023 22:13:55 +0100 | From | Mark Brown <> | Subject | Re: [PATCH] regulator: userspace-consumer: Add regulator event support |
| |
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@bewilderbeest.net> 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. [unhandled content-type:application/pgp-signature] | |