Messages in this thread Patch in this message | | | Date | Tue, 3 Nov 2009 22:58:54 +0800 | Subject | Re: [PATCH]: use spin_lock_irqsave in try_one_irq() | From | Yong Zhang <> |
| |
> This happens because the &desc->lock is taken with spin_lock_irqsave and > just a spin_lock. In the try_one_irq(), this lock really should be a > spin_lock_irqsave(). >
Cc'ed Ingo and Thomas.
The reason is that try_one_irq() is called both from hardirq context and softirq context. And by default the timer handler poll_all_shared_irqs() is called with irq enabled. Then the two usage will cause inconsistent.
So I think the following patch is also workable to you.
diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c index 114e704..11affbc 100644 --- a/kernel/irq/spurious.c +++ b/kernel/irq/spurious.c @@ -111,6 +111,7 @@ static void poll_all_shared_irqs(void)
for_each_irq_desc(i, desc) { unsigned int status; + unsigned long flags;
if (!i) continue; @@ -121,7 +122,9 @@ static void poll_all_shared_irqs(void) if (!(status & IRQ_SPURIOUS_DISABLED)) continue;
+ local_irq_save(flags); try_one_irq(i, desc); + local_irq_restore(flags); } }
> I have not yet narrowed down the reason for the spurious interrupt (although > I suspect it maybe to do with the radeon driver). > Successfully tested by me. > > Signed-off-by: Prarit Bhargava <prarit@redhat.com> > > --- linux-2.6.31.x86_64.orig/kernel/irq/spurious.c 2009-09-09 18:13:59.000000000 -0400 > +++ linux-2.6.31.x86_64/kernel/irq/spurious.c 2009-10-26 10:55:56.709845786 -0400 > @@ -27,8 +27,9 @@ static int try_one_irq(int irq, struct i > { > struct irqaction *action; > int ok = 0, work = 0; > + unsigned long flags; > > - spin_lock(&desc->lock); > + spin_lock_irqsave(&desc->lock, flags); > /* Already running on another processor */ > if (desc->status & IRQ_INPROGRESS) { > /* > @@ -37,13 +38,13 @@ static int try_one_irq(int irq, struct i > */ > if (desc->action && (desc->action->flags & IRQF_SHARED)) > desc->status |= IRQ_PENDING; > - spin_unlock(&desc->lock); > + spin_unlock_irqrestore(&desc->lock, flags); > return ok; > } > /* Honour the normal IRQ locking */ > desc->status |= IRQ_INPROGRESS; > action = desc->action; > - spin_unlock(&desc->lock); > + spin_unlock_irqrestore(&desc->lock, flags); > > while (action) { > /* Only shared IRQ handlers are safe to call */ > @@ -56,7 +57,7 @@ static int try_one_irq(int irq, struct i > } > local_irq_disable(); > /* Now clean up the flags */ > - spin_lock(&desc->lock); > + spin_lock_irqsave(&desc->lock, flags); > action = desc->action; > > /* > @@ -68,9 +69,9 @@ static int try_one_irq(int irq, struct i > * Perform real IRQ processing for the IRQ we deferred > */ > work = 1; > - spin_unlock(&desc->lock); > + spin_unlock_irqrestore(&desc->lock, flags); > handle_IRQ_event(irq, action); > - spin_lock(&desc->lock); > + spin_lock_irqsave(&desc->lock, flags); > desc->status &= ~IRQ_PENDING; > } > desc->status &= ~IRQ_INPROGRESS; > @@ -80,7 +81,7 @@ static int try_one_irq(int irq, struct i > */ > if (work && desc->chip && desc->chip->end) > desc->chip->end(irq); > - spin_unlock(&desc->lock); > + spin_unlock_irqrestore(&desc->lock, flags); > > return ok; > } > -- > 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/ > -- 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/
| |