| Message ID | 49d264dd.YwbXLw18YaD1tQqD%Larry.Finger@lwfinger.net |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Am Dienstag 31 März 2009 20:45:49 schrieb Larry Finger: > On an SMP system, the following message is printed. The patch below gets > fixes the problem. Are you sure it is never called with disabled interrupts? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Neukum <oliver@neukum.org> Date: Tue, 31 Mar 2009 23:50:38 +0200 > Am Dienstag 31 März 2009 20:45:49 schrieb Larry Finger: > > On an SMP system, the following message is printed. The patch below gets > > fixes the problem. > > Are you sure it is never called with disabled interrupts? Please be specific, what is this "it" you are referring to? Larry's patch modifies the locking many functions, and you didn't quote the kernel locking error message if that provides the context of your comment. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Neukum <oliver@neukum.org> Date: Wed, 1 Apr 2009 00:13:04 +0200 > Am Dienstag 31 März 2009 20:45:49 schrieb Larry Finger: > > > @@ -796,7 +799,7 @@ static int kaweth_start_xmit(struct sk_b > > > @@ -848,7 +851,7 @@ skip: > > net->trans_start = jiffies; > > } > > > > - spin_unlock(&kaweth->device_lock); > > + spin_unlock_irq(&kaweth->device_lock); > > Here you enable interrupts. Are you sure ndo_start_xmit is never > called with interrupts disabled? It must never be invoked that way, this would break so many drivers. On the other hand, all of these driver paths never execute in a real hardware interrupt context, the deepest it gets into is software interrupts. So spin_*lock*_bh() might be more appropriate. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag 31 März 2009 20:45:49 schrieb Larry Finger: > @@ -796,7 +799,7 @@ static int kaweth_start_xmit(struct sk_b > @@ -848,7 +851,7 @@ skip: > net->trans_start = jiffies; > } > > - spin_unlock(&kaweth->device_lock); > + spin_unlock_irq(&kaweth->device_lock); Here you enable interrupts. Are you sure ndo_start_xmit is never called with interrupts disabled? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Neukum wrote: > Am Dienstag 31 März 2009 20:45:49 schrieb Larry Finger: >> On an SMP system, the following message is printed. The patch below gets >> fixes the problem. > > Are you sure it is never called with disabled interrupts? I think not, but all I really know is that the change from spin_lock() to spin_lock_irq() makes the errors go away. I checked a number of drivers to see what their ndo_start_xmit routines do. Most use spin_lock_irq(), but some do no locking. I tried removing the locking, but that resulted in a stall even when I bypassed the faulty router. Larry -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch 01 April 2009 00:10:28 schrieb David Miller: > > Here you enable interrupts. Are you sure ndo_start_xmit is never > > called with interrupts disabled? > > It must never be invoked that way, this would break so many > drivers. > > On the other hand, all of these driver paths never execute > in a real hardware interrupt context, the deepest it gets > into is software interrupts. So spin_*lock*_bh() might be > more appropriate. Thanks. The patch is good. I'll check the other USB network drivers. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
================================= [ INFO: inconsistent lock state ] 2.6.29-Linus-05093-gc31f403 #57 --------------------------------- inconsistent {hardirq-on-W} -> {in-hardirq-W} usage. bash/4105 [HC1[1]:SC0[0]:HE0:SE1] takes: (&kaweth->device_lock){+...}, at: [<ffffffffa01aa286>] kaweth_usb_receive+0x77/0x1af [kaw eth] {hardirq-on-W} state was registered at: [<ffffffff80260503>] __lock_acquire+0x753/0x1685 [<ffffffff8026148a>] lock_acquire+0x55/0x71 [<ffffffff80461ba6>] _spin_lock+0x31/0x3d [<ffffffffa01aaa0c>] kaweth_start_xmit+0x2b/0x1e1 [kaweth] [<ffffffff803eccd3>] dev_hard_start_xmit+0x22e/0x2ad [<ffffffff803fe120>] __qdisc_run+0xf2/0x203 [<ffffffff803ed0cd>] dev_queue_xmit+0x263/0x39b [<ffffffffa03a47cb>] packet_sendmsg_spkt+0x1c4/0x20a [af_packet] [<ffffffff803de0c2>] sock_sendmsg+0xe4/0xfd [<ffffffff803dec8f>] sys_sendto+0xe4/0x10c [<ffffffff8020bccb>] system_call_fastpath+0x16/0x1b [<ffffffffffffffff>] 0xffffffffffffffff irq event stamp: 1280 hardirqs last enabled at (1279): [<ffffffff80461a71>] _spin_unlock_irqrestore+0x44/0x4c hardirqs last disabled at (1280): [<ffffffff8020bad7>] save_args+0x67/0x70 softirqs last enabled at (660): [<ffffffff8024192c>] __do_softirq+0x14d/0x15d softirqs last disabled at (651): [<ffffffff8020ce9c>] call_softirq+0x1c/0x28 Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net> --- Jeff, As the listed authors of this driver have not touched it in several years, I have taken the liberty of sending it to you as networking drivers maintainer. I hope this is OK. Larry --- Index: wireless-testing/drivers/net/usb/kaweth.c =================================================================== --- wireless-testing.orig/drivers/net/usb/kaweth.c +++ wireless-testing/drivers/net/usb/kaweth.c @@ -36,7 +36,6 @@ * Run test procedures * Fix bugs from previous two steps * Snoop other OSs for any tricks we're not doing - * SMP locking * Reduce arbitrary timeouts * Smart multicast support * Temporary MAC change support @@ -796,7 +799,7 @@ static int kaweth_start_xmit(struct sk_b int res; - spin_lock(&kaweth->device_lock); + spin_lock_irq(&kaweth->device_lock); kaweth_async_set_rx_mode(kaweth); netif_stop_queue(net); @@ -814,7 +817,7 @@ static int kaweth_start_xmit(struct sk_b if (!copied_skb) { kaweth->stats.tx_errors++; netif_start_queue(net); - spin_unlock(&kaweth->device_lock); + spin_unlock_irq(&kaweth->device_lock); return 0; } } @@ -848,7 +851,7 @@ skip: net->trans_start = jiffies; } - spin_unlock(&kaweth->device_lock); + spin_unlock_irq(&kaweth->device_lock); return 0; }