| Message ID | 20231130134630.18198-11-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio/pinctrl: replace gpiochip_is_requested() with a safer interface | expand |
On Thu, Nov 30, 2023 at 02:46:30PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We have no external users of gpiochip_is_requested(). Let's remove it > and replace its internal calls with direct testing of the REQUESTED flag. ... > - cpy = kstrdup(label, GFP_KERNEL); > - if (!cpy) > - return ERR_PTR(-ENOMEM); > + scoped_guard(spinlock_irqsave, &gpio_lock) { > + if (!test_bit(FLAG_REQUESTED, &desc->flags)) > + return NULL; > + cpy = kstrdup(desc->label, GFP_KERNEL); > + if (!cpy) > + return ERR_PTR(-ENOMEM); You just introduced these lines earlier in the series, and here you moved them again. With guard() instead it may be kept in a better shape. > + }
On Thu, Nov 30, 2023 at 5:46 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:30PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > We have no external users of gpiochip_is_requested(). Let's remove it > > and replace its internal calls with direct testing of the REQUESTED flag. > > ... > > > - cpy = kstrdup(label, GFP_KERNEL); > > - if (!cpy) > > - return ERR_PTR(-ENOMEM); > > + scoped_guard(spinlock_irqsave, &gpio_lock) { > > + if (!test_bit(FLAG_REQUESTED, &desc->flags)) > > + return NULL; > > > + cpy = kstrdup(desc->label, GFP_KERNEL); > > + if (!cpy) > > + return ERR_PTR(-ENOMEM); > > You just introduced these lines earlier in the series, and here you moved > them again. With guard() instead it may be kept in a better shape. > I wanted to limit the critical section to a minimum hence scoped variant. And this will go away as soon as we have a desc lock so it's temporary anyway. What matters to me is how the code looks when sending it to Torvalds. On the off chance that we don't get the locking rework merged in time for v6.8, I want this to at least be under the existing lock. Bart > > + } > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Nov 30, 2023 at 06:46:20PM +0100, Bartosz Golaszewski wrote: > On Thu, Nov 30, 2023 at 5:46 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:30PM +0100, Bartosz Golaszewski wrote: ... > > > - cpy = kstrdup(label, GFP_KERNEL); > > > - if (!cpy) > > > - return ERR_PTR(-ENOMEM); > > > + scoped_guard(spinlock_irqsave, &gpio_lock) { > > > + if (!test_bit(FLAG_REQUESTED, &desc->flags)) > > > + return NULL; > > > > > + cpy = kstrdup(desc->label, GFP_KERNEL); > > > + if (!cpy) > > > + return ERR_PTR(-ENOMEM); > > > > You just introduced these lines earlier in the series, and here you moved > > them again. With guard() instead it may be kept in a better shape. > > > > I wanted to limit the critical section to a minimum hence scoped > variant. And this will go away as soon as we have a desc lock so it's > temporary anyway. What matters to me is how the code looks when > sending it to Torvalds. On the off chance that we don't get the > locking rework merged in time for v6.8, I want this to at least be > under the existing lock. guard() here is equally scoped, no? And what's wrong with that when gets to Torvalds? He accepted your guard() cases last time IIRC. > > > + }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 8e932e6a6a8d..3070a4f7bbb1 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1085,7 +1085,7 @@ void gpiochip_remove(struct gpio_chip *gc) spin_lock_irqsave(&gpio_lock, flags); for (i = 0; i < gdev->ngpio; i++) { - if (gpiochip_is_requested(gc, i)) + if (test_bit(FLAG_REQUESTED, &gdev->descs[i].flags)) break; } spin_unlock_irqrestore(&gpio_lock, flags); @@ -2373,33 +2373,6 @@ void gpiod_free(struct gpio_desc *desc) gpio_device_put(desc->gdev); } -/** - * gpiochip_is_requested - return string iff signal was requested - * @gc: controller managing the signal - * @offset: of signal within controller's 0..(ngpio - 1) range - * - * Returns NULL if the GPIO is not currently requested, else a string. - * The string returned is the label passed to gpio_request(); if none has been - * passed it is a meaningless, non-NULL constant. - * - * This function is for use by GPIO controller drivers. The label can - * help with diagnostics, and knowing that the signal is used as a GPIO - * can help avoid accidentally multiplexing it to another controller. - */ -const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset) -{ - struct gpio_desc *desc; - - desc = gpiochip_get_desc(gc, offset); - if (IS_ERR(desc)) - return NULL; - - if (test_bit(FLAG_REQUESTED, &desc->flags) == 0) - return NULL; - return desc->label; -} -EXPORT_SYMBOL_GPL(gpiochip_is_requested); - /** * gpiochip_dup_line_label - Get a copy of the consumer label. * @gc: GPIO chip controlling this line. @@ -2414,16 +2387,21 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested); */ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset) { - const char *label; + struct gpio_desc *desc; char *cpy; - label = gpiochip_is_requested(gc, offset); - if (!label) + desc = gpiochip_get_desc(gc, offset); + if (IS_ERR(desc)) return NULL; - cpy = kstrdup(label, GFP_KERNEL); - if (!cpy) - return ERR_PTR(-ENOMEM); + scoped_guard(spinlock_irqsave, &gpio_lock) { + if (!test_bit(FLAG_REQUESTED, &desc->flags)) + return NULL; + + cpy = kstrdup(desc->label, GFP_KERNEL); + if (!cpy) + return ERR_PTR(-ENOMEM); + } return cpy; } diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index b1ed501e9ee0..9a44016789c8 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -531,7 +531,6 @@ struct gpio_chip { #endif /* CONFIG_OF_GPIO */ }; -const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);