| Message ID | 1461914677-24899-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Fri, Apr 29, 2016 at 9:24 AM, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Currrently the gpio_chip.to_irq() callback returns -ENOSYS on error, > which causes bad interactions with the serial_mctrl_gpio helpers. > > mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is > intended to be ignored by its callers. However, ignoring -ENOSYS when it > was caused by a gpiod_to_irq() failure will lead to a crash later: > > Unable to handle kernel paging request at virtual address ffffffde > ... > PC is at mctrl_gpio_set+0x14/0x78 > > Fix this by returning -ENXIO instead. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Is -ENXIO the right error code? I would say that whatever the generic helpers in drivers/gpio/gpiolib.c returns should be the norm. However the guy who wrote them (me) isn't very smart either. It looks like so: static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { return irq_find_mapping(chip->irqdomain, offset); } That returns 0 (NO_IRQ) on failure. And as you say: > - Drivers that call irq_find_mapping(), irq_create_mapping(), or > irq_create_fwspec_mapping() return zero! This also applies to the > core helper gpiochip_to_irq(). Zero means NO_IRQ. Reminder: http://lwn.net/Articles/470820/ What we should do is patch all drivers to return 0 on failure, and patch any consumers like mctrl_gpio_init() to handle that correctly. Yours, Linus Walleij
Hi Linus, On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> Currrently the gpio_chip.to_irq() callback returns -ENOSYS on error, >> which causes bad interactions with the serial_mctrl_gpio helpers. >> >> mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is >> intended to be ignored by its callers. However, ignoring -ENOSYS when it >> was caused by a gpiod_to_irq() failure will lead to a crash later: >> >> Unable to handle kernel paging request at virtual address ffffffde >> ... >> PC is at mctrl_gpio_set+0x14/0x78 >> >> Fix this by returning -ENXIO instead. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> Is -ENXIO the right error code? > > I would say that whatever the generic helpers in drivers/gpio/gpiolib.c > returns should be the norm. However the guy who wrote them (me) > isn't very smart either. It looks like so: > > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > return irq_find_mapping(chip->irqdomain, offset); > } > > That returns 0 (NO_IRQ) on failure. > > And as you say: > >> - Drivers that call irq_find_mapping(), irq_create_mapping(), or >> irq_create_fwspec_mapping() return zero! This also applies to the >> core helper gpiochip_to_irq(). > > Zero means NO_IRQ. > > Reminder: > http://lwn.net/Articles/470820/ > > What we should do is patch all drivers to return 0 on failure, and > patch any consumers like mctrl_gpio_init() to handle that correctly. That's the Long Term Plan. There are still too many non-zero NO_IRQ definitions in use... Is -ENXIO acceptable for the short term? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, May 2, 2016 at 10:03 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>> - Drivers that call irq_find_mapping(), irq_create_mapping(), or >>> irq_create_fwspec_mapping() return zero! This also applies to the >>> core helper gpiochip_to_irq(). >> >> Zero means NO_IRQ. >> >> Reminder: >> http://lwn.net/Articles/470820/ >> >> What we should do is patch all drivers to return 0 on failure, and >> patch any consumers like mctrl_gpio_init() to handle that correctly. > > That's the Long Term Plan. There are still too many non-zero NO_IRQ > definitions in use... > > Is -ENXIO acceptable for the short term? I don't understand. You say you have a problem with mctrl_gpio_init() which looks like this: ret = gpiod_to_irq(gpios->gpio[i]); if (ret <= 0) { dev_err(port->dev, (...) This function is already *correctly* handling zero as NO_IRQ i.e. an error. Just make your driver return 0/NO_IRQ and it is fixed. Or are there other problems that you're not telling about? Yours, Linus Walleij
Hi Linus, On Mon, May 2, 2016 at 10:30 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, May 2, 2016 at 10:03 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>> - Drivers that call irq_find_mapping(), irq_create_mapping(), or >>>> irq_create_fwspec_mapping() return zero! This also applies to the >>>> core helper gpiochip_to_irq(). >>> >>> Zero means NO_IRQ. >>> >>> Reminder: >>> http://lwn.net/Articles/470820/ >>> >>> What we should do is patch all drivers to return 0 on failure, and >>> patch any consumers like mctrl_gpio_init() to handle that correctly. >> >> That's the Long Term Plan. There are still too many non-zero NO_IRQ >> definitions in use... >> >> Is -ENXIO acceptable for the short term? > > I don't understand. You say you have a problem with > mctrl_gpio_init() which looks like this: > > ret = gpiod_to_irq(gpios->gpio[i]); > if (ret <= 0) { > dev_err(port->dev, (...) > > This function is already *correctly* handling zero as NO_IRQ > i.e. an error. > > Just make your driver return 0/NO_IRQ and it is fixed. > > Or are there other problems that you're not telling about? "mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is intended to be ignored by its callers. However, ignoring -ENOSYS when it was caused by a gpiod_to_irq() failure will lead to a crash later:" Please see the !CONFIG_GPIOLIB stubs in drivers/tty/serial/serial_mctrl_gpio.h. Hence -ENOSYS is to be ignored by the driver, and it's safe to call any of the mctrl_gpio_*() helpers if !CONFIG_GPIOLIB. That was done so drivers don't have to check for !CONFIG_GPIOLIB theirselves. However, if CONFIG_GPIOLIB=y, and -ENOSYS was a real error, calling any of the mctrl_gpio_*() helpers will cause a crash. Without testing for CONFIG_GPIOLIB, there's no way the driver can know -ENOSYS was a real error. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Linus, On Mon, May 2, 2016 at 10:53 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, May 2, 2016 at 10:30 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Mon, May 2, 2016 at 10:03 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>>> - Drivers that call irq_find_mapping(), irq_create_mapping(), or >>>>> irq_create_fwspec_mapping() return zero! This also applies to the >>>>> core helper gpiochip_to_irq(). >>>> >>>> Zero means NO_IRQ. >>>> >>>> Reminder: >>>> http://lwn.net/Articles/470820/ >>>> >>>> What we should do is patch all drivers to return 0 on failure, and >>>> patch any consumers like mctrl_gpio_init() to handle that correctly. >>> >>> That's the Long Term Plan. There are still too many non-zero NO_IRQ >>> definitions in use... >>> >>> Is -ENXIO acceptable for the short term? >> >> I don't understand. You say you have a problem with >> mctrl_gpio_init() which looks like this: >> >> ret = gpiod_to_irq(gpios->gpio[i]); >> if (ret <= 0) { >> dev_err(port->dev, (...) >> >> This function is already *correctly* handling zero as NO_IRQ >> i.e. an error. >> >> Just make your driver return 0/NO_IRQ and it is fixed. >> >> Or are there other problems that you're not telling about? [silly response deleted] Scrap it. The only annoying thing is that 0 cannot easily be propagated upstream as an error code, so it has to be tested for explicitly. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c index a6681b8b17c3b30c..2fffb9c32231adb3 100644 --- a/drivers/pinctrl/sh-pfc/gpio.c +++ b/drivers/pinctrl/sh-pfc/gpio.c @@ -212,7 +212,7 @@ static int gpio_pin_to_irq(struct gpio_chip *gc, unsigned offset) } } - return -ENOSYS; + return -ENXIO; found: return pfc->irqs[i];
Currrently the gpio_chip.to_irq() callback returns -ENOSYS on error, which causes bad interactions with the serial_mctrl_gpio helpers. mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is intended to be ignored by its callers. However, ignoring -ENOSYS when it was caused by a gpiod_to_irq() failure will lead to a crash later: Unable to handle kernel paging request at virtual address ffffffde ... PC is at mctrl_gpio_set+0x14/0x78 Fix this by returning -ENXIO instead. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Is -ENXIO the right error code? - Most drivers seem to return -ENXIO on failure, just like gpiod_to_irq() does when no .to_irq() callback is provided by the driver, - Some drivers use -EINVAL, - Drivers that call irq_find_mapping(), irq_create_mapping(), or irq_create_fwspec_mapping() return zero! This also applies to the core helper gpiochip_to_irq(). --- drivers/pinctrl/sh-pfc/gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)