[PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll

[PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll

[Edward Donovan]

Hi Thomas -

I did isolate another regression in the bad-irq handling, and this
patch sets things right on my machines.  After bisecting it, I've just
been troubleshooting to find which code was making the difference.
This is my attempt to restore it, in a reasonable place.

I don't know if this is the way you'd like to do it in the reworked
IRQ code, since I'm not very familiar with the system yet.  This is
the most C I've dealt with, and partly I'm just moving puzzle pieces
around.  I have tested it a good bit, though, and functionally it has
been fine.

So, let me know if you have a preferred patch, and I'll test it on
these machines.  Thanks!

Edward

[PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll

[Edward Donovan]

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.

Signed-off-by: Edward Donovan <edward.donovan@numble.net>
---
 kernel/irq/spurious.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index b5f4742..dc813a9 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -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

Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)

[Rogério Brito]

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:

> --- Comment #3 from Rogério Brito <rbrito@ime.usp.br>  2011-08-16 05:54:47 ---
> Add note that this bug was first reported on 2011-07-04
>
>    https://lkml.org/lkml/2011/7/24/54
>
> As stated on that e-mail, I am willing to perform any tests to help fix this
> bug. Just ask me and I will do my best to give you copious amounts of data.


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

Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)

[Linus Torvalds]

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 <rbrito@ime.usp.br>:
> Hi there.
>
> Now that, at least part of kernel.org is up, I would appreciate if
> 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:
>
>> --- Comment #3 from Rogério Brito <rbrito@ime.usp.br>  2011-08-16 05:54:47 ---
>> Add note that this bug was first reported on 2011-07-04
>>
>>    https://lkml.org/lkml/2011/7/24/54
>>
>> As stated on that e-mail, I am willing to perform any tests to help fix this
>> bug. Just ask me and I will do my best to give you copious amounts of data.
>
>
> 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
>

Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)

[Edward Donovan]

Hi all -

On Sun, Nov 27, 2011 at 12:22:54PM -0800, Linus Torvalds wrote:
> 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


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

[PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll

[Edward Donovan]

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.

Signed-off-by: Edward Donovan <edward.donovan@numble.net>
---
 kernel/irq/spurious.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index b5f4742..dc813a9 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -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 */

Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)

[Rogério Brito]

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.

On Mon, Nov 28, 2011 at 01:46, Edward Donovan <edward.donovan@numble.net> wrote:
> On Sun, Nov 27, 2011 at 12:22:54PM -0800, Linus Torvalds wrote:
>> 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.
(...)
>
> I experienced the regression in fa2727, too, and recently submitted a
> patch; I believe Thomas has it queued for review.  I'll repost here.

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.

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

I have some other bad-irq boxes that I can test on [*], but, so far,
you can add a "Tested-by: Rogério Brito <rbrito@ime.usp.br>" 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

Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)

[Thomas Gleixner]

On Sun, 27 Nov 2011, Linus Torvalds wrote:

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

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

------>
>From edward.donovan@numble.net Sun Nov 20 02:57:17 2011
Date: Sat, 19 Nov 2011 20:54:23 -0500
From: Edward Donovan <edward.donovan@numble.net>
To: tglx@linutronix.de
Cc: linux-kernel@vger.kernel.org, maciej.rutecki@gmail.com
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.

Signed-off-by: Edward Donovan <edward.donovan@numble.net>
---
 kernel/irq/spurious.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index b5f4742..dc813a9 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -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