Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)

Hi there.

Now that, at least part of kernel.org is up, I would appreciate it more people could help with debugging something that I reported to bugzilla in july of this year, a regression (bisected) that led to a bad commit of Thomas in the 2.6.39 cycle as the first bad one.

Bugzilla has more information on the issue, but as it seems to be down, I can, of course, summarize what I found. And, unfortunately, the problem still occurs with a recent pull of Linus tree up to v3.2-rc2-43-gaa1b052.

Below is the link to my first message that was archived:

Thanks,
Rogério Brito.

Rogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA
http://rb.doesntexist.org : Packages for LaTeX : algorithms.berlios.de
DebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br

Thomas, Ingo?

I haven't seen any response to this one, and while clearly commit fa27271bc8d2 ("genirq: Fixup poll handling") was *supposed* to be a no-op, it isn't.

The commit message says "Shorter version with the same functionality.", but since it causes a regression, it clearly is not with the same functionality at all. And I assume that Thomas doesn't have a machine that actually ever triggers the spurious irq issue to begin with, so it probably was never tested.

In short, it really sounds like this should just be reverted, since the code clearly doesn't do what the commit message claims it does.

Comments?

Linus

2011/11/22 Rogério Brito <[email protected]>:

Hi all -

I experienced the regression in fa2727, too, and recently submitted a patch; I believe Thomas has it queued for review. I'll repost here. The commit won't need to be fully reverted--it wouldn't be a simple reversion, amidst the rest of the 2.6.39 irq overhaul, and Thomas' rewrite is indeed better organized, I think.

I isolated the regression to the new version of 'try_one_irq' not testing for:

(action->handler(irq, action->dev_id) == IRQ_HANDLED)

before trying to deal with the interrupt, as the old did. My patch puts it with the other tests in the restructured code.

Happy to revise, or test suggestions against my bad-irq boxes.

Thanks -

Edward

Hi, Linus, and Edward.

First of all, thank you very much for being so gentle to take the time to read my report. For some months now I tried to reach some people in this CC list, but got no response.

(...)

Thanks. I just compiled Linus's tree with your patch and I'm typing this message running with such kernel. From my very basic testing, it seems that everything is working as well as it did in 2.6.38.

I have some other bad-irq boxes that I can test on [*], but, so far, you can add a "Tested-by: Rogério Brito <[email protected]>" to your patch (and, if you like, a Reported-by too).

Thanks a lot,
Rogério Brito.

[*] For instance, my mother's laptop, as reported on
https://lkml.org/lkml/2011/8/19/428

Rogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA
http://rb.doesntexist.org : Packages for LaTeX : algorithms.berlios.de
DebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br

commit fa27271bc8d2("genirq: Fixup poll handling") introduced a regression that broke irqfixup/irqpoll for some hardware configurations. Amidst reorganizing 'try_one_irq', that patch removed a test that checked for 'action->handler' returning IRQ_HANDLED, before acting on the interrupt. Restoring this test back returns the functionality lost since 2.6.39. In the current set of tests, after 'action' is set, it must precede '!action->next' to take effect.

With this and my previous patch to irq/spurious.c, c75d720fca8a, all IRQ regressions that I have encountered are fixed.

---
 kernel/irq/spurious.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

index b5f4742..dc813a9 100644
@@ -84,7 +84,9 @@ static int try_one_irq(int irq, struct irq_desc *desc, bool force)
 	 */
 	action = desc->action;
 	if (!action || !(action->flags & IRQF_SHARED) ||
-	    (action->flags & __IRQF_TIMER) || !action->next)
+	    (action->flags & __IRQF_TIMER) ||
+	    (action->handler(irq, action->dev_id) == IRQ_HANDLED) ||
+	    !action->next)
 		goto out;
 
 	/* Already running on another processor */

Yes, Edward tracked it down already. One part of the fix is already in your tree commit 52553ddffad. The second half is in my queue.

See below. Thanks,

tglx

------>

Date: Sat, 19 Nov 2011 20:54:23 -0500
From: Edward Donovan <[email protected]>
To: [email protected]
Cc: [email protected], [email protected]
Subject: [PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll

commit fa2727("genirq: Fixup poll handling") introduced a regression that broke irqfixup/irqpoll for some hardware configurations. That patch removed a test that checked for 'action->handler' returning IRQ_HANDLED, before acting on the IRQ. Putting this test back restores the functionality lost since 2.6.39. In the current set of tests after 'action' is set, it must precede '!action->next' to take effect.

With this and the previous patch to spurious.c, c75d720fca8a, all IRQ regressions that I have encountered are fixed.

---
 kernel/irq/spurious.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

index b5f4742..dc813a9 100644
@@ -84,7 +84,9 @@ static int try_one_irq(int irq, struct irq_desc *desc, bool force)
 	 */
 	action = desc->action;
 	if (!action || !(action->flags & IRQF_SHARED) ||
-	    (action->flags & __IRQF_TIMER) || !action->next)
+	    (action->flags & __IRQF_TIMER) ||
+	    (action->handler(irq, action->dev_id) == IRQ_HANDLED) ||
+	    !action->next)
 		goto out;
 
 	/* Already running on another processor */

1.7.5.4