Re: [PATCH 09/22] gpio: remove gpio_lock

[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>
>
> The "multi-function" gpio_lock is pretty much useless with how it's used
> in GPIOLIB currently. Because many GPIO API calls can be called from all
> contexts but may also call into sleeping driver callbacks, there are
> many places with utterly broken workarounds like yielding the lock to
> call a possibly sleeping function and then re-acquiring it again without
> taking into account that the protected state may have changed.
>
> It was also used to protect several unrelated things: like individual
> descriptors AND the GPIO device list. We now serialize access to these
> two with SRCU and so can finally remove the spinlock.
>
> There is of course the question of consistency of lockless access to
> GPIO descriptors. Because we only support exclusive access to GPIOs
> (officially anyway, I'm looking at you broken
> GPIOD_FLAGS_BIT_NONEXCLUSIVE bit...) and the API contract with providers
> does not guarantee serialization, it's enough to ensure we cannot
> accidentally dereference an invalid pointer and that the state we present
> to both users and providers remains consistent. To achieve that: read the
> flags field atomically except for a few special cases. Read their current
> value before executing callback code and use this value for any subsequent
> logic. Modifying the flags depends on the particular use-case and can
> differ. For instance: when requesting a GPIO, we need to set the
> REQUESTED bit immediately so that the next user trying to request the
> same line sees -EBUSY.
>
> While at it: the allocations that used GFP_ATOMIC until this point can
> now switch to GFP_KERNEL.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>

Neat!
Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

(I'm sorry about NONEXCLUSIVE, let's see what we can do about it...)

> @@ -578,6 +577,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>                 return -EINVAL;
>         }
>
> +       if (!test_and_set_bit(FLAG_EXPORT, &desc->flags))
> +               return -EPERM;

This exit early split off from the big if() below (which is good) is
a new thing right? Maybe mention this in the commit message?

> -/* gpio_lock prevents conflicts during gpio_desc[] table updates.
> - * While any GPIO is requested, its gpio_chip is not removable;
> - * each GPIO's "requested" flag serves as a lock and refcount.
> - */
> -DEFINE_SPINLOCK(gpio_lock);

GOOD RIDDANCE.

> -               /* FIXME: make this GFP_KERNEL once the spinlock is out. */
> -               new = kstrdup_const(label, GFP_ATOMIC);
> +               new = kstrdup_const(label, GFP_KERNEL);

And all of this is neat as well.

Someone might complain about splitting that in a separate patch,
but not me because I don't care so much about such processy
things. (The patchset is already split enough as it is.)

Yours,
Linus Walleij