On Thu, May 03, 2007 at 12:19:32AM +0200, Bernhard Walle wrote:
> * Vivek Goyal <[email protected]> [2007-04-30 10:48]:
> >
> > handle_edge_irq() already makes sure that desc->action is not null, still
> > note_interrupt() is receiving desc->action as null, that's strange. On my
> > system this is happening for irq 4 and /proc/interrupt shows that it is
> > coming from "serial".
>
> Unfortunately, I couldn't reproduce this here. Vivek, do you have time
> to take a look at this at your site? For the meanwhile, should I
> create a patch that checks for desc->action in note_interrupt(), too?
>
Hi Bernhard,
I can reproduce this problem only on one machine. I think there is some
race condition and your code somehow just exposes it.
I put few WARN_ON(!desc->action) in handle_edge_irq() and what I find
that after handle_IRQ_event(), desc->action has become null. That means
in the meantime somebody has gone ahead and modified the desc. This must
have happened because we have release desc->lock while running
handle_IRQ_event().
This means there is a race somewhere. It is verified by the fact that
this problem does not occur if same system is booted with only one
cpu (maxcpus=1).
Thanks
Vivek
* Vivek Goyal <[email protected]> [2007-05-08 19:18]:
> On Thu, May 03, 2007 at 12:19:32AM +0200, Bernhard Walle wrote:
> > * Vivek Goyal <[email protected]> [2007-04-30 10:48]:
> > >
> > > handle_edge_irq() already makes sure that desc->action is not null, still
> > > note_interrupt() is receiving desc->action as null, that's strange. On my
> > > system this is happening for irq 4 and /proc/interrupt shows that it is
> > > coming from "serial".
> >
> > Unfortunately, I couldn't reproduce this here. Vivek, do you have time
> > to take a look at this at your site? For the meanwhile, should I
> > create a patch that checks for desc->action in note_interrupt(), too?
>
> I can reproduce this problem only on one machine. I think there is some
> race condition and your code somehow just exposes it.
thanks for finding that out. Could you try/review out the patch below?
As the lock is only aquired when irqfixup == 2 it shouldn't impact
performance of a 'normal' system.
Thanks,
Bernhard
---
kernel/irq/spurious.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -145,10 +145,20 @@ void note_interrupt(unsigned int irq, st
}
if (unlikely(irqfixup)) {
- /* Don't punish working computers */
- if ((irqfixup == 2 && ((irq == 0) ||
- (desc->action->flags & IRQF_IRQPOLL))) ||
- action_ret == IRQ_NONE) {
+ int call_misrouted_irq = action_ret == IRQ_NONE;
+
+ if (!call_misrouted_irq && irqfixup == 2) {
+ if (irq == 0)
+ call_misrouted_irq = 1;
+ else {
+ spin_lock(&desc->lock);
+ if (desc->action && (desc->action->flags & IRQF_IRQPOLL))
+ call_misrouted_irq = 1;
+ spin_unlock(&desc->lock);
+ }
+ }
+
+ if (call_misrouted_irq) {
int ok = misrouted_irq(irq);
if (action_ret == IRQ_NONE)
desc->irqs_unhandled -= ok;
On Mon, May 14, 2007 at 04:05:15PM +0200, Bernhard Walle wrote:
> * Vivek Goyal <[email protected]> [2007-05-08 19:18]:
> > On Thu, May 03, 2007 at 12:19:32AM +0200, Bernhard Walle wrote:
> > > * Vivek Goyal <[email protected]> [2007-04-30 10:48]:
> > > >
> > > > handle_edge_irq() already makes sure that desc->action is not null, still
> > > > note_interrupt() is receiving desc->action as null, that's strange. On my
> > > > system this is happening for irq 4 and /proc/interrupt shows that it is
> > > > coming from "serial".
> > >
> > > Unfortunately, I couldn't reproduce this here. Vivek, do you have time
> > > to take a look at this at your site? For the meanwhile, should I
> > > create a patch that checks for desc->action in note_interrupt(), too?
> >
> > I can reproduce this problem only on one machine. I think there is some
> > race condition and your code somehow just exposes it.
>
> thanks for finding that out. Could you try/review out the patch below?
> As the lock is only aquired when irqfixup == 2 it shouldn't impact
> performance of a 'normal' system.
>
Hi Bernhard,
It does fix up my problem. I have modified your patch a bit. I think
new version is little more clear. What do you think?
Thanks
Vivek
o System crashes if booted with irqpoll command line option.
o Problem happens because Inside note_interrupt() we are accessing
desc->action->flag without taking the desc->lock. While accessing it
somebody goes ahead and unregisters the irq handler hence desc->action
is NULL. By the time note_interrupt() checks it, it crashes.
o In my system it is irq 4 seriving to serial driver.
o Take the desc->lock before accessing desc->action->flag.
Signed-off-by: Bernhard Walle <[email protected]>
Signed-off-by: Vivek Goyal <[email protected]>
---
linux-2.6.21-git12-root/kernel/irq/spurious.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff -puN kernel/irq/spurious.c~fix-irqpoll-crash kernel/irq/spurious.c
--- linux-2.6.21-git12/kernel/irq/spurious.c~fix-irqpoll-crash 2007-05-17 17:36:50.000000000 +0530
+++ linux-2.6.21-git12-root/kernel/irq/spurious.c 2007-05-17 17:53:52.000000000 +0530
@@ -138,6 +138,8 @@ report_bad_irq(unsigned int irq, struct
void note_interrupt(unsigned int irq, struct irq_desc *desc,
irqreturn_t action_ret)
{
+ int call_misrouted_irq = 0;
+
if (unlikely(action_ret != IRQ_HANDLED)) {
desc->irqs_unhandled++;
if (unlikely(action_ret != IRQ_NONE))
@@ -146,9 +148,24 @@ void note_interrupt(unsigned int irq, st
if (unlikely(irqfixup)) {
/* Don't punish working computers */
- if ((irqfixup == 2 && ((irq == 0) ||
- (desc->action->flags & IRQF_IRQPOLL))) ||
- action_ret == IRQ_NONE) {
+ if (action_ret == IRQ_NONE)
+ /* Nobody handled irq. Possibly a misrouted one. */
+ call_misrouted_irq = 1;
+ else if (irqfixup == 2) {
+ /* irqpoll is enabled. Is this the irq driving
+ * polling.
+ */
+ if (irq == 0)
+ call_misrouted_irq = 1;
+ else {
+ spin_lock(&desc->lock);
+ if (desc->action &&
+ (desc->action->flags & IRQF_IRQPOLL))
+ call_misrouted_irq = 1;
+ spin_unlock(&desc->lock);
+ }
+ }
+ if (call_misrouted_irq) {
int ok = misrouted_irq(irq);
if (action_ret == IRQ_NONE)
desc->irqs_unhandled -= ok;
_
* Vivek Goyal <[email protected]> [2007-05-17 15:05]:
> On Mon, May 14, 2007 at 04:05:15PM +0200, Bernhard Walle wrote:
> > * Vivek Goyal <[email protected]> [2007-05-08 19:18]:
> > > On Thu, May 03, 2007 at 12:19:32AM +0200, Bernhard Walle wrote:
> > > > * Vivek Goyal <[email protected]> [2007-04-30 10:48]:
> > > > >
> > > > > handle_edge_irq() already makes sure that desc->action is not null, still
> > > > > note_interrupt() is receiving desc->action as null, that's strange. On my
> > > > > system this is happening for irq 4 and /proc/interrupt shows that it is
> > > > > coming from "serial".
> > > >
> > > > Unfortunately, I couldn't reproduce this here. Vivek, do you have time
> > > > to take a look at this at your site? For the meanwhile, should I
> > > > create a patch that checks for desc->action in note_interrupt(), too?
> > >
> > > I can reproduce this problem only on one machine. I think there is some
> > > race condition and your code somehow just exposes it.
> >
> > thanks for finding that out. Could you try/review out the patch below?
> > As the lock is only aquired when irqfixup == 2 it shouldn't impact
> > performance of a 'normal' system.
>
> It does fix up my problem. I have modified your patch a bit. I think
> new version is little more clear. What do you think?
Aggreed. Thanks for spotting that problem out!
Bernhard
o System crashes if booted with irqpoll command line option.
o Problem happens because Inside note_interrupt() we are accessing
desc->action->flag without taking the desc->lock. While accessing it
somebody goes ahead and unregisters the irq handler hence desc->action
is NULL. By the time note_interrupt() checks it, it crashes.
o In that system it is irq 4 seriving to serial driver.
o Take the desc->lock before accessing desc->action->flag.
Signed-off-by: Bernhard Walle <[email protected]>
Signed-off-by: Vivek Goyal <[email protected]>
---
linux-2.6.21-git12-root/kernel/irq/spurious.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff -puN kernel/irq/spurious.c~fix-irqpoll-crash kernel/irq/spurious.c
--- linux-2.6.21-git12/kernel/irq/spurious.c~fix-irqpoll-crash 2007-05-17 17:36:50.000000000 +0530
+++ linux-2.6.21-git12-root/kernel/irq/spurious.c 2007-05-17 17:53:52.000000000 +0530
@@ -138,6 +138,8 @@ report_bad_irq(unsigned int irq, struct
void note_interrupt(unsigned int irq, struct irq_desc *desc,
irqreturn_t action_ret)
{
+ int call_misrouted_irq = 0;
+
if (unlikely(action_ret != IRQ_HANDLED)) {
desc->irqs_unhandled++;
if (unlikely(action_ret != IRQ_NONE))
@@ -146,9 +148,24 @@ void note_interrupt(unsigned int irq, st
if (unlikely(irqfixup)) {
/* Don't punish working computers */
- if ((irqfixup == 2 && ((irq == 0) ||
- (desc->action->flags & IRQF_IRQPOLL))) ||
- action_ret == IRQ_NONE) {
+ if (action_ret == IRQ_NONE)
+ /* Nobody handled irq. Possibly a misrouted one. */
+ call_misrouted_irq = 1;
+ else if (irqfixup == 2) {
+ /* irqpoll is enabled. Is this the irq driving
+ * polling.
+ */
+ if (irq == 0)
+ call_misrouted_irq = 1;
+ else {
+ spin_lock(&desc->lock);
+ if (desc->action &&
+ (desc->action->flags & IRQF_IRQPOLL))
+ call_misrouted_irq = 1;
+ spin_unlock(&desc->lock);
+ }
+ }
+ if (call_misrouted_irq) {
int ok = misrouted_irq(irq);
if (action_ret == IRQ_NONE)
desc->irqs_unhandled -= ok;
_
On Tue, 22 May 2007, Bernhard Walle wrote:
>
> o System crashes if booted with irqpoll command line option.
>
> o Problem happens because Inside note_interrupt() we are accessing
> desc->action->flag without taking the desc->lock. While accessing it
> somebody goes ahead and unregisters the irq handler hence desc->action
> is NULL. By the time note_interrupt() checks it, it crashes.
I absolutely _detest_ patches that make already complex and unreadable
code even more so. Especially conditionals. Please don't do that.
If you need a variable for a conditional, make it be an implicit one from
an inline function, and aim for making it readable.
So how about instead writing it out as a nice self-explanatory inline
function? I can almost guarantee that this generates no worse code, and it
also makes it easy to explain things like "we don't bother with the lock,
because we don't care enough".
Untested, but I think the point of the patch is obvious. Anybody want to
test it, send it back to me, and fix the bug while making the code more
readable?
I think we should instate a hard new rule:
- if a bugfix doesn't make the code more readable, it's not a bugfix at
all.
There was a reason for the bug in the first place, and that reason is
likely that the code was hard to think about. So making it even _harder_
to think about isn't really fixing the deeper problem!
Linus
---
kernel/irq/spurious.c | 46 +++++++++++++++++++++++++++++++++++++---------
1 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index b0d81aa..bd9e272 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -135,6 +135,39 @@ report_bad_irq(unsigned int irq, struct irq_desc *desc, irqreturn_t action_ret)
}
}
+static inline int try_misrouted_irq(unsigned int irq, struct irq_desc *desc, irqreturn_t action_ret)
+{
+ struct irqaction *action;
+
+ if (!irqfixup)
+ return 0;
+
+ /* We didn't actually handle the IRQ - see if it was misrouted? */
+ if (action_ret == IRQ_NONE)
+ return 1;
+
+ /*
+ * But for 'irqfixup == 2' we also do it for handled interrupts if
+ * they are marked as IRQF_IRQPOLL (or for irq zero, which is the
+ * traditional PC timer interrupt.. Legacy)
+ */
+ if (irqfixup < 2)
+ return 0;
+
+ if (!irq)
+ return 1;
+
+ /*
+ * Since we don't get the descriptor lock, "action" can
+ * change under us. We don't really care, but we don't
+ * want to follow a NULL pointer. So tell the compiler to
+ * just load it once by using a barrier.
+ */
+ action = desc->action;
+ barrier();
+ return action && (action->flags & IRQF_IRQPOLL);
+}
+
void note_interrupt(unsigned int irq, struct irq_desc *desc,
irqreturn_t action_ret)
{
@@ -144,15 +177,10 @@ void note_interrupt(unsigned int irq, struct irq_desc *desc,
report_bad_irq(irq, desc, action_ret);
}
- if (unlikely(irqfixup)) {
- /* Don't punish working computers */
- if ((irqfixup == 2 && ((irq == 0) ||
- (desc->action->flags & IRQF_IRQPOLL))) ||
- action_ret == IRQ_NONE) {
- int ok = misrouted_irq(irq);
- if (action_ret == IRQ_NONE)
- desc->irqs_unhandled -= ok;
- }
+ if (unlikely(try_misrouted_irq(irq, desc, action_ret))) {
+ int ok = misrouted_irq(irq);
+ if (action_ret == IRQ_NONE)
+ desc->irqs_unhandled -= ok;
}
desc->irq_count++;
On Wed, May 23, 2007 at 09:01:04AM -0700, Linus Torvalds wrote:
>
>
> On Tue, 22 May 2007, Bernhard Walle wrote:
> >
> > o System crashes if booted with irqpoll command line option.
> >
> > o Problem happens because Inside note_interrupt() we are accessing
> > desc->action->flag without taking the desc->lock. While accessing it
> > somebody goes ahead and unregisters the irq handler hence desc->action
> > is NULL. By the time note_interrupt() checks it, it crashes.
>
> I absolutely _detest_ patches that make already complex and unreadable
> code even more so. Especially conditionals. Please don't do that.
>
> If you need a variable for a conditional, make it be an implicit one from
> an inline function, and aim for making it readable.
>
> So how about instead writing it out as a nice self-explanatory inline
> function? I can almost guarantee that this generates no worse code, and it
> also makes it easy to explain things like "we don't bother with the lock,
> because we don't care enough".
>
> Untested, but I think the point of the patch is obvious. Anybody want to
> test it, send it back to me, and fix the bug while making the code more
> readable?
>
Hi Linus,
I tested it. It works fine. And yes, this patch is more readable.
Thanks
Vivek