[RFC][PATCH -mm] postpone misrouted irqs when disabled

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 2006-06-04 at 23:44 +0200, Ingo Molnar wrote:

> pretty much the only correct solution seems to be to go with Arjan's 
> suggestion and make the 'disabled' property per-action, instead of the 
> current per-desc thing. (obviously the physical act of masking an 
> interrupt line is fundamentally per-desc, but the act of running an 
> action "behind the back" of a masked line is still OK.) Unfortunately 
> this would also mean the manual conversion of 300+ places that use 
> disable_irq()/enable_irq() currently ... so it's no small work. (and the 
> hardest part of the work is to find a safe method to convert them 
> without introducing bugs)

OK, I got some time to play.

This is what I was talking about before.  Here I honour the IRQ_DISABLED
in misrouted_irq, but I have some accounting that will not enable the
irq on exit of the interrupt handler if the misrouted irq encountered a
disabled interrupt.  What happens is that the IRQ stays masked until who
ever disabled the irq enables it.

On enable_irq a check is made and if a misrouted irq was done while the
irq was disabled, it then calls the interrupt handler, and checks to see
if it should unmask the irq in question.

Disclaimer: I did this patch in between compiles of doing real work, so
it is really a big hack.  But it helps to explain what I'm getting at.
BTW, my machine with the vortex card deadlocks without this patch
(happens in the disabled_irq with spin_lock-irqs-enabled section).  But
with this patch it actual runs, and I put in my internal logging to see
if the disable_irq + misrouted_irq that deadlocks normal happens.  It
did happen and the machine kept on going :)

And Ingo, this was inspired by the way -rt does hard irq threading.
That is to keep the irq unmasked until the thread takes care of it.
That's all, I wasn't suggesting that we add irq threads here.

Another positive with this approach: you don't need to update 300+ calls
to disable_irq and enable_irq and make sure they are right.

This patch goes against -mm3 + the patch I sent earlier to fix the
desc->chip->end() bug.

-- Steve

(Signing off, but this isn't a real patch - needs more work)

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


Index: linux-2.6.17-rc5-mm3/include/linux/irq.h
===================================================================
--- linux-2.6.17-rc5-mm3.orig/include/linux/irq.h	2006-06-05 17:26:15.000000000 -0400
+++ linux-2.6.17-rc5-mm3/include/linux/irq.h	2006-06-05 23:08:07.000000000 -0400
@@ -165,6 +165,8 @@ struct irq_desc {
 
 extern struct irq_desc irq_desc[NR_IRQS];
 
+extern void misroute_enable_irq(unsigned int irq, struct irq_desc *desc);
+
 /*
  * Migration helpers for obsolete names, they will go away:
  */
@@ -332,8 +334,8 @@ static inline void generic_handle_irq(un
 }
 
 /* Handling of unhandled and spurious interrupts: */
-extern void note_interrupt(unsigned int irq, struct irq_desc *desc,
-			   int action_ret, struct pt_regs *regs);
+extern int note_interrupt(unsigned int irq, struct irq_desc *desc,
+			  int action_ret, struct pt_regs *regs);
 
 /* Resending of interrupts :*/
 void check_irq_resend(struct irq_desc *desc, unsigned int irq);
Index: linux-2.6.17-rc5-mm3/kernel/irq/handle.c
===================================================================
--- linux-2.6.17-rc5-mm3.orig/kernel/irq/handle.c	2006-06-05 22:34:55.000000000 -0400
+++ linux-2.6.17-rc5-mm3/kernel/irq/handle.c	2006-06-05 23:07:33.000000000 -0400
@@ -153,6 +153,7 @@ fastcall unsigned int __do_IRQ(unsigned 
 	struct irq_desc *desc = irq_desc + irq;
 	struct irqaction *action;
 	unsigned int status;
+	int misrouted_pending = 0;
 
 	spin_lock(&sdr_lock);
 	if (!irq) {
@@ -237,19 +238,23 @@ fastcall unsigned int __do_IRQ(unsigned 
 
 		spin_lock(&desc->lock);
 		if (!noirqdebug)
-			note_interrupt(irq, desc, action_ret, regs);
+			misrouted_pending = note_interrupt(irq, desc, action_ret, regs);
 		if (likely(!(desc->status & IRQ_PENDING)))
 			break;
+		if (unlikely(misrouted_pending))
+			break;
 		desc->status &= ~IRQ_PENDING;
 	}
 	desc->status &= ~IRQ_INPROGRESS;
 
 out:
-	/*
-	 * The ->end() handler has to deal with interrupts which got
-	 * disabled while the handler was running.
-	 */
-	desc->chip->end(irq);
+	if (likely(!misrouted_pending)) {
+		/*
+		 * The ->end() handler has to deal with interrupts which got
+		 * disabled while the handler was running.
+		 */
+		desc->chip->end(irq);
+	}
 	spin_unlock(&desc->lock);
 
 	return 1;
Index: linux-2.6.17-rc5-mm3/kernel/irq/manage.c
===================================================================
--- linux-2.6.17-rc5-mm3.orig/kernel/irq/manage.c	2006-06-05 17:26:15.000000000 -0400
+++ linux-2.6.17-rc5-mm3/kernel/irq/manage.c	2006-06-05 23:05:24.000000000 -0400
@@ -123,6 +123,16 @@ void enable_irq(unsigned int irq)
 
 		/* Prevent probing on this irq: */
 		desc->status = status | IRQ_NOPROBE;
+		/*
+		 * Here we check to see if we need to handle
+		 * a misrouted irq, and we were disabled when
+		 * it happened.
+		 * Really, the check_irq_resend could handle this
+		 * but that code is dependent on the chip having
+		 * a retrigger operation and the misrouted irq
+		 * can't depend on that.
+		 */
+		misroute_enable_irq(irq, desc);
 		check_irq_resend(desc, irq);
 		/* fall-through */
 	}
Index: linux-2.6.17-rc5-mm3/kernel/irq/spurious.c
===================================================================
--- linux-2.6.17-rc5-mm3.orig/kernel/irq/spurious.c	2006-06-05 19:53:27.000000000 -0400
+++ linux-2.6.17-rc5-mm3/kernel/irq/spurious.c	2006-06-05 23:29:42.000000000 -0400
@@ -10,15 +10,20 @@
 #include <linux/module.h>
 #include <linux/kallsyms.h>
 #include <linux/interrupt.h>
+#include <linux/bitops.h>
 
 static int irqfixup __read_mostly;
 
+DEFINE_SPINLOCK(misroute_lock);
+DECLARE_BITMAP(misroute_irq_disabled, NR_IRQS);
+DECLARE_BITMAP(misroute_irq_pending, NR_IRQS);
+
 /*
  * Recovery handler for misrouted interrupts.
  */
 static int misrouted_irq(int irq, struct pt_regs *regs)
 {
-	int i, ok = 0, work = 0;
+	int i, ok = 0, work = 0, disabled = 0;
 
 	for (i = 1; i < NR_IRQS; i++) {
 		struct irq_desc *desc = irq_desc + i;
@@ -39,8 +44,28 @@ static int misrouted_irq(int irq, struct
 			spin_unlock(&desc->lock);
 			continue;
 		}
+		/* Honour disable irq */
+		if (desc->status & IRQ_DISABLED) {
+			/*
+			 * Disabled: We set the pending bit to let the
+			 * enabled_irq know that it should check to
+			 * run again, and that we might not have unmasked
+			 * the interrupt yet.
+			 */
+			if (desc->action && (desc->action->flags & SA_SHIRQ)) {
+				desc->status |= IRQ_PENDING;
+				spin_lock(&misroute_lock);
+				__set_bit(i, misroute_irq_disabled);
+				spin_unlock(&misroute_lock);
+				disabled = 1;
+			}
+			spin_unlock(&desc->lock);
+			continue;
+		}
+
 		/* Honour the normal IRQ locking */
 		desc->status |= IRQ_INPROGRESS;
+
 		action = desc->action;
 		spin_unlock(&desc->lock);
 
@@ -133,9 +158,12 @@ report_bad_irq(unsigned int irq, struct 
 	}
 }
 
-void note_interrupt(unsigned int irq, struct irq_desc *desc,
-		    irqreturn_t action_ret, struct pt_regs *regs)
+int note_interrupt(unsigned int irq, struct irq_desc *desc,
+		   irqreturn_t action_ret, struct pt_regs *regs)
 {
+	int ok = -1;
+	int ret = 0;
+
 	if (unlikely(action_ret != IRQ_HANDLED)) {
 		desc->irqs_unhandled++;
 		if (unlikely(action_ret != IRQ_NONE))
@@ -145,15 +173,26 @@ void note_interrupt(unsigned int irq, st
 	if (unlikely(irqfixup)) {
 		/* Don't punish working computers */
 		if ((irqfixup == 2 && irq == 0) || action_ret == IRQ_NONE) {
-			int ok = misrouted_irq(irq, regs);
+			ok = misrouted_irq(irq, regs);
 			if (action_ret == IRQ_NONE)
 				desc->irqs_unhandled -= ok;
 		}
 	}
 
 	desc->irq_count++;
+	spin_lock(&misroute_lock);
+	/*
+	 * If we didn't handle an interrupt, and we have disabled
+	 * interrupts, then don't unmask this when we leave
+	 * the interrupt handler.
+	 */
+	if (!ok && find_first_bit(misroute_irq_disabled, NR_IRQS) != NR_IRQS) {
+		__set_bit(irq, misroute_irq_pending);
+		ret = 1;
+	}
+	spin_unlock(&misroute_lock);
 	if (likely(desc->irq_count < 100000))
-		return;
+		return ret;
 
 	desc->irq_count = 0;
 	if (unlikely(desc->irqs_unhandled > 99900)) {
@@ -170,6 +209,114 @@ void note_interrupt(unsigned int irq, st
 		desc->chip->disable(irq);
 	}
 	desc->irqs_unhandled = 0;
+	return ret;
+}
+
+/*
+ * When called, the desc->lock is held.
+ */
+void misroute_enable_irq(unsigned int irq, struct irq_desc *desc)
+{
+	struct irqaction *action;
+	int ok = 0;
+	int i;
+
+	/*
+	 * Perhaps a new flag would be good so that systems
+	 * without misroute irq enabled would skip this every time.
+	 */
+	if ((desc->status & IRQ_PENDING) == 0)
+		return;
+
+	spin_lock(&misroute_lock);
+	if (!test_and_clear_bit(irq, misroute_irq_disabled)) {
+		spin_unlock(&misroute_lock);
+		return;
+	}
+
+	spin_unlock(&misroute_lock);
+
+	/*
+	 * Run our handler.  This really needs to be wrapped up
+	 * in another function.
+	 */
+	desc->status |= IRQ_INPROGRESS;
+	desc->status &= ~IRQ_PENDING;
+
+	action = desc->action;
+	spin_unlock(&desc->lock);
+
+	while (action) {
+		/* Only shared IRQ handlers are safe to call */
+		if (action->flags & SA_SHIRQ) {
+			/*
+			 * Really no handler looks at regs.
+			 */
+			if (action->handler(irq, action->dev_id, NULL) ==
+			    IRQ_HANDLED)
+				ok = 1;
+		}
+		action = action->next;
+	}
+	/* Now clean up the flags */
+	spin_lock(&desc->lock);
+	action = desc->action;
+
+	/*
+	 * While we were looking for a fixup someone queued a real
+	 * IRQ clashing with our walk:
+	 */
+	while ((desc->status & IRQ_PENDING) && action) {
+		/*
+		 * Perform real IRQ processing for the IRQ we deferred
+		 */
+		ok = 1;
+		spin_unlock(&desc->lock);
+		handle_IRQ_event(irq, NULL, action);
+		spin_lock(&desc->lock);
+		desc->status &= ~IRQ_PENDING;
+	}
+	desc->status &= ~IRQ_INPROGRESS;
+	spin_unlock(&desc->lock);
+
+	spin_lock(&misroute_lock);
+	/*
+	 * If we did something or there are no more misrouted irqs then
+	 * unmask all the irqs that were waiting on us.
+	 */
+	while ((i = find_first_bit(misroute_irq_pending, NR_IRQS)) != NR_IRQS) {
+		struct irq_desc *d = irq_desc + i;
+		int m;
+
+		spin_unlock(&misroute_lock);
+
+		spin_lock(&d->lock);
+		spin_lock(&misroute_lock);
+		/*
+		 * Need to make sure that we suddenly didn't have another misroute.
+		 */
+		if ((m=find_first_bit(misroute_irq_disabled, NR_IRQS)) != NR_IRQS) {
+			spin_unlock(&d->lock);
+			break;
+		}
+
+		if (desc->chip && desc->chip->end)
+			desc->chip->end(i);
+		clear_bit(i, misroute_irq_pending);
+		spin_lock(&misroute_lock);
+		spin_unlock(&d->lock);
+	}
+	{
+		static int once;
+		if (!once) {
+			once = 1;
+			LOGDUMP();
+		}
+	}
+	spin_unlock(&misroute_lock);
+
+	/* leave as we came */
+	spin_lock(&desc->lock);
 }
 
 int noirqdebug __read_mostly;


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/