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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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.

Yours,
Linus Walleij