Re: [PATCH] let setup_irq reenable a shared irq

[Thomas Gleixner <tglx@linutronix.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2129 bytes --]

On Mon, 28 Apr 2008, Uwe Kleine-König wrote:
> Consider two devices A and B sharing an irq and B already asserts the irq on
> a booting machine.
> If the driver for A is loaded first the irq starts triggering and gets
> disabled after some time by note_interrupt().  Later when the driver for B
> is loaded the interrupt should be reenabled---other wise both A and B don't
> work properly.

Oh no. There is lots of code in drivers, which does:

   disable_irq();
   do_some_protected_stuff();
   enable_irq();

So when the second driver is loaded on another CPU it would see the
IRQ_DISABLED bit set and unconditionally reenable the interrupt. 

This unprotects the protected operation and definitely triggers the
WARN_ON() in enable_irq() where we check for desc->depth == 0.

I can see the rationale for your patch, as we have no way to silence
stupid hardware or hardware which was left in that state by the BIOS
other than disabling the interrupt line completely.

Waht kind of scenario/devices do you have which trigger this ?

Thanks,
	tglx

> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/irq/manage.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 438a014..831ebb2 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -365,11 +365,18 @@ int setup_irq(unsigned int irq, struct irqaction *new)
>  
>  		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
>  				  IRQ_INPROGRESS);
> +	}
> +
> +	/* in the shared case the interrupt might be disabled if a device
> +	 * asserts the irq but the corresponding driver wasn't the first to be
> +	 * loaded.  So retry enabling the irq.
> +	 */
> +	if (!shared || desc->status & IRQ_DISABLED) {
>  
>  		if (!(desc->status & IRQ_NOAUTOEN)) {
>  			desc->depth = 0;
>  			desc->status &= ~IRQ_DISABLED;
> -			if (desc->chip->startup)
> +			if (!shared && desc->chip->startup)
>  				desc->chip->startup(irq);
>  			else
>  				desc->chip->enable(irq);
> -- 
> 1.5.5.1
>