Re: [PATCH] iio: trigger: Avoid data race

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 28 May 2025 16:17:06 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> On Wed, May 28, 2025 at 6:19 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Tue, May 27, 2025 at 11:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:  
> > > On Wed, May 28, 2025 at 5:25 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:  
> > > > On Tue, May 27, 2025 at 10:05 PM Gyeyoung Baek <gye976@gmail.com> wrote:  
> >
> > ...
> >  
> > > > At bare minimum they are not relevant to the patch change and haven't
> > > > been described in the commit messages.  
> > >
> > > Hi Andy, thanks for your review.
> > > I initially skipped this part as I thought it was minor.
> > > But on a second look, it seems better to separate the declaration from
> > > the logic.
> > >
> > > What do you think about the data race logic? Would it make sense?  
> >
> > The point is valid, the atomic_read() + atomic_set() is 101 thingy,
> > whoever did that doesn't really have a clue what atomic(ity) is.  

:) 

I'm trying to recall what this protection is actually for so this might
be a little vague as descriptions go...

The key here is what can happen in that 'race' and hence why I'm still fairly 
sure it isn't a real race.  Firstly this is called in an irq handler
so we can only have one call of this particular function at a time
for a given trigger.  So no race against itself.

The atomic part is about decrements that can happen elsewhere, but there
can never be 'more' decrements than the value we set the counter to in this
function.  That is it never goes negative.

Those decrements ultimately happen in calls that can't happen until after
the set (via various convoluted paths ultimately getting to
iio_trigger_notify_done()).  In many cases the trigger is masked until it
is reenabled on the counter == 0 (elsewhere) - but not always...

IIRC correctly aim is to not double trigger in cases where we can't mask
the trigger (a particularly rubbish trigger) - so if any of the downstream
devices still hasn't called iio_trigger_notify_done() then we quietly
drop this particular irq on the floor. We don't mind dropping a few
too many, just dropping too few a then we end up loosing count of who
has to be 'done' with the trigger.

Hence the counter won't change between atomic_get and the atomic_set
as it's always 0 which means no one else is left to decrement it.

Atomics don't always need to be atomic all the time, they just are in
some states.

So, is this something that has caused observed problems, or based
on code inspection? My remembering of what was going on here might well
be flawed.

There are some 'fun' corners for what happens after that set though
where a handler can run fast enough in race conditions we end up
hitting 0 in iio_trigger_notify_done_atomic() and have to schedule
restarting of the trigger because that might involve a bus write over
a sleeping bus.  That one was a painful bug report some years ago...

Jonathan

> 
> Thanks for your explanation.
> Then I’ll send a v2 patch with only the `int i` change, following the
> review feedback.
> 
> --
> Best regards,
> Gyeyoung
> 
>