Received-SPF: pass (google.com: domain of linux-kernel+bounces-47034-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240130124828.14678-1-brgl@bgdev.pl> <20240130124828.14678-10-brgl@bgdev.pl>
In-Reply-To: <20240130124828.14678-10-brgl@bgdev.pl>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Wed, 31 Jan 2024 20:51:14 +0100
Message-ID: <CACRpkdY8v2x6VzRnEV6baa4_XLGLm0BOe6Nj6wzjujHxqEDHVA@mail.gmail.com>
Subject: Re: [PATCH 09/22] gpio: remove gpio_lock
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Kent Gibson <warthog618@gmail.com>, Alex Elder <elder@linaro.org>, 
	Geert Uytterhoeven <geert+renesas@glider.be>, "Paul E . McKenney" <paulmck@kernel.org>, 
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Wolfram Sang <wsa@the-dreams.de>, 
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 30, 2024 at 1:48=E2=80=AFPM Bartosz Golaszewski <brgl@bgdev.pl>=
 wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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 subsequen=
t
> 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@linaro.org>

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

(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 directi=
on_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 =3D kstrdup_const(label, GFP_ATOMIC);
> +               new =3D 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