Re: [PATCH] let setup_irq reenable a shared irq

Re: [PATCH] let setup_irq reenable a shared irq

[Thomas Gleixner]

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

On Wed, 30 Apr 2008, Uwe Kleine-König wrote:
> From: Thomas Gleixner <tglx@linutronix.de>

I have the fixed version already queued for linus since yesterday :)

Thanks,
	tlgx

Re: [PATCH] let setup_irq reenable a shared irq

[Andrew Morton]

On Wed, 30 Apr 2008 14:32:58 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 30 Apr 2008, Uwe Kleine-König wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> 
> I have the fixed version already queued for linus since yesterday :)
> 

Where is it queued?

Re: [PATCH] let setup_irq reenable a shared irq

[Thomas Gleixner]

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

On Wed, 30 Apr 2008, Andrew Morton wrote:

> On Wed, 30 Apr 2008 14:32:58 +0200 (CEST) Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Wed, 30 Apr 2008, Uwe Kleine-König wrote:
> > > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > I have the fixed version already queued for linus since yesterday :)
> > 
> 
> Where is it queued?

In my crap pile which is in preparation for a git update of
http://git.kernel.org/?p=linux/kernel/git/tglx/linux-2.6-genirq.git
 
Thanks,
	tglx

[PATCH] let setup_irq reenable a shared irq

[Uwe Kleine-König]

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.

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

Re: [PATCH] let setup_irq reenable a shared irq

[Thomas Gleixner]

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

Re: [PATCH] let setup_irq reenable a shared irq

[Uwe Kleine-König]

Hello,

Thomas Gleixner wrote:
> 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.
mmpf.

It's not nice to use disable_irq()/enable_irq() in a driver, is it?
 
> 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 ?
It's a ns9215 SoC.  The rtc component has a clock flag that I need to
set before accessing the address space.  After enabling the clock flag
I get an irq if the rtc is up.  The rtc itself uses the same irq.  As I
want to handle the clk enabling in platform code and the actual rtc in a
driver I need a shared irq.
Now the problem is that the rtc remembers its alarm flags over
reboots[1] and starts asserting the irq when the platform code has
requested the irq.

I currently see two ways to handle that:
 1) find an alternative patch similar to the one I sent that don't break
    driver code; or
 2) let the platform code disable the rtc's irqs.

for 1) you need to remember the reason for disabling the irq (or fix all
drivers not to use disable_irq()).  And 2) is ugly because I have to
ioremap then.

Best regards
Uwe

[1] not entirely sure, but I had the rtc in a state in which the rtc
component asserted an irq directly after a reset.

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

Re: [PATCH] let setup_irq reenable a shared irq

[Thomas Gleixner]

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

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)

> > Waht kind of scenario/devices do you have which trigger this ?
>
> It's a ns9215 SoC.  The rtc component has a clock flag that I need to
> set before accessing the address space.  After enabling the clock flag
> I get an irq if the rtc is up.  The rtc itself uses the same irq.  As I
> want to handle the clk enabling in platform code and the actual rtc in a
> driver I need a shared irq.
> Now the problem is that the rtc remembers its alarm flags over
> reboots[1] and starts asserting the irq when the platform code has
> requested the irq.
> 
> I currently see two ways to handle that:
>  1) find an alternative patch similar to the one I sent that don't break
>     driver code; or
>  2) let the platform code disable the rtc's irqs.
> 
> for 1) you need to remember the reason for disabling the irq (or fix all
> drivers not to use disable_irq()).  And 2) is ugly because I have to
> ioremap then.

1) makes a lot of sense, as we can see those stupid device/bios keeps
irq asserted problems once in a while. Giving the irq a new chance
when a new driver arrives is definitely not a bad idea. See patch
below.

Reworking all the drivers would be a nice but definitely non trivial
project and it needs the acceptance of all the affected subsystem
maintainers.

2) ioremap is not a really big deal, so that's not a good argument not
to put hardware back into the state which was found when we started
the box, especially when we know that it does stupid things
eventually.

Thanks,
	tglx

------------>

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 */
 
 #ifdef CONFIG_IRQ_PER_CPU
 # define CHECK_IRQ_PER_CPU(var) ((var) & IRQ_PER_CPU)
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/kernel/irq/manage.c
@@ -149,6 +149,26 @@ void disable_irq(unsigned int irq)
 }
 EXPORT_SYMBOL(disable_irq);
 
+static void __enable_irq(struct irq_desc *desc, unsigned int irq)
+{
+	switch (desc->depth) {
+	case 0:
+		printk(KERN_WARNING "Unbalanced enable for IRQ %d\n", irq);
+		WARN_ON(1);
+		break;
+	case 1: {
+		unsigned int status = desc->status & ~IRQ_DISABLED;
+
+		/* Prevent probing on this irq: */
+		desc->status = status | IRQ_NOPROBE;
+		check_irq_resend(desc, irq);
+		/* fall-through */
+	}
+	default:
+		desc->depth--;
+	}
+}
+
 /**
  *	enable_irq - enable handling of an irq
  *	@irq: Interrupt to enable
@@ -168,22 +188,7 @@ void enable_irq(unsigned int irq)
 		return;
 
 	spin_lock_irqsave(&desc->lock, flags);
-	switch (desc->depth) {
-	case 0:
-		printk(KERN_WARNING "Unbalanced enable for IRQ %d\n", irq);
-		WARN_ON(1);
-		break;
-	case 1: {
-		unsigned int status = desc->status & ~IRQ_DISABLED;
-
-		/* Prevent probing on this irq: */
-		desc->status = status | IRQ_NOPROBE;
-		check_irq_resend(desc, irq);
-		/* fall-through */
-	}
-	default:
-		desc->depth--;
-	}
+	__enable_irq(desc, irq);
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
 EXPORT_SYMBOL(enable_irq);
@@ -364,7 +369,7 @@ int setup_irq(unsigned int irq, struct i
 			compat_irq_chip_set_default_handler(desc);
 
 		desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING |
-				  IRQ_INPROGRESS);
+				  IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED);
 
 		if (!(desc->status & IRQ_NOAUTOEN)) {
 			desc->depth = 0;
@@ -380,6 +385,16 @@ int setup_irq(unsigned int irq, struct i
 	/* Reset broken irq detection when installing new handler */
 	desc->irq_count = 0;
 	desc->irqs_unhandled = 0;
+
+	/*
+	 * Check whether we disabled the irq via the spurious handler
+	 * before. Reenable it and give it another chance.
+	 */
+	if (shared && (desc->status & IRQ_SPURIOUS_DISABLED)) {
+		desc->status &= ~IRQ_SPURIOUS_DISABLED;
+		__enable_irq(desc, irq);
+	}
+
 	spin_unlock_irqrestore(&desc->lock, flags);
 
 	new->irq = irq;
Index: linux-2.6/kernel/irq/spurious.c
===================================================================
--- linux-2.6.orig/kernel/irq/spurious.c
+++ linux-2.6/kernel/irq/spurious.c
@@ -209,8 +209,8 @@ void note_interrupt(unsigned int irq, st
 		 * Now kill the IRQ
 		 */
 		printk(KERN_EMERG "Disabling IRQ #%d\n", irq);
-		desc->status |= IRQ_DISABLED;
-		desc->depth = 1;
+		desc->status |= IRQ_DISABLED | IRQ_SPURIOUS_DISABLED;
+		desc->depth++;
 		desc->chip->disable(irq);
 	}
 	desc->irqs_unhandled = 0;

Re: [PATCH] let setup_irq reenable a shared irq

[Uwe Kleine-König]

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

Re: [PATCH] let setup_irq reenable a shared irq

[Thomas Gleixner]

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

On Tue, 29 Apr 2008, Uwe Kleine-König wrote:
> > @@ -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?

Doh, of course not :)

Thanks,
	tglx

Re: [PATCH] let setup_irq reenable a shared irq

[Eric W. Biederman]

Thomas Gleixner <tglx@linutronix.de> writes:
 >
> 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.

This case also happens on a regular basis in kdump kernels where we
deliberately don't shutdown the hardware before starting the new kernel.
This patch should reduce the need for using irqpoll in that situation
by a small amount.

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

Looks like a duplicate define here. Don't you want:
+#define IRQ_SPURIOUS_DISABLED 0x00800000 /* IRQ was disabled by the spurious trap */

Re: [PATCH] let setup_irq reenable a shared irq

[Thomas Gleixner]

On Wed, 30 Apr 2008, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>  >
> > 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.
> 
> This case also happens on a regular basis in kdump kernels where we
> deliberately don't shutdown the hardware before starting the new kernel.
> This patch should reduce the need for using irqpoll in that situation
> by a small amount.

Makes sense.
 
> > 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 */
> 
> Looks like a duplicate define here. Don't you want:
> +#define IRQ_SPURIOUS_DISABLED 0x00800000 /* IRQ was disabled by the spurious trap */

Your the third one to notice :)

Thanks,
	tglx