PATCH to fix irq sharing and SA_INTERRUPT on x86. please review

PATCH to fix irq sharing and SA_INTERRUPT on x86. please review

[James Harper]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

further to my email yesterday (to which i've had no response :) i 
propose the attached patch to arch/i386/kernel/irq.c. it corrects what i 
see as a bug in interrupt handling.

currently if a driver requests SA_INTERRUPT in an interrupt handler, it 
is only called with interrupts disabled if it is the first handler in 
the list.

my patch modifies setup_irq to put any interrupt with SA_INTERRUPT in 
the front of the handler queue (eg before any handlers without the flag).

and also modifies handle_IRQ_event to only enable interrupts when it 
hits the first handler with SA_INTERRUPT not set.

the patch is against 2.5.62 and greatly improves stability on my SMP system.

james


[-- Attachment #2: SA_INTERRUPT_fix.diff --]
[-- Type: text/plain, Size: 1870 bytes --]

--- linux/arch/i386/kernel/irq.c	Wed Feb 12 21:36:34 2003
+++ linux.test/arch/i386/kernel/irq.c	Mon Feb 24 21:01:04 2003
@@ -201,11 +201,13 @@
 int handle_IRQ_event(unsigned int irq, struct pt_regs * regs, struct irqaction * action)
 {
 	int status = 1;	/* Force the "do bottom halves" bit */
-
-	if (!(action->flags & SA_INTERRUPT))
-		local_irq_enable();
+	int enabled = SA_INTERRUPT;
 
 	do {
+		if (~action->flags & enabled) {
+			local_irq_enable();
+			enabled = 0;
+		}
 		status |= action->flags;
 		action->handler(irq, action->dev_id, regs);
 		action = action->next;
@@ -761,23 +763,38 @@
 	 * The following block of code has to be executed atomically
 	 */
 	spin_lock_irqsave(&desc->lock,flags);
-	p = &desc->action;
-	if ((old = *p) != NULL) {
-		/* Can't share interrupts unless both agree to */
-		if (!(old->flags & new->flags & SA_SHIRQ)) {
-			spin_unlock_irqrestore(&desc->lock,flags);
-			return -EBUSY;
+	if (new->flags & SA_INTERRUPT) {
+		p = &desc->action;
+		old = *p;
+		/* Add SA_INTERRUPT interrupts to the front of the queue
+		 * 'cos it's faster than adding them in the middle */
+		if (old) {
+			if (!(old->flags & new->flags & SA_SHIRQ)) {
+				spin_unlock_irqrestore(&desc->lock,flags);
+				return -EBUSY;
+			}
+			new->next = old;
+			shared = 1;
 		}
+		*p = new;
+	} else {
+		p = &desc->action;
+		if ((old = *p) != NULL) {
+			/* Can't share interrupts unless both agree to */
+			if (!(old->flags & new->flags & SA_SHIRQ)) {
+				spin_unlock_irqrestore(&desc->lock,flags);
+				return -EBUSY;
+			}
 
-		/* add new interrupt at end of irq queue */
-		do {
-			p = &old->next;
-			old = *p;
-		} while (old);
-		shared = 1;
+			/* add new interrupt at end of irq queue */
+			do {
+				p = &old->next;
+				old = *p;
+			} while (old);
+			shared = 1;
+		}
+		*p = new;
 	}
-
-	*p = new;
 
 	if (!shared) {
 		desc->depth = 0;

Re: PATCH to fix irq sharing and SA_INTERRUPT on x86. please review

[Andrew Morton]

James Harper <james.harper@bigpond.com> wrote:
>
> further to my email yesterday (to which i've had no response :) i 
> propose the attached patch to arch/i386/kernel/irq.c. it corrects what i 
> see as a bug in interrupt handling.
> 
> currently if a driver requests SA_INTERRUPT in an interrupt handler, it 
> is only called with interrupts disabled if it is the first handler in 
> the list.

ewww..

> my patch modifies setup_irq to put any interrupt with SA_INTERRUPT in 
> the front of the handler queue (eg before any handlers without the flag).
> 
> and also modifies handle_IRQ_event to only enable interrupts when it 
> hits the first handler with SA_INTERRUPT not set.
> 

Yes, that's a nice fix, thanks.

Other architectures appear to have inherited this bug.