* [PATCH] forcedeth: hardirq lockdep warning
@ 2006-09-19 12:55 Peter Zijlstra
2006-09-19 18:14 ` Andrew Morton
2006-10-05 10:48 ` Jeff Garzik
0 siblings, 2 replies; 6+ messages in thread
From: Peter Zijlstra @ 2006-09-19 12:55 UTC (permalink / raw)
To: linux-kernel
Cc: Jeff Garzik, Ingo Molnar, Arjan van de Ven, Dave Jones,
Andrew Morton
BUG: warning at kernel/lockdep.c:1816/trace_hardirqs_on() (Not tainted)
Call Trace:
show_trace
dump_stack
trace_hardirqs_on
:forcedeth:nv_nic_irq_other
handle_IRQ_event
__do_IRQ
do_IRQ
ret_from_intr
DWARF2 barf
default_idle
cpu_idle
rest_init
start_kernel
_sinittext
These 3 functions nv_nic_irq_tx(), nv_nic_irq_rx() and nv_nic_irq_other()
are reachable from IRQ context and process context. Make use of the
irq-save/restore spinlock variant.
(Compile tested only, since I do not have the hardware)
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Andrew Morton <akpm@osdl.org>
---
drivers/net/forcedeth.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
Index: linux-2.6-mm/drivers/net/forcedeth.c
===================================================================
--- linux-2.6-mm.orig/drivers/net/forcedeth.c
+++ linux-2.6-mm/drivers/net/forcedeth.c
@@ -2497,6 +2497,7 @@ static irqreturn_t nv_nic_irq_tx(int foo
u8 __iomem *base = get_hwbase(dev);
u32 events;
int i;
+ unsigned long flags;
dprintk(KERN_DEBUG "%s: nv_nic_irq_tx\n", dev->name);
@@ -2508,16 +2509,16 @@ static irqreturn_t nv_nic_irq_tx(int foo
if (!(events & np->irqmask))
break;
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
nv_tx_done(dev);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
if (events & (NVREG_IRQ_TX_ERR)) {
dprintk(KERN_DEBUG "%s: received irq with events 0x%x. Probably TX fail.\n",
dev->name, events);
}
if (i > max_interrupt_work) {
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
/* disable interrupts on the nic */
writel(NVREG_IRQ_TX_ALL, base + NvRegIrqMask);
pci_push(base);
@@ -2527,7 +2528,7 @@ static irqreturn_t nv_nic_irq_tx(int foo
mod_timer(&np->nic_poll, jiffies + POLL_WAIT);
}
printk(KERN_DEBUG "%s: too many iterations (%d) in nv_nic_irq_tx.\n", dev->name, i);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
break;
}
@@ -2601,6 +2602,7 @@ static irqreturn_t nv_nic_irq_rx(int foo
u8 __iomem *base = get_hwbase(dev);
u32 events;
int i;
+ unsigned long flags;
dprintk(KERN_DEBUG "%s: nv_nic_irq_rx\n", dev->name);
@@ -2614,14 +2616,14 @@ static irqreturn_t nv_nic_irq_rx(int foo
nv_rx_process(dev, dev->weight);
if (nv_alloc_rx(dev)) {
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
if (!np->in_shutdown)
mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
}
if (i > max_interrupt_work) {
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
/* disable interrupts on the nic */
writel(NVREG_IRQ_RX_ALL, base + NvRegIrqMask);
pci_push(base);
@@ -2631,7 +2633,7 @@ static irqreturn_t nv_nic_irq_rx(int foo
mod_timer(&np->nic_poll, jiffies + POLL_WAIT);
}
printk(KERN_DEBUG "%s: too many iterations (%d) in nv_nic_irq_rx.\n", dev->name, i);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
break;
}
}
@@ -2648,6 +2650,7 @@ static irqreturn_t nv_nic_irq_other(int
u8 __iomem *base = get_hwbase(dev);
u32 events;
int i;
+ unsigned long flags;
dprintk(KERN_DEBUG "%s: nv_nic_irq_other\n", dev->name);
@@ -2660,14 +2663,14 @@ static irqreturn_t nv_nic_irq_other(int
break;
if (events & NVREG_IRQ_LINK) {
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
nv_link_irq(dev);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
}
if (np->need_linktimer && time_after(jiffies, np->link_timeout)) {
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
nv_linkchange(dev);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
np->link_timeout = jiffies + LINK_TIMEOUT;
}
if (events & (NVREG_IRQ_UNKNOWN)) {
@@ -2675,7 +2678,7 @@ static irqreturn_t nv_nic_irq_other(int
dev->name, events);
}
if (i > max_interrupt_work) {
- spin_lock_irq(&np->lock);
+ spin_lock_irqsave(&np->lock, flags);
/* disable interrupts on the nic */
writel(NVREG_IRQ_OTHER, base + NvRegIrqMask);
pci_push(base);
@@ -2685,7 +2688,7 @@ static irqreturn_t nv_nic_irq_other(int
mod_timer(&np->nic_poll, jiffies + POLL_WAIT);
}
printk(KERN_DEBUG "%s: too many iterations (%d) in nv_nic_irq_other.\n", dev->name, i);
- spin_unlock_irq(&np->lock);
+ spin_unlock_irqrestore(&np->lock, flags);
break;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] forcedeth: hardirq lockdep warning
2006-09-19 12:55 [PATCH] forcedeth: hardirq lockdep warning Peter Zijlstra
@ 2006-09-19 18:14 ` Andrew Morton
2006-09-19 18:28 ` Dave Jones
` (2 more replies)
2006-10-05 10:48 ` Jeff Garzik
1 sibling, 3 replies; 6+ messages in thread
From: Andrew Morton @ 2006-09-19 18:14 UTC (permalink / raw)
To: Peter Zijlstra, Ayaz Abdulla
Cc: linux-kernel, Jeff Garzik, Ingo Molnar, Arjan van de Ven,
Dave Jones
On Tue, 19 Sep 2006 14:55:22 +0200
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> BUG: warning at kernel/lockdep.c:1816/trace_hardirqs_on() (Not tainted)
I wonder what line that was. DEBUG_LOCKS_WARN_ON(current->hardirq_context),
I suppose.
> Call Trace:
> show_trace
> dump_stack
> trace_hardirqs_on
> :forcedeth:nv_nic_irq_other
> handle_IRQ_event
> __do_IRQ
> do_IRQ
> ret_from_intr
> DWARF2 barf
It's good, isn't it?
> default_idle
> cpu_idle
> rest_init
> start_kernel
> _sinittext
>
> These 3 functions nv_nic_irq_tx(), nv_nic_irq_rx() and nv_nic_irq_other()
> are reachable from IRQ context and process context.
That's "fix deadlock", not "fix lockdep warning". However it's often the
case that it's not really deadlockable because (often) the card's IRQ line
has been disabled. That appears to be the case in nv_do_nic_poll()'s call
to nv_nic_irq_tx, for example.
> Make use of the
> irq-save/restore spinlock variant.
So this perhaps is another lockdep workaround..
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] forcedeth: hardirq lockdep warning
2006-09-19 18:14 ` Andrew Morton
@ 2006-09-19 18:28 ` Dave Jones
2006-09-19 19:12 ` Peter Zijlstra
2006-09-20 1:04 ` Arjan van de Ven
2 siblings, 0 replies; 6+ messages in thread
From: Dave Jones @ 2006-09-19 18:28 UTC (permalink / raw)
To: Andrew Morton
Cc: Peter Zijlstra, Ayaz Abdulla, linux-kernel, Jeff Garzik,
Ingo Molnar, Arjan van de Ven
On Tue, Sep 19, 2006 at 11:14:48AM -0700, Andrew Morton wrote:
> On Tue, 19 Sep 2006 14:55:22 +0200
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>
> > BUG: warning at kernel/lockdep.c:1816/trace_hardirqs_on() (Not tainted)
>
> I wonder what line that was. DEBUG_LOCKS_WARN_ON(current->hardirq_context),
> I suppose.
That's what it matches up to in the Fedora kernel. (We have a bunch of lockdep
fixes scooped up from lkml over the last month or so, which may offset us).
Dave
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] forcedeth: hardirq lockdep warning
2006-09-19 18:14 ` Andrew Morton
2006-09-19 18:28 ` Dave Jones
@ 2006-09-19 19:12 ` Peter Zijlstra
2006-09-20 1:04 ` Arjan van de Ven
2 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2006-09-19 19:12 UTC (permalink / raw)
To: Andrew Morton
Cc: Ayaz Abdulla, linux-kernel, Jeff Garzik, Ingo Molnar,
Arjan van de Ven, Dave Jones
On Tue, 2006-09-19 at 11:14 -0700, Andrew Morton wrote:
> On Tue, 19 Sep 2006 14:55:22 +0200
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>
> > BUG: warning at kernel/lockdep.c:1816/trace_hardirqs_on() (Not tainted)
>
> I wonder what line that was. DEBUG_LOCKS_WARN_ON(current->hardirq_context),
> I suppose.
Correct indeed.
> > Call Trace:
> > show_trace
> > dump_stack
> > trace_hardirqs_on
> > :forcedeth:nv_nic_irq_other
> > handle_IRQ_event
> > __do_IRQ
> > do_IRQ
> > ret_from_intr
> > DWARF2 barf
>
> It's good, isn't it?
Yeah, I hope we'll get it sorted out quickly....
> > default_idle
> > cpu_idle
> > rest_init
> > start_kernel
> > _sinittext
> >
> > These 3 functions nv_nic_irq_tx(), nv_nic_irq_rx() and nv_nic_irq_other()
> > are reachable from IRQ context and process context.
>
> That's "fix deadlock", not "fix lockdep warning". However it's often the
> case that it's not really deadlockable because (often) the card's IRQ line
> has been disabled. That appears to be the case in nv_do_nic_poll()'s call
> to nv_nic_irq_tx, for example.
Yeah, I saw some of that. But I'm not well versed in the various netdev
irq receive paths nor in the driver. I couldn't find the actual
maintainer in the MAINTAINERS file nor in the source, thanks for
including him in the CC. Lets hope he can say if this is an actual fix
or just a workaround.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] forcedeth: hardirq lockdep warning
2006-09-19 18:14 ` Andrew Morton
2006-09-19 18:28 ` Dave Jones
2006-09-19 19:12 ` Peter Zijlstra
@ 2006-09-20 1:04 ` Arjan van de Ven
2 siblings, 0 replies; 6+ messages in thread
From: Arjan van de Ven @ 2006-09-20 1:04 UTC (permalink / raw)
To: Andrew Morton
Cc: Peter Zijlstra, Ayaz Abdulla, linux-kernel, Jeff Garzik,
Ingo Molnar, Dave Jones
> That's "fix deadlock", not "fix lockdep warning". However it's often the
> case that it's not really deadlockable because (often) the card's IRQ line
> has been disabled. That appears to be the case in nv_do_nic_poll()'s call
> to nv_nic_irq_tx, for example.
>
except when it's a shared irq line, then the game rules changed ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] forcedeth: hardirq lockdep warning
2006-09-19 12:55 [PATCH] forcedeth: hardirq lockdep warning Peter Zijlstra
2006-09-19 18:14 ` Andrew Morton
@ 2006-10-05 10:48 ` Jeff Garzik
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Garzik @ 2006-10-05 10:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Ingo Molnar, Arjan van de Ven, Dave Jones,
Andrew Morton
applied to #upstream-fixes
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-10-05 10:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-09-19 12:55 [PATCH] forcedeth: hardirq lockdep warning Peter Zijlstra
2006-09-19 18:14 ` Andrew Morton
2006-09-19 18:28 ` Dave Jones
2006-09-19 19:12 ` Peter Zijlstra
2006-09-20 1:04 ` Arjan van de Ven
2006-10-05 10:48 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox