[PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[Larry Finger]

On an SMP system, the following message is printed. The patch below gets
fixes the problem.

=================================
[ 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;
 }

Re: [PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[Oliver Neukum]

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

Re: [PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[David Miller]

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.

Re: [PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[David Miller]

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.

Re: [PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[Oliver Neukum]

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

Re: [PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[Larry Finger]

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

Re: [PATCH 1/2 V2] kaweth: Fix locking to be SMP-safe

[Oliver Neukum]

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