| Message ID | 20240205093418.39755-18-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: rework locking and object life-time control | expand |
On Mon, Feb 05, 2024 at 10:34:12AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Checking desc->gdev->chip for NULL without holding it in place with some > serializing mechanism is pointless. Remove this check. Also don't check > desc->gdev for NULL as it can never happen. We'll be protecting > gdev->chip with SRCU soon but we will provide a dedicated, automatic > class for that. ... > void gpiod_free(struct gpio_desc *desc) > { > - /* > - * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip > - * may already be NULL but we still want to put the references. > - */ > - if (!desc) > - return; > + VALIDATE_DESC_VOID(desc); IIRC we (used to) have two cases like this (you added one in some code like last year).
On Mon, Feb 5, 2024 at 2:47 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 05, 2024 at 10:34:12AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Checking desc->gdev->chip for NULL without holding it in place with some > > serializing mechanism is pointless. Remove this check. Also don't check > > desc->gdev for NULL as it can never happen. We'll be protecting > > gdev->chip with SRCU soon but we will provide a dedicated, automatic > > class for that. > > ... > > > void gpiod_free(struct gpio_desc *desc) > > { > > - /* > > - * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip > > - * may already be NULL but we still want to put the references. > > - */ > > - if (!desc) > > - return; > > + VALIDATE_DESC_VOID(desc); > > IIRC we (used to) have two cases like this (you added one in some code like > last year). > None of the consumer-facing functions does it anymore. Not sure about this, maybe it was removed earlier. Bart
On Mon, Feb 05, 2024 at 08:22:23PM +0100, Bartosz Golaszewski wrote: > On Mon, Feb 5, 2024 at 2:47 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 05, 2024 at 10:34:12AM +0100, Bartosz Golaszewski wrote: ... > > > void gpiod_free(struct gpio_desc *desc) > > > { > > > - /* > > > - * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip > > > - * may already be NULL but we still want to put the references. > > > - */ > > > - if (!desc) > > > - return; > > > + VALIDATE_DESC_VOID(desc); > > > > IIRC we (used to) have two cases like this (you added one in some code like > > last year). > > > > None of the consumer-facing functions does it anymore. Not sure about > this, maybe it was removed earlier. Okay, the only place that might be considered is gpiod_to_gpio_device(). But that API seems new, I don't know if VALIDATE_DESC_VOID() is okay to use there, maybe it should be commented if not. Also there is a typo in the kernel doc — 'the users already holds' --> 'the user already holds'.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index f8d53ebbf25b..7d897c807e95 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2251,19 +2251,12 @@ static int validate_desc(const struct gpio_desc *desc, const char *func) { if (!desc) return 0; + if (IS_ERR(desc)) { pr_warn("%s: invalid GPIO (errorpointer)\n", func); return PTR_ERR(desc); } - if (!desc->gdev) { - pr_warn("%s: invalid GPIO (no device)\n", func); - return -EINVAL; - } - if (!desc->gdev->chip) { - dev_warn(&desc->gdev->dev, - "%s: backing chip is gone\n", func); - return 0; - } + return 1; } @@ -2339,12 +2332,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc) void gpiod_free(struct gpio_desc *desc) { - /* - * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip - * may already be NULL but we still want to put the references. - */ - if (!desc) - return; + VALIDATE_DESC_VOID(desc); if (!gpiod_free_commit(desc)) WARN_ON(1);