| Message ID | 20231130134630.18198-1-brgl@bgdev.pl |
|---|---|
| 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 02:46:23PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Use the new gpiochip_dup_line_label() helper to safely retrieve the > descriptor label. ... > - seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label); > + seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, > + label ?: "Unrequested"); Haha, see previous comment (in previous email).
On Thu, Nov 30, 2023 at 02:46:27PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Use the new gpiochip_dup_line_label() helper to safely retrieve the > descriptor label. ... > seq_printf(s, > " gpio-%-3d (%-20.20s) %s %s %s pad-%-3d offset:0x%03x mux:%d %s%s%s", > pin, > - label, > + label ?: "Unrequested", This already fourth (?) duplication among drivers. Perhaps you want a helper: gpiochip_dup_line_label_fallback() // naming is up to you which will return the same for everybody and we don't need to hunt for the different meaning of "Unrequested". Also the word "Unrequested" is a bit doubtful as it can be a label, right? Something with special characters / spaces / etc would suit better? In any case it might require to add a warning (?) to the GPIO lib core when label gets assigned if it clashes with the "reserved" word. > val & BYT_INPUT_EN ? " " : "in", > val & BYT_OUTPUT_EN ? " " : "out", > str_hi_lo(val & BYT_LEVEL),
On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Rework for_each_requested_gpio_in_range() to use the new helper to > retrieve a dynamically allocated copy of the descriptor label and free > it at the end of each iteration. We need to leverage the CLASS()' > destructor to make sure that the label is freed even when breaking out > of the loop. ... > const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); > char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); > > + One blank line is enough. > +struct _gpiochip_for_each_data { > + const char **label; > + int *i; Why is this a signed? > +}; ... > +DEFINE_CLASS(_gpiochip_for_each_data, > + struct _gpiochip_for_each_data, > + if (*_T.label) kfree(*_T.label), > + ({ struct _gpiochip_for_each_data _data = { label, i }; > + *_data.i = 0; > + _data; }), To me indentation of ({}) is quite weird. Where is this style being used instead of more readable ({ ... }) ?
On Thu, Nov 30, 2023 at 5:36 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:27PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Use the new gpiochip_dup_line_label() helper to safely retrieve the > > descriptor label. > > ... > > > seq_printf(s, > > " gpio-%-3d (%-20.20s) %s %s %s pad-%-3d offset:0x%03x mux:%d %s%s%s", > > pin, > > - label, > > + label ?: "Unrequested", > > This already fourth (?) duplication among drivers. > Perhaps you want a helper: > gpiochip_dup_line_label_fallback() // naming is up to you > which will return the same for everybody and we don't need to hunt for > the different meaning of "Unrequested". > IMO the overhead here is very small in return for better readability (IOW: `label ?: "Unrequested"` is more readable than some function named `gpiochip_dup_line_label_fallback()`). Given the string is in .rodata anyway, I wouldn't be surprised if adding a helper resulted in bigger code. > Also the word "Unrequested" is a bit doubtful as it can be a label, right? > Something with special characters / spaces / etc would suit better? > In any case it might require to add a warning (?) to the GPIO lib core > when label gets assigned if it clashes with the "reserved" word. > Agreed but this is a functional change in debugfs output. I know debugfs is not considered stable but I didn't write it, I don't know who's using it and I prefer to leave it be. Bart > > val & BYT_INPUT_EN ? " " : "in", > > val & BYT_OUTPUT_EN ? " " : "out", > > str_hi_lo(val & BYT_LEVEL), > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Nov 30, 2023 at 5:40 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Rework for_each_requested_gpio_in_range() to use the new helper to > > retrieve a dynamically allocated copy of the descriptor label and free > > it at the end of each iteration. We need to leverage the CLASS()' > > destructor to make sure that the label is freed even when breaking out > > of the loop. > > ... > > > const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); > > char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); > > > > + > > One blank line is enough. > > > +struct _gpiochip_for_each_data { > > + const char **label; > > + int *i; > > Why is this a signed? > Some users use signed, others use unsigned. It doesn't matter as we can't overflow it with the limit on the lines we have. Bart > > +}; > > ... > > > +DEFINE_CLASS(_gpiochip_for_each_data, > > + struct _gpiochip_for_each_data, > > + if (*_T.label) kfree(*_T.label), > > + ({ struct _gpiochip_for_each_data _data = { label, i }; > > + *_data.i = 0; > > + _data; }), > > To me indentation of ({}) is quite weird. Where is this style being used > instead of more readable > There are no guidelines for this type of C abuse AFAIK. The macro may be ugly but at least it hides the details from users which look nice instead. 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. > > ... > > > + 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:42:37PM +0100, Bartosz Golaszewski wrote: > On Thu, Nov 30, 2023 at 5:40 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 30, 2023 at 02:46:29PM +0100, Bartosz Golaszewski wrote: ... > > > const char *gpiochip_is_requested(struct gpio_chip *gc, unsigned int offset); > > > char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset); > > > > > > + > > > > One blank line is enough. > > > > > +struct _gpiochip_for_each_data { > > > + const char **label; > > > + int *i; > > > > Why is this a signed? > > Some users use signed, others use unsigned. It doesn't matter as we > can't overflow it with the limit on the lines we have. What's the problem to make it unsigned and be done with that for good? > > > +}; ... > > > +DEFINE_CLASS(_gpiochip_for_each_data, > > > + struct _gpiochip_for_each_data, > > > + if (*_T.label) kfree(*_T.label), > > > + ({ struct _gpiochip_for_each_data _data = { label, i }; > > > + *_data.i = 0; > > > + _data; }), > > > > To me indentation of ({}) is quite weird. Where is this style being used > > instead of more readable > > There are no guidelines for this type of C abuse AFAIK. The macro may > be ugly but at least it hides the details from users which look nice > instead. If we can make it more readable for free, why not doing that way? > > ({ > > ... > > }) > > > > ?
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 > >
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> While reworking the locking in GPIOLIB I realized that locking the descriptor with users still calling gpiochip_is_requested() will still be buggy as it returns a pointer to a string that can be freed whenever the descriptor is released. Let's provide a safer alternative in the form of a function that returns a copy of the label. Use it in all drivers and remove gpiochip_is_requested(). I plan to provide this series in an immutable branch for the pinctrl and baytrail trees to pull. v1 -> v2: - use DEFINE_CLASS() to register a destructor for making sure that the duplicated label doesn't leak out of the for_each loops even with break; Bartosz Golaszewski (10): gpiolib: provide gpiochip_dup_line_label() gpio: wm831x: use gpiochip_dup_line_label() gpio: wm8994: use gpiochip_dup_line_label() gpio: stmpe: use gpiochip_dup_line_label() pinctrl: abx500: use gpiochip_dup_line_label() pinctrl: nomadik: use gpiochip_dup_line_label() pinctrl: baytrail: use gpiochip_dup_line_label() pinctrl: sppctl: use gpiochip_dup_line_label() gpiolib: use gpiochip_dup_line_label() in for_each helpers gpiolib: remove gpiochip_is_requested() drivers/gpio/gpio-stmpe.c | 6 +++- drivers/gpio/gpio-wm831x.c | 14 ++++++--- drivers/gpio/gpio-wm8994.c | 13 +++++--- drivers/gpio/gpiolib.c | 37 ++++++++++++++--------- drivers/pinctrl/intel/pinctrl-baytrail.c | 11 ++++--- drivers/pinctrl/nomadik/pinctrl-abx500.c | 9 ++++-- drivers/pinctrl/nomadik/pinctrl-nomadik.c | 6 +++- drivers/pinctrl/sunplus/sppctl.c | 10 +++--- include/linux/gpio/driver.h | 37 +++++++++++++++++------ 9 files changed, 95 insertions(+), 48 deletions(-)