On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
Neat!
Reviewed-by: Linus Walleij <[email protected]>
(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