Re: [PATCH] gpiolib: check if irqchip already has the irq hook replacements

[Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx>]

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@xxxxxxxxx>
Tested-by: Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx>

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;