Re: [PATCH 10/22] gpio: reinforce desc->flags handling

[Bartosz Golaszewski <brgl@bgdev.pl>]

On Wed, Jan 31, 2024 at 9:35 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Jan 31, 2024 at 9:01 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > We now removed the gpio_lock spinlock and modified the places
> > > previously protected by it to handle desc->flags access in a consistent
> > > way. Let's improve other places that were previously unprotected by
> > > reading the flags field of gpio_desc once and using the stored value for
> > > logic consistency. If we need to modify the field, let's also write it
> > > back once with a consistent value resulting from the function's logic.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > (...)
> >
> > I have a trouble with this one:
> >
> > gpiochip_find_base_unlocked()
> > > +       unsigned long flags;
> > (...)
> > > +       flags = READ_ONCE(desc->flags);
> > (...)
> > > +       if (test_bit(FLAG_OPEN_DRAIN, &flags) &&
> > > +           test_bit(FLAG_IS_OUT, &flags))
> > >                 return 0;
> > (...)
> > > +       assign_bit(FLAG_IS_OUT, &flags, !ret);
> > > +       WRITE_ONCE(desc->flags, flags);
> >
> > I unerstand the atomicity of each operation here, but ... if what you want
> > to protect is modifications from other CPUs, how do we know that another
> > CPU isn't coming in and reading and modifying and assigning
> > another flag inbetween these operations while the value is only
> > stored in the CPU-local flags variable?
> >
> > Same with gpiod_direction_output().
> >
> > To me it seems like maybe you need to actually protect the desc->flags
> > with the SRCU struct in these cases? (and not only use it for the
> > label protection then).
> >
> > An alternative is maybe to rewrite the code with test_and_set().
> >
> > But as you say it is currently unprotected, I just wonder if this really
> > adds any protection.
>
> After re-reading the cover letter I'm fine with this, but I still wonder
> if it buys us anything.
>

This was a tough one...

I don't really see any way around it. SRCU is for pointers but even
then - we wouldn't get with SRCU anything more than what we're getting
with atomic reads and writes. As neither sleeping nor atomic locks
will work in the case of the GPIO subsystem, I figured that we should
strive for the maximum of coherence we can achieve - and for that I
figured that we should read the flags once, do our thing and then
write back a consistent result. If someone else comes around at the
same time and writes something else - well, he better be an
*exclusive*  user of that GPIO and know what they're doing. :)

Anyway, I think this series is already a big step forward and should
at least protect us from crashing. We can continue the work on
achieving full state consistency later.

Bart

> Maybe some words looped back from the
> commit message that we are not really protecting the callbacks
> because access is [predominantly] exclusive?
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij