Message-ID: <20250731135925.00007e5d@huawei.com>
Date: Thu, 31 Jul 2025 13:59:25 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Sean Anderson <sean.anderson@...ux.dev>
CC: Jonathan Cameron <jic23@...nel.org>, Nuno Sá
	<noname.nuno@...il.com>, Jean Delvare <jdelvare@...e.com>, Guenter Roeck
	<linux@...ck-us.net>, <linux-iio@...r.kernel.org>,
	<linux-hwmon@...r.kernel.org>, Andy Shevchenko <andy@...nel.org>, Nuno
 Sá <nuno.sa@...log.com>, <linux-kernel@...r.kernel.org>,
	David Lechner <dlechner@...libre.com>
Subject: Re: [PATCH 3/7] iio: Add in-kernel API for events

On Tue, 29 Jul 2025 16:09:20 -0400
Sean Anderson <sean.anderson@...ux.dev> wrote:

> On 7/29/25 14:33, Jonathan Cameron wrote:
> > On Mon, 28 Jul 2025 18:44:30 -0400
> > Sean Anderson <sean.anderson@...ux.dev> wrote:
> >   
> >> On 7/27/25 12:21, Jonathan Cameron wrote:  
> >> > On Tue, 15 Jul 2025 12:52:19 -0400
> >> > Sean Anderson <sean.anderson@...ux.dev> wrote:
> >> >     
> >> >> On 7/15/25 07:09, Nuno Sá wrote:    
> >> >> > On Mon, 2025-07-14 at 21:20 -0400, Sean Anderson wrote:      
> >> >> >> Add an API to notify consumers about events. Events still need to be
> >> >> >> enabled using the iio_read_event/iio_write_event functions. Of course,
> >> >> >> userspace can also manipulate the enabled events. I don't think this is
> >> >> >> too much of an issue, since userspace can also manipulate the event
> >> >> >> thresholds. But enabling events may cause existing programs to be
> >> >> >> surprised when they get something unexpected. Maybe we should set the
> >> >> >> interface as busy when there are any in-kernel listeners?
> >> >> >>       
> >> >> > 
> >> >> > Sensible question. I'm not that familiar with events but I suspect is not
> >> >> > trivial (if doable) to do a similar approach as with buffers? With buffers, an
> >> >> > inkernal consumer get's it's own buffer object (that goes into a list of active
> >> >> > buffers in the iio device) with all channels enabled and then we demux the
> >> >> > appropriate channels for each consumer.      
> >> >> 
> >> >> For in-kernel consumers I think it's reasonable to expect them to handle
> >> >> events they didn't explicitly enable. I'm not sure about userspace
> >> >> consumers.    
> >> > 
> >> > This already happens because we don't have a demux equivalent (what we do
> >> > for buffered data flow) so if a device only has a single enable bit that covers
> >> > multiple events (annoyingly common for accelerometers for example) then
> >> > userspace will get events it didn't ask for.   We 'could' fix that,
> >> > but it's never really been worth the effort.
> >> > 
> >> > Events tend to be low data rate so an occasionally extra is rather different
> >> > to having to have much larger data buffers to handle a range of channels you
> >> > never asked for.
> >> > 
> >> > Lets be careful to document this behaviour as 'may enable extra events'
> >> > as then if we decide later to do demux type stuff we won't be breaking ABI.
> >> > No one will mind getting fewer spurious events due to a core improvement.    
> >> 
> >> Where would this get documented?  
> > 
> > Starting point will be in the docs for the ABI that asks for any events at all.
> > 
> > Also useful to add some thing to Documentation/IIO though there are lots of
> > other things those docs don't yet cover :(  
> 
> Notably the whole events API :l
> 
> >>   
> >> >>     
> >> >> > Independent of the above, we can argue that having both inkernel and userspace
> >> >> > changing thresholds is ok (I mean, there's nothing stopping two userspace apps
> >> >> > doing that) but we should likely be careful with enabling/disabling. If multiple
> >> >> > consumers enable the same event, one of them disabling it should not disable it
> >> >> > for all the consumers, right?      
> >> >> 
> >> >> Right now the HWMON consumer never permanently disable events to avoid this
> >> >> issue. It does toggle the enable to determine if an alarm should stay
> >> >> enabled:
> >> >>              ________
> >> >> condition __/        \________
> >> >>           _____    ____    ___
> >> >> enable         \__/    \__/
> >> >> 
> >> >> event       |     |
> >> >>              __    ____
> >> >> alarm     __/  \__/    \_____
> >> >> 
> >> >> read           1       1    0
> >> >> 
> >> >> I suppose this could also be done by comparing the raw threshold to the
> >> >> channel.    
> >> > 
> >> > I wonder if we should add the option to do a 'get_exclusive' or similar
> >> > to block the IIO user interfaces if something critical is using the device.
> >> > 
> >> > If we were for instance to use this to block the IOCTL to get the events
> >> > fd then any built in driver etc will almost certainly load before anyone
> >> > can call the ioctl so it will fairly cleanly block things.    
> >> 
> >> This is how it currently works for userspace. Only one process can create
> >> the event fd, and everyone else gets -EBUSY.
> >> 
> >> Of course, it would be pretty surprising to have an IIO device where
> >> some channels were used by userspace and others were used by hwmon and
> >> then have your daemon stop working after you update your kernel because
> >> now the hwmon driver takes exclusive event access.  
> > 
> > True.  I wonder how many boards we don't know about are using the iio-hwmon
> > bridge. We can check the ones in kernel for whether they grab all the
> > channels (which would rule this out).
> >
> > Another things we could do is have an opt in from the IIO driver.
> > That way only 'new' drivers would have this behaviour.  Not nice though.  
> 
> I would really like for this to "just work" if at all possible, so an
> opt-out would be preferable. Maybe a hwmon module parameter.
> 
> But I think we can do better:
> 
> - Both kernel/userspace can/should handle unexpected events
>   - This includes extra (synthetic) events.
> - Both kernel/userspace mostly just want to enable events
> - Disabling events is not as important because of the previous bullet.
> - But losing events is probably bad so we want to ensure we trigger
>   events at the same places they would have been triggered before.
> 
> So maybe we have an implementation where
> 
> - Enabling an event disables the backing event before re-enabling it if
>   there are any existing users
> - Disabling an event only disables the backing event if all users are
>   gone
> 
> It could look something like
> 
> iio_sysfs_event_set(event, val):
>     if val:
>         if !event.user_enable
>             disable(event)
>         enable(event)
>     else if !event.kernel_enables
>         disable(event)
>     event.user_enable = val
> 
> iio_inkern_event_set(event, val):
>     if val:
>         if event.kernel_enables++ || event.user_enable
>             disable(event)
>         enable(event)
>     else if !--event.kernel_enables && !event.user_enable:
>         disable(event)

Something like that should work.  We'll need to be careful
to gate any push towards userspace on it waiting for something.

Given we only send them when IIO_BUSY_BIT_POS is set on the
event interface (which happens on requesting the fd) I think
we may be fine already.

Jonathan

> 
> --Sean
> 
> >> 
> >> I originally had kernel users read from the kfifo just like userspace,
> >> but I was concerned about the above scenario.
> >>   
> > 
> > yeah, always a problem to retrofit policy.
> >   
> >> --Sean
> >>   
> >   
> 
> 

Powered by blists - more mailing lists