Re:Re: [PATCH] iio: gyro: bmg160: reject duplicate event disable

[jiale yao]

At 2026-09-28 01:16:32, "Jonathan Cameron" <jic23@xxxxxxxxxx> wrote:
>On Sat, 26 Sep 2026 02:01:33 +0100
>Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
>> On Fri, 25 Sep 2026 22:33:48 +0800 (CST)
>> "jiale yao" <19888972804@xxxxxxx> wrote:
>> 
>> > At 2026-09-25 22:18:05, "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxx> wrote:  
>> > >On Fri, Sep 25, 2026 at 10:09:54PM +0800, Jiale Yao wrote:    
>> > >> The IIO core does not filter duplicate writes to the event enable
>> > >> attribute.    
>> > >
>> > >Can it be done there once for all?    
>> > 
>> > I don't think the core can safely do this generically, since it doesn't own
>> > the per-event state and read_event_config() may reflect shared hardware state.
>> > 
>> > The runtime PM accounting is driver-specific, so handling duplicate writes in
>> > the driver seems safer. gp2ap002 does the same in commit 579c049b4cb6,
>> > refer https://lore.kernel.org/all/20260720193911.74919-2-nikhilgtr@xxxxxxxxx/  
>> 
>> Exactly right.
>> 
>> Event handling in the core would indeed require complex mapping of
>> what can actually be enabled on a given device. Some of those we might
>> be able to represent with simple approaches similar to what we do
>> for avail_scan_masks for buffered channels but it is hard to generalize
>> and FWIW that concept still misses some corner cases of channel enabling
>> where drivers have to apply constraints in the actual buffer enable.
>> Those are rare enough that it isn't too much of a problem in practice.
>> Probably fine on 99% of drivers.
>> 
>> Events have simplre cases like:
>> - 1 enable bit turns a whole load of events on (we don't filter in
>>   the kernel - so userspace has to cope with surprise events).
>> - Always on event signals.
>> - Limited numbers of general purpose event engines (the ADI IMUs do
>>   this as they tend to have 2 instances). Each engine can do anything
>>   but there are only two of them.
>> 
>> We also have more complex ones like mode bits that flip a subset
>> of events from one enable state to another.  There are some really
>> odd rules in some devices because some upstream filter that is needed
>> for 5 seemingly unrelated events has to be in a particular state for
>> subsets of them.
>> 
>> Anyhow, all in all event control is a mess of 'original solutions'
>> from vendors and relies on the (naughty) get out of IIO ABI that
>> just occasionally any write to any userspace ABI element can
>> change what is read back from any other.
>> 
>> Not great, but I've never come up with a better solution for events.
>> If anyone wants to have a go at architecting a filtering solution
>> a bit like the IIO buffer demux maybe we can opt in to it from
>> some drivers.
>> 
>> I am wondering if it is worth registering a driver specific tidy
>> up events call though that would disable everything a bit like
>> we disable buffers as part of the userspace tear down.  That would
>> close out some races sashiko has reported.  That project would
>> I think be a lot simpler to implement.
>> 
>This one needs a fixes tag, as do the other similar ones.
>Fine to just reply to each thread with the appropriate tag.

Fixes: 22b46c45fb9b ("iio:gyro:bmg160 Gyro Sensor driver")

>
>Thanks,
>
>Jonathan
>
>> Jonathan
>> 
>> >   
>> > >    
>> > >> bmg160_write_event_config() already ignores repeated enable
>> > >> requests, but a repeated disable request still calls
>> > >> bmg160_set_power_state(data, false), dropping a runtime PM reference
>> > >> that was not acquired for this request. This can underflow the runtime
>> > >> PM usage count and trigger a "Runtime PM usage count underflow" warning.
>> > >> 
>> > >> Return early when the requested state already matches ev_enable_state.    
>> > >
>> > >-- 
>> > >With Best Regards,
>> > >Andy Shevchenko
>> > >    
>> 
>>