Re: [PATCH] iio: pressure: rohm-bm1390: notify trigger on all error paths

[Andy Shevchenko]

On Mon, May 18, 2026 at 03:55:21PM +0100, Jonathan Cameron wrote:
> On Mon, 18 May 2026 09:59:24 +0300
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:
> > On Mon, May 18, 2026 at 08:21:17AM +0300, Matti Vaittinen wrote:
> > > On 17/05/2026 20:12, David Lechner wrote:  
> > > > On 5/17/26 11:08 AM, Stepan Ionichev wrote:  

...

> > > Maybe it would be better to do something like:
> > > 
> > > void iio_trigger_poll_nested(struct iio_trigger *trig)
> > > {
> > >         int i;
> > > 
> > >         if (!atomic_read(&trig->use_count)) {
> > >                 atomic_set(&trig->use_count,
> > > CONFIG_IIO_CONSUMERS_PER_TRIGGER);  
> > 
> > Just in case somebody is going to do that, avoid doing atomic_read() followed
> > by atomic_set(). This is typical TOCTOU issue. This should be something like
> > atomic_xchg() or atomic_add_return() or something like this in a single atomic
> > operation.
> 
> Just to clarify - the current code is fine.  This got reported a few years
> back and I did the analysis to prove it. From what I recall the key is
> that the state space isn't as complex as it immediately looks.
> That counter is either non 0 at the start (we don't use it here and we
> skip an interrupt - that's actually the desired behaviour if the trigger is running
> too fast - triggers must survive that - reenable() callback is there to make that
> all work).
> 
> Otherwise there is a single path that sets it and we know any decrement until after
> that happens would have undeflowed (and hence was a bug). The rest are decrement
> only and it can never go to less than 0.
> 
> Hence it is fine.
> 
> Agreed things get messy if we make this alg any more complex though!

Perhaps we need a good comment just on top of this atomic_read()/atomic_set()
pair. Because it's really the code no one should take as an example how to do
atomics :-) Logical question, why do we even have atomics there? Shouldn't
be that READ_ONCE()/WRITE_ONCE() to have an integrity in place? (This I believe
even mentioned in the documentation for atomics.)

> > >                 for (i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
> > >                         if (trig->subirqs[i].enabled)
> > >                                 handle_nested_irq(trig->subirq_base + i);
> > >                         else
> > >                                 iio_trigger_notify_done(trig);
> > >                 }
> > > 		atomic_set(&trig->use_count, 0); /* Clear the use_count if drivers didn't
> > > */
> > >         }
> > > }
> > > 
> > > to prevent this class of problems once and for all. But yeah, wiser minds
> > > have designed this - so let's hear some other opinions as well :)  

-- 
With Best Regards,
Andy Shevchenko