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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Jan 31, 2024 at 9:01 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> (...)
>
> 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.

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@xxxxxxxxxx>

Yours,
Linus Walleij