| Message ID | 20231130134630.18198-2-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio/pinctrl: replace gpiochip_is_requested() with a safer interface | expand |
On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > gpiochip_is_requested() not only has a misleading name but it returns > a pointer to a string that is freed when the descriptor is released. > > Provide a new helper meant to replace it, which returns a copy of the > label string instead. ... > + * Must not be called from atomic context. Put the respective lockdep annotation. ... > + char *cpy; So, why not naming it fully, i.e. "copy"?
On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > gpiochip_is_requested() not only has a misleading name but it returns > > a pointer to a string that is freed when the descriptor is released. > > > > Provide a new helper meant to replace it, which returns a copy of the > > label string instead. > > ... > > > + * Must not be called from atomic context. > > Put the respective lockdep annotation. > > ... > > > + char *cpy; > > So, why not naming it fully, i.e. "copy"? > Ekhm... let me quote the BigPinguin :) -- C is a Spartan language, and your naming conventions should follow suit. Unlike Modula-2 and Pascal programmers, C programmers do not use cute names like ThisVariableIsATemporaryCounter. A C programmer would call that variable ``tmp``, which is much easier to write, and not the least more difficult to understand. -- Bart > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Nov 30, 2023 at 06:48:06PM +0100, Bartosz Golaszewski wrote: > On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote: ... > > > + char *cpy; > > > > So, why not naming it fully, i.e. "copy"? > > > > Ekhm... let me quote the BigPinguin :) > > -- > > C is a Spartan language, and your naming conventions should follow suit. > Unlike Modula-2 and Pascal programmers, C programmers do not use cute > names like ThisVariableIsATemporaryCounter. A C programmer would call that > variable ``tmp``, which is much easier to write, and not the least more > difficult to understand. In contrary the cpy is more difficult to understand. `git grep -lw cpy` vs. `git grep -lw copy` suggests that my variant is preferable (even excluding tools/ and Documentation/).
On Thu, Nov 30, 2023 at 7:00 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 06:48:06PM +0100, Bartosz Golaszewski wrote: > > On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote: > > ... > > > > > + char *cpy; > > > > > > So, why not naming it fully, i.e. "copy"? > > > > > > > Ekhm... let me quote the BigPinguin :) > > > > -- > > > > C is a Spartan language, and your naming conventions should follow suit. > > Unlike Modula-2 and Pascal programmers, C programmers do not use cute > > names like ThisVariableIsATemporaryCounter. A C programmer would call that > > variable ``tmp``, which is much easier to write, and not the least more > > difficult to understand. > > In contrary the cpy is more difficult to understand. > > `git grep -lw cpy` vs. `git grep -lw copy` suggests that my variant > is preferable (even excluding tools/ and Documentation/). > You are a thorough reviewer but man, are you also a bikeshedder. :) Whatever, let's make it 'copy'. Bart > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Nov 30, 2023 at 5:27 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:21PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > gpiochip_is_requested() not only has a misleading name but it returns > > a pointer to a string that is freed when the descriptor is released. > > > > Provide a new helper meant to replace it, which returns a copy of the > > label string instead. > > ... > > > + * Must not be called from atomic context. > > Put the respective lockdep annotation. > What are you referring to? Bart > ... > > > + char *cpy; > > So, why not naming it fully, i.e. "copy"? > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index a5faaea6915d..8e932e6a6a8d 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2400,6 +2400,35 @@ const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset) } EXPORT_SYMBOL_GPL(gpiochip_is_requested); +/** + * gpiochip_dup_line_label - Get a copy of the consumer label. + * @gc: GPIO chip controlling this line. + * @offset: Hardware offset of the line. + * + * Returns: + * Pointer to a copy of the consumer label if the line is requested or NULL + * if it's not. If a valid pointer was returned, it must be freed using + * kfree(). In case of a memory allocation error, the function returns %ENOMEM. + * + * Must not be called from atomic context. + */ +char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset) +{ + const char *label; + char *cpy; + + label = gpiochip_is_requested(gc, offset); + if (!label) + return NULL; + + cpy = kstrdup(label, GFP_KERNEL); + if (!cpy) + return ERR_PTR(-ENOMEM); + + return cpy; +} +EXPORT_SYMBOL_GPL(gpiochip_dup_line_label); + /** * gpiochip_request_own_desc - Allow GPIO chip to request its own descriptor * @gc: GPIO chip diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 100c329dc986..9796a34e2fee 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -532,6 +532,7 @@ struct gpio_chip { }; const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); +char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); /** * for_each_requested_gpio_in_range - iterates over requested GPIOs in a given range