Message ID | f0c57681-03da-eb79-a931-d239bf47e0d9@xs4all.nl |
---|---|
State | New |
Headers | show |
Series | gpiolib: check if irqchip already has the irq hook replacements | expand |
On Fri, Sep 14, 2018 at 10:36 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > Some drivers use a single irqchip for multiple gpiochips. As a result the > irqchip hooks are overridden for the first gpiochip that was added, but > for the other gpiochip instances this should not happen again, otherwise > we would go into an infinite recursion. > > Check for this, but also log a message that the driver should be fixed > since this is bad practice. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> Patch tentatively applied, let's see if we can also get some Tested-by from Ludovic! Yours, Linus Walleij
On Fri, Sep 14, 2018 at 10:36:39AM +0200, Hans Verkuil wrote: > Some drivers use a single irqchip for multiple gpiochips. As a result the > irqchip hooks are overridden for the first gpiochip that was added, but > for the other gpiochip instances this should not happen again, otherwise > we would go into an infinite recursion. > > Check for this, but also log a message that the driver should be fixed > since this is bad practice. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> Tested-by: Ludovic Desroches <ludovic.desroches@microchip.com> Thanks for the fix. > --- > Ludovic, can you test this with your unmodified driver? In particular I'd > like to see how many of these messages you get and how they look like in > the kernel log. I think you should get 4 of these messages. > Here are the logs: pinctrl core: initialized pinctrl subsystem NET: Registered protocol family 16 DMA: preallocated 256 KiB pool for atomic coherent allocations AT91: PM: standby: standby, suspend: ulp0 No ATAGs? gpio-at91 fffff200.gpio: at address (ptrval) gpio gpiochip1: (fffff400.gpio): detected irqchip that is shared with multiple gpiochips: please fix the driver. gpio-at91 fffff400.gpio: at address (ptrval) gpio gpiochip2: (fffff600.gpio): detected irqchip that is shared with multiple gpiochips: please fix the driver. gpio-at91 fffff600.gpio: at address (ptrval) gpio gpiochip3: (fffff800.gpio): detected irqchip that is shared with multiple gpiochips: please fix the driver. gpio-at91 fffff800.gpio: at address (ptrval) gpio gpiochip4: (fffffa00.gpio): detected irqchip that is shared with multiple gpiochips: please fix the driver. gpio-at91 fffffa00.gpio: at address (ptrval) pinctrl-at91 ahb:apb:pinctrl@fffff200: initialized AT91 pinctrl driver clocksource: tcb_clksrc: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 115833966437 ns at_hdmac ffffe600.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), 8 channels at_hdmac ffffe800.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), 8 channels Regards Ludovic > Thanks! > > Hans > --- > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index efce534a269b..a29c917b459f 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1859,6 +1859,16 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip) > } > if (WARN_ON(gpiochip->irq.irq_enable)) > return; > + /* Check if the irqchip already has this hook... */ > + if (irqchip->irq_enable == gpiochip_irq_enable) { > + /* > + * ...and if so, give a gentle warning that this is bad > + * practice. > + */ > + chip_info(gpiochip, > + "detected irqchip that is shared with multiple gpiochips: please fix the driver.\n"); > + return; > + } > gpiochip->irq.irq_enable = irqchip->irq_enable; > gpiochip->irq.irq_disable = irqchip->irq_disable; > irqchip->irq_enable = gpiochip_irq_enable;
On Fri, Sep 14, 2018 at 10:36:39AM +0200, Hans Verkuil wrote: > Some drivers use a single irqchip for multiple gpiochips. As a result the > irqchip hooks are overridden for the first gpiochip that was added, but > for the other gpiochip instances this should not happen again, otherwise > we would go into an infinite recursion. > > Check for this, but also log a message that the driver should be fixed > since this is bad practice. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > Tested-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > Ludovic, can you test this with your unmodified driver? In particular I'd > like to see how many of these messages you get and how they look like in > the kernel log. I think you should get 4 of these messages. > Hee is the output from a qemu test run (realview-pb-a8, realview_defconfig). pl061_gpio 10013000.gpio: PL061 GPIO chip @0x10013000 registered gpio gpiochip1: (10014000.gpio): detected irqchip that is shared with multiple gpiochips: please fix the driver. pl061_gpio 10014000.gpio: PL061 GPIO chip @0x10014000 registered gpio gpiochip2: (10015000.gpio): detected irqchip that is shared with multiple gpiochips: please fix the driver. pl061_gpio 10015000.gpio: PL061 GPIO chip @0x10015000 registered Tested-by: Guenter Roeck <linux@roeck-us.net> Guenter
Hi Linus, On 09/14/2018 10:47 AM, Linus Walleij wrote: > On Fri, Sep 14, 2018 at 10:36 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> Some drivers use a single irqchip for multiple gpiochips. As a result the >> irqchip hooks are overridden for the first gpiochip that was added, but >> for the other gpiochip instances this should not happen again, otherwise >> we would go into an infinite recursion. >> >> Check for this, but also log a message that the driver should be fixed >> since this is bad practice. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > Patch tentatively applied, let's see if we can also get some > Tested-by from Ludovic! > > Yours, > Linus Walleij > I have no idea whether it is possible or not, but I think it would be a good idea to merge this patch with the patch that introduced this problem. This issue can cause havoc during bisecting for devices that share a single irqchip with multiple gpiochips and I'm not sure you want to get that in the mainline kernel. It would mean rebasing the linux-gpio git tree and if that's impossible, then so be it, but it might be worth the hassle to avoid a bisecting hell. I know the linux-media tree is rebased in rare cases to avoid similar issues. Regards, Hans
On Sun, Sep 16, 2018 at 3:38 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > I have no idea whether it is possible or not, but I think it would be a > good idea to merge this patch with the patch that introduced this problem. (...) > It would mean rebasing the linux-gpio git tree and if that's impossible, > then so be it, but it might be worth the hassle to avoid a bisecting hell. I can do this. git log --oneline: 171948ea33e1 gpiolib: check if irqchip already has the irq hook replacements d0121b8548bc gpiolib: use better errno if get_direction is not available fa38869b0161 gpiolib: Don't support irq sharing for userspace 0b35cd7b1860 gpio: uapi: Grammar s/array/array of/ 36e2add18225 gpio: vf610: Cut down on boilerplate 45e8296cc9a2 gpio: vf610: Include the right header 8734fae64eb0 gpio: of: make example syntactically correct 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings 1c939cb556b9 gpio-bcm-kona: use new req/relres and dis/enable_irq funcs 4f8183ae7092 gpio/driver.rst: document gpiochip_disable/enable_irq() 461c1a7d4733 gpiolib: override irq_enable/disable 4e9439ddacea gpiolib: add flag to indicate if the irq is disabled ca620f2de153 gliolib: set hooks in gpiochip_set_irq_hooks() 4e6b823867e2 gpiolib: export gpiochip_irq_reqres/relres() Which commit shall I merge commit 1719(..) into? It has no Fixes: tag.. I could try and educated guess but better ask. Yours, Linus Walleij
On Fri, Sep 14, 2018 at 10:37 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > Some drivers use a single irqchip for multiple gpiochips. As a result the > irqchip hooks are overridden for the first gpiochip that was added, but > for the other gpiochip instances this should not happen again, otherwise > we would go into an infinite recursion. > > Check for this, but also log a message that the driver should be fixed > since this is bad practice. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> Looks like there may still be several drivers that would trigger this. On next-20190121: git grep -l "static struct irq_chip" -- $(git grep -l gpiochip_irqchip_add) drivers/bcma/driver_gpio.c drivers/gpio/gpio-104-dio-48e.c drivers/gpio/gpio-104-idi-48.c drivers/gpio/gpio-104-idio-16.c drivers/gpio/gpio-adnp.c drivers/gpio/gpio-altera.c drivers/gpio/gpio-aspeed.c drivers/gpio/gpio-ath79.c drivers/gpio/gpio-cadence.c drivers/gpio/gpio-crystalcove.c drivers/gpio/gpio-dln2.c drivers/gpio/gpio-ep93xx.c drivers/gpio/gpio-ftgpio010.c drivers/gpio/gpio-intel-mid.c drivers/gpio/gpio-lynxpoint.c drivers/gpio/gpio-max732x.c drivers/gpio/gpio-merrifield.c drivers/gpio/gpio-mt7621.c drivers/gpio/gpio-pca953x.c drivers/gpio/gpio-pcf857x.c drivers/gpio/gpio-pci-idio-16.c drivers/gpio/gpio-pcie-idio-24.c drivers/gpio/gpio-stmpe.c drivers/gpio/gpio-tc3589x.c drivers/gpio/gpio-vf610.c drivers/gpio/gpio-wcove.c drivers/gpio/gpio-ws16c48.c drivers/gpio/gpio-xlp.c drivers/gpio/gpio-zx.c drivers/gpio/gpio-zynq.c drivers/hid/hid-cp2112.c drivers/pinctrl/bcm/pinctrl-bcm2835.c drivers/pinctrl/bcm/pinctrl-iproc-gpio.c drivers/pinctrl/intel/pinctrl-baytrail.c drivers/pinctrl/intel/pinctrl-cherryview.c drivers/pinctrl/intel/pinctrl-intel.c drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c drivers/pinctrl/pinctrl-amd.c drivers/pinctrl/pinctrl-coh901.c drivers/pinctrl/pinctrl-mcp23s08.c drivers/pinctrl/pinctrl-ocelot.c drivers/pinctrl/pinctrl-st.c drivers/pinctrl/sirf/pinctrl-atlas7.c drivers/pinctrl/sirf/pinctrl-sirf.c drivers/pinctrl/spear/pinctrl-plgpio.c drivers/platform/x86/intel_int0002_vgpio.c Not all of them may have multiple instances, though. I saw patches for some of them. Gr{oetje,eeting}s, Geert
On Mon, Jan 21, 2019 at 11:37 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Looks like there may still be several drivers that would trigger this. > On next-20190121: (...) > Not all of them may have multiple instances, though. > I saw patches for some of them. If I read the code right the warning will not be printed until you try to register the same irqchip twice, on the second registration. If you just register one chip which is static const, nothing happens. I think we have fixed most ... but I need to fix some more still. Yours, Linus Walleij
Hi Linus, On Mon, Jan 21, 2019 at 1:13 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jan 21, 2019 at 11:37 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > Looks like there may still be several drivers that would trigger this. > > On next-20190121: > (...) > > Not all of them may have multiple instances, though. > > I saw patches for some of them. > > If I read the code right the warning will not be printed until > you try to register the same irqchip twice, on the second > registration. > > If you just register one chip which is static const, nothing > happens. Correct. > I think we have fixed most ... but I need to fix some more > still. Indeed. There are a few more which are i2c GPIO expanders, which can easily have multiple instances. Searching in the mailing list archives for the printed message revealed a vf610 log showing 4 instances. Gr{oetje,eeting}s, Geert
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index efce534a269b..a29c917b459f 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1859,6 +1859,16 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip) } if (WARN_ON(gpiochip->irq.irq_enable)) return; + /* Check if the irqchip already has this hook... */ + if (irqchip->irq_enable == gpiochip_irq_enable) { + /* + * ...and if so, give a gentle warning that this is bad + * practice. + */ + chip_info(gpiochip, + "detected irqchip that is shared with multiple gpiochips: please fix the driver.\n"); + return; + } gpiochip->irq.irq_enable = irqchip->irq_enable; gpiochip->irq.irq_disable = irqchip->irq_disable; irqchip->irq_enable = gpiochip_irq_enable;
Some drivers use a single irqchip for multiple gpiochips. As a result the irqchip hooks are overridden for the first gpiochip that was added, but for the other gpiochip instances this should not happen again, otherwise we would go into an infinite recursion. Check for this, but also log a message that the driver should be fixed since this is bad practice. Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> --- Ludovic, can you test this with your unmodified driver? In particular I'd like to see how many of these messages you get and how they look like in the kernel log. I think you should get 4 of these messages. Thanks! Hans ---