| Message ID | 20190808132543.26274-2-sr@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpiolib: Add for_each_gpio_suffix() helper | expand |
On Thu, Aug 08, 2019 at 03:25:43PM +0200, Stefan Roese wrote: > This patch fixes a backward compatibility issue, when boards use the > old style GPIO suffix "-gpio" instead of the new "-gpios". This > potential problem has been introduced by commit d99482673f95 ("serial: > mctrl_gpio: Check if GPIO property exisits before requesting it"). > > This patch now fixes this issue by iterating over all supported GPIO > suffixes by using the newly introduced for_each_gpio_suffix() helper. > > Also, the string buffer is now allocated on the stack to avoid the > problem of allocation in a loop and its potential failure. > for (i = 0; i < UART_GPIO_MAX; i++) { > enum gpiod_flags flags; > - char *gpio_str; > + const char *suffix; > + char gpio_str[32]; /* 32 is max size of property name */ Hmm... don't we have some define for the maximum length of property? Or maybe we can still continue using kasprintf() approach? > bool present; > + int k; > + > + /* > + * Check if GPIO property exists and continue if not. Iterate > + * over all supported GPIO suffixes (foo-gpios vs. foo-gpio). > + */ > + for_each_gpio_suffix(k, suffix) { > + snprintf(gpio_str, sizeof(gpio_str), "%s-%s", > + mctrl_gpios_desc[i].name, suffix); > + > + present = device_property_present(dev, gpio_str); > + if (present) > + break; > + } > > - /* Check if GPIO property exists and continue if not */ > - gpio_str = kasprintf(GFP_KERNEL, "%s-gpios", > - mctrl_gpios_desc[i].name); > - if (!gpio_str) > - continue; > - > - present = device_property_present(dev, gpio_str); Because there is no more explicit assignment of present outside of the loop, the compiler may warn about uninitialized variable in use... > - kfree(gpio_str); > if (!present) ...here. > continue;
Hi Andy, On 08.08.19 15:48, Andy Shevchenko wrote: > On Thu, Aug 08, 2019 at 03:25:43PM +0200, Stefan Roese wrote: >> This patch fixes a backward compatibility issue, when boards use the >> old style GPIO suffix "-gpio" instead of the new "-gpios". This >> potential problem has been introduced by commit d99482673f95 ("serial: >> mctrl_gpio: Check if GPIO property exisits before requesting it"). >> >> This patch now fixes this issue by iterating over all supported GPIO >> suffixes by using the newly introduced for_each_gpio_suffix() helper. >> >> Also, the string buffer is now allocated on the stack to avoid the >> problem of allocation in a loop and its potential failure. > >> for (i = 0; i < UART_GPIO_MAX; i++) { >> enum gpiod_flags flags; >> - char *gpio_str; >> + const char *suffix; >> + char gpio_str[32]; /* 32 is max size of property name */ > > Hmm... don't we have some define for the maximum length of property? I've come up with this assumption from this code (identical comment): https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-of.c#L293 (and other places in drivers/gpio/*) > Or maybe we can still continue using kasprintf() approach? Frankly, I was feeling a bit uncomfortable with this memory allocation in a loop. And Pavel also commented on this: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html So I would really prefer to move this buffer to the stack instead. >> bool present; >> + int k; >> + >> + /* >> + * Check if GPIO property exists and continue if not. Iterate >> + * over all supported GPIO suffixes (foo-gpios vs. foo-gpio). >> + */ >> + for_each_gpio_suffix(k, suffix) { >> + snprintf(gpio_str, sizeof(gpio_str), "%s-%s", >> + mctrl_gpios_desc[i].name, suffix); >> + >> + present = device_property_present(dev, gpio_str); >> + if (present) >> + break; >> + } >> >> - /* Check if GPIO property exists and continue if not */ >> - gpio_str = kasprintf(GFP_KERNEL, "%s-gpios", >> - mctrl_gpios_desc[i].name); >> - if (!gpio_str) >> - continue; >> - > >> - present = device_property_present(dev, gpio_str); > > Because there is no more explicit assignment of present outside of the loop, > the compiler may warn about uninitialized variable in use... > >> - kfree(gpio_str); > >> if (!present) > > ...here. My compiler does not warn (MIPS GCC 8.1) but I see your point. I'll add an initialization in the next version. Thanks, Stefan
On Thu, Aug 08, 2019 at 03:59:36PM +0200, Stefan Roese wrote: > On 08.08.19 15:48, Andy Shevchenko wrote: > > On Thu, Aug 08, 2019 at 03:25:43PM +0200, Stefan Roese wrote: > > > This patch fixes a backward compatibility issue, when boards use the > > > old style GPIO suffix "-gpio" instead of the new "-gpios". This > > > potential problem has been introduced by commit d99482673f95 ("serial: > > > mctrl_gpio: Check if GPIO property exisits before requesting it"). > > > > > > This patch now fixes this issue by iterating over all supported GPIO > > > suffixes by using the newly introduced for_each_gpio_suffix() helper. > > > > > > Also, the string buffer is now allocated on the stack to avoid the > > > problem of allocation in a loop and its potential failure. > > > > > for (i = 0; i < UART_GPIO_MAX; i++) { > > > enum gpiod_flags flags; > > > - char *gpio_str; > > > + const char *suffix; > > > + char gpio_str[32]; /* 32 is max size of property name */ > > > > Hmm... don't we have some define for the maximum length of property? > > I've come up with this assumption from this code (identical comment): > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-of.c#L293 > > (and other places in drivers/gpio/*) I tried hard to find an evidence of this in Linux kernel, I assume that comes from DT compiler or something, but fail. Linux kernel OF properties handling is written in the assumption of arbitrary length of the property name. It might be that my hard was not hard at all and I missed something. > > Or maybe we can still continue using kasprintf() approach? > > Frankly, I was feeling a bit uncomfortable with this memory allocation > in a loop. And Pavel also commented on this: > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html If memory allocator fails, it's a big issue, and what will happen next probably much less important. > So I would really prefer to move this buffer to the stack instead.
Hi Stefan, On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote: > This patch fixes a backward compatibility issue, when boards use the > old style GPIO suffix "-gpio" instead of the new "-gpios". This > potential problem has been introduced by commit d99482673f95 ("serial: > mctrl_gpio: Check if GPIO property exisits before requesting it"). > > This patch now fixes this issue by iterating over all supported GPIO > suffixes by using the newly introduced for_each_gpio_suffix() helper. > > Also, the string buffer is now allocated on the stack to avoid the > problem of allocation in a loop and its potential failure. > > Signed-off-by: Stefan Roese <sr@denx.de> Do we really need to spread this *-gpio" legacy support all over the kernel? Seeing the only in-kernel users of legacy "rts-gpio" are arch/arm/boot/dts/am335x-nano.dts: rts-gpio = <&gpio0 13 GPIO_ACTIVE_HIGH>; arch/arm/boot/dts/am335x-nano.dts: rts-gpio = <&gpio2 15 GPIO_ACTIVE_HIGH>; arch/arm/boot/dts/am335x-pdu001.dts: rts-gpio = <&gpio1 9 GPIO_ACTIVE_HIGH>; and this is handled by omap-serial.c, predating mctrl_gpio, I'd like to reconsider. Documentation/devicetree/bindings/serial/serial.txt always described the "*-gpios" variants, so there should be no users of the legacy "*-gpio" variants. Gr{oetje,eeting}s, Geert
Hi Geert, On 12.08.19 13:17, Geert Uytterhoeven wrote: > Hi Stefan, > > On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote: >> This patch fixes a backward compatibility issue, when boards use the >> old style GPIO suffix "-gpio" instead of the new "-gpios". This >> potential problem has been introduced by commit d99482673f95 ("serial: >> mctrl_gpio: Check if GPIO property exisits before requesting it"). >> >> This patch now fixes this issue by iterating over all supported GPIO >> suffixes by using the newly introduced for_each_gpio_suffix() helper. >> >> Also, the string buffer is now allocated on the stack to avoid the >> problem of allocation in a loop and its potential failure. >> >> Signed-off-by: Stefan Roese <sr@denx.de> > > Do we really need to spread this *-gpio" legacy support all over the kernel? > > Seeing the only in-kernel users of legacy "rts-gpio" are > > arch/arm/boot/dts/am335x-nano.dts: rts-gpio = <&gpio0 13 > GPIO_ACTIVE_HIGH>; > arch/arm/boot/dts/am335x-nano.dts: rts-gpio = <&gpio2 15 > GPIO_ACTIVE_HIGH>; > arch/arm/boot/dts/am335x-pdu001.dts: rts-gpio = <&gpio1 9 > GPIO_ACTIVE_HIGH>; > > and this is handled by omap-serial.c, predating mctrl_gpio, I'd like to > reconsider. > > Documentation/devicetree/bindings/serial/serial.txt always described > the "*-gpios" > variants, so there should be no users of the legacy "*-gpio" variants. Hmmm, you were the one to comment about supporting (not breaking) the "-gpio" variant: https://lkml.org/lkml/2019/6/24/248 That was my main motivation to work on this patch. If we all agree, that only the documented "-gpios" variant needs to be supported, then we can drop this patch. Thanks, Stefan
Hi Stefan, On Mon, Aug 12, 2019 at 1:54 PM Stefan Roese <sr@denx.de> wrote: > On 12.08.19 13:17, Geert Uytterhoeven wrote: > > On Thu, Aug 8, 2019 at 3:25 PM Stefan Roese <sr@denx.de> wrote: > >> This patch fixes a backward compatibility issue, when boards use the > >> old style GPIO suffix "-gpio" instead of the new "-gpios". This > >> potential problem has been introduced by commit d99482673f95 ("serial: > >> mctrl_gpio: Check if GPIO property exisits before requesting it"). > >> > >> This patch now fixes this issue by iterating over all supported GPIO > >> suffixes by using the newly introduced for_each_gpio_suffix() helper. > >> > >> Also, the string buffer is now allocated on the stack to avoid the > >> problem of allocation in a loop and its potential failure. > >> > >> Signed-off-by: Stefan Roese <sr@denx.de> > > > > Do we really need to spread this *-gpio" legacy support all over the kernel? > > > > Seeing the only in-kernel users of legacy "rts-gpio" are > > > > arch/arm/boot/dts/am335x-nano.dts: rts-gpio = <&gpio0 13 > > GPIO_ACTIVE_HIGH>; > > arch/arm/boot/dts/am335x-nano.dts: rts-gpio = <&gpio2 15 > > GPIO_ACTIVE_HIGH>; > > arch/arm/boot/dts/am335x-pdu001.dts: rts-gpio = <&gpio1 9 > > GPIO_ACTIVE_HIGH>; > > > > and this is handled by omap-serial.c, predating mctrl_gpio, I'd like to > > reconsider. > > > > Documentation/devicetree/bindings/serial/serial.txt always described > > the "*-gpios" > > variants, so there should be no users of the legacy "*-gpio" variants. > > Hmmm, you were the one to comment about supporting (not breaking) the > "-gpio" variant: > > https://lkml.org/lkml/2019/6/24/248 > > That was my main motivation to work on this patch. I know. That's why I wrote "reconsider". Before, I assumed there were actual users of the legacy properties, which turned out to be wrong. IMHO no driver should be aware there can be multiple possible suffixes. Sorry for the trouble. > If we all agree, that only the documented "-gpios" variant needs to > be supported, then we can drop this patch. BTW, I'm still wondering if d99482673f950817 ("serial: mctrl_gpio: Check if GPIO property exisits before requesting it") is the right fix. This is a place where we rely explicitly on named GPIOs being present, so IMHO the ACPI code should not return a random GPIO instead. Gr{oetje,eeting}s, Geert
On Mon, Aug 12, 2019 at 02:11:42PM +0200, Geert Uytterhoeven wrote: > On Mon, Aug 12, 2019 at 1:54 PM Stefan Roese <sr@denx.de> wrote: > > On 12.08.19 13:17, Geert Uytterhoeven wrote: > > If we all agree, that only the documented "-gpios" variant needs to > > be supported, then we can drop this patch. > > BTW, I'm still wondering if d99482673f950817 ("serial: mctrl_gpio: Check > if GPIO property exisits before requesting it") is the right fix. > This is a place where we rely explicitly on named GPIOs being present, so > IMHO the ACPI code should not return a random GPIO instead. ACPI is not taking it to consideration for now. That is, there is no support of mctrl gpio for ACPI.
On Mon 2019-08-12 13:53:07, Andy Shevchenko wrote: > On Thu, Aug 08, 2019 at 03:59:36PM +0200, Stefan Roese wrote: > > On 08.08.19 15:48, Andy Shevchenko wrote: > > > On Thu, Aug 08, 2019 at 03:25:43PM +0200, Stefan Roese wrote: > > > > This patch fixes a backward compatibility issue, when boards use the > > > > old style GPIO suffix "-gpio" instead of the new "-gpios". This > > > > potential problem has been introduced by commit d99482673f95 ("serial: > > > > mctrl_gpio: Check if GPIO property exisits before requesting it"). > > > > > > > > This patch now fixes this issue by iterating over all supported GPIO > > > > suffixes by using the newly introduced for_each_gpio_suffix() helper. > > > > > > > > Also, the string buffer is now allocated on the stack to avoid the > > > > problem of allocation in a loop and its potential failure. > > > > > > > for (i = 0; i < UART_GPIO_MAX; i++) { > > > > enum gpiod_flags flags; > > > > - char *gpio_str; > > > > + const char *suffix; > > > > + char gpio_str[32]; /* 32 is max size of property name */ > > > > > > Hmm... don't we have some define for the maximum length of property? > > > > I've come up with this assumption from this code (identical comment): > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-of.c#L293 > > > > (and other places in drivers/gpio/*) > > I tried hard to find an evidence of this in Linux kernel, I assume that comes > from DT compiler or something, but fail. Linux kernel OF properties handling is > written in the assumption of arbitrary length of the property name. > > It might be that my hard was not hard at all and I missed something. > > > > Or maybe we can still continue using kasprintf() approach? > > > > Frankly, I was feeling a bit uncomfortable with this memory allocation > > in a loop. And Pavel also commented on this: > > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html > > If memory allocator fails, it's a big issue, and what will happen next probably > much less important. Not... really. With "too big" allocations, it will fail. Anyway, my point is that allocating in a loop for this is slow and ugly. If we don't have a maximum property length, we should probably invent some. I mean, we can agree that 64KB property name is not okay, right? Pavel
On Tue, Aug 13, 2019 at 01:42:58PM +0200, Pavel Machek wrote: > On Mon 2019-08-12 13:53:07, Andy Shevchenko wrote: > > On Thu, Aug 08, 2019 at 03:59:36PM +0200, Stefan Roese wrote: > > > On 08.08.19 15:48, Andy Shevchenko wrote: > > > > On Thu, Aug 08, 2019 at 03:25:43PM +0200, Stefan Roese wrote: > > I tried hard to find an evidence of this in Linux kernel, I assume that comes > > from DT compiler or something, but fail. Linux kernel OF properties handling is > > written in the assumption of arbitrary length of the property name. > > > > It might be that my hard was not hard at all and I missed something. > > > > > > Or maybe we can still continue using kasprintf() approach? > > > > > > Frankly, I was feeling a bit uncomfortable with this memory allocation > > > in a loop. And Pavel also commented on this: > > > > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2066286.html > > > > If memory allocator fails, it's a big issue, and what will happen next probably > > much less important. > > Not... really. With "too big" allocations, it will fail. Yeah, or with relatively small if memory is too much fragmented. But we are talking about really small allocations here in most cases, right? > Anyway, my point is that allocating in a loop for this is slow and > ugly. If we don't have a maximum property length, we should probably > invent some. I mean, we can agree that 64KB property name is not okay, > right? My point that is should be declared on the level of property API. Restricting it on the level of one, 'client' to that API, framework is not a solution.
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c index 2b400189be91..d444fdaa280a 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.c +++ b/drivers/tty/serial/serial_mctrl_gpio.c @@ -15,6 +15,7 @@ #include <linux/property.h> #include "serial_mctrl_gpio.h" +#include "../../gpio/gpiolib.h" struct mctrl_gpios { struct uart_port *port; @@ -117,17 +118,24 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) for (i = 0; i < UART_GPIO_MAX; i++) { enum gpiod_flags flags; - char *gpio_str; + const char *suffix; + char gpio_str[32]; /* 32 is max size of property name */ bool present; + int k; + + /* + * Check if GPIO property exists and continue if not. Iterate + * over all supported GPIO suffixes (foo-gpios vs. foo-gpio). + */ + for_each_gpio_suffix(k, suffix) { + snprintf(gpio_str, sizeof(gpio_str), "%s-%s", + mctrl_gpios_desc[i].name, suffix); + + present = device_property_present(dev, gpio_str); + if (present) + break; + } - /* Check if GPIO property exists and continue if not */ - gpio_str = kasprintf(GFP_KERNEL, "%s-gpios", - mctrl_gpios_desc[i].name); - if (!gpio_str) - continue; - - present = device_property_present(dev, gpio_str); - kfree(gpio_str); if (!present) continue;
This patch fixes a backward compatibility issue, when boards use the old style GPIO suffix "-gpio" instead of the new "-gpios". This potential problem has been introduced by commit d99482673f95 ("serial: mctrl_gpio: Check if GPIO property exisits before requesting it"). This patch now fixes this issue by iterating over all supported GPIO suffixes by using the newly introduced for_each_gpio_suffix() helper. Also, the string buffer is now allocated on the stack to avoid the problem of allocation in a loop and its potential failure. Signed-off-by: Stefan Roese <sr@denx.de> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Pavel Machek <pavel@denx.de> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/tty/serial/serial_mctrl_gpio.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)