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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 18 May 2026 21:31:48 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> 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@intel.com> 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.)

There are atomic decrements elsewhere that need to be (multiple drivers
can decrement at the same time) and mixing and matching between atomic accessors
and non atomic is a mess.

Comment wise. I'd like to say I'll get to it, but unlikely it'll be soon.

Jonathan


> 
> > > >                 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 :)    
>