gpiolib: check if irqchip already has the irq hook replacements

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
---

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