Re: [PATCH] let setup_irq reenable a shared irq

["Uwe Kleine-König" <Uwe.Kleine-Koenig@digi.com>]

Hello,

Thomas Gleixner wrote:
> On Mon, 28 Apr 2008, Uwe Kleine-König wrote:
> > > 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.
> > mmpf.
> > 
> > It's not nice to use disable_irq()/enable_irq() in a driver, is it?
> 
> Well, it's not nice, but it's there (in rather large quantities)
Ah, and now I finally understood desc->depth ...
 
> Subject: genirq: reenable a nobody cared disabled irq when a new driver arrives
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Mon, 28 Apr 2008 17:01:56 +0200
> 
> Uwe Kleine-Koenig has some strange hardware where one of the shared
> interrupts can be asserted during boot before the appropriate driver
> loads. Requesting the shared irq line from another driver results in a
> spurious interrupt storm which finally disables the interrupt line.
> 
> I have seen similar behaviour on resume before (the hardware does not
> work anymore so I can not verify) and this spurious irq issue is
> raised on a regular base in bugreports.
> 
> Change the spurious disable logic to increment the disable depth and
> mark the interrupt with an extra flag which allows us to reenable the
> interrupt when a new driver arrives which requests the same irq
> line. In the worst case this will disable the irq again via the
> spurious trap, but there is a decent chance that the new driver is the
> one which can handle the already asserted interrupt and makes the box
> usable again.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Ingo Molnar <mingo@elte.hu>
> ---
>  include/linux/irq.h   |    1 +
>  kernel/irq/manage.c   |   49 ++++++++++++++++++++++++++++++++-----------------
>  kernel/irq/spurious.c |    4 ++--
>  3 files changed, 35 insertions(+), 19 deletions(-)
> 
> Index: linux-2.6/include/linux/irq.h
> ===================================================================
> --- linux-2.6.orig/include/linux/irq.h
> +++ linux-2.6/include/linux/irq.h
> @@ -61,6 +61,7 @@ typedef	void (*irq_flow_handler_t)(unsig
>  #define IRQ_WAKEUP		0x00100000	/* IRQ triggers system wakeup */
>  #define IRQ_MOVE_PENDING	0x00200000	/* need to re-target IRQ destination */
>  #define IRQ_NO_BALANCING	0x00400000	/* IRQ is excluded from balancing */
> +#define IRQ_SPURIOUS_DISABLED	0x00400000	/* IRQ was disabled by the spurious trap */
Is it intended that IRQ_NO_BALANCING == IRQ_SPURIOUS_DISABLED?

Other than that

Tested-and-Acked-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>

Best regards and thanks
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962