* Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)
2011-11-27 20:22 ` Linus Torvalds
@ 2011-11-28 3:46 ` Edward Donovan
2011-11-28 5:08 ` Rogério Brito
2011-11-28 4:07 ` [PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll Edward Donovan
2011-11-30 9:42 ` [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version) Thomas Gleixner
2 siblings, 1 reply; 6+ messages in thread
From: Edward Donovan @ 2011-11-28 3:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, Ingo Molnar, Dave Airlie, linux-kernel,
drivers_pci, Rogério Brito, Maciej Rutecki
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)
2011-11-28 3:46 ` Edward Donovan
@ 2011-11-28 5:08 ` Rogério Brito
0 siblings, 0 replies; 6+ messages in thread
From: Rogério Brito @ 2011-11-28 5:08 UTC (permalink / raw)
To: Edward Donovan
Cc: Linus Torvalds, Thomas Gleixner, Ingo Molnar, Dave Airlie,
linux-kernel, drivers_pci, Maciej Rutecki
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll
2011-11-27 20:22 ` Linus Torvalds
2011-11-28 3:46 ` Edward Donovan
@ 2011-11-28 4:07 ` Edward Donovan
2011-11-30 9:42 ` [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version) Thomas Gleixner
2 siblings, 0 replies; 6+ messages in thread
From: Edward Donovan @ 2011-11-28 4:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, Ingo Molnar, Dave Airlie, linux-kernel,
drivers_pci, Rogério Brito, Maciej Rutecki,
Rafael J. Wysocki
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 */
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Bug 41132] [BISECTED][REGRESSION] Regression with the IRQ subsystem introduced in 2.6.39 (and present in the 3.x version)
2011-11-27 20:22 ` Linus Torvalds
2011-11-28 3:46 ` Edward Donovan
2011-11-28 4:07 ` [PATCH] genirq: fix second 2.6.39 regression in irqfixup, irqpoll Edward Donovan
@ 2011-11-30 9:42 ` Thomas Gleixner
2 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2011-11-30 9:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Dave Airlie, linux-kernel, drivers_pci,
Rogério Brito
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
^ permalink raw reply related [flat|nested] 6+ messages in thread