sky2: Lock transmit queue while disabling device

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Signed-off-by Mike McCormack<[email protected]>

---
  drivers/net/sky2.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/sky2.c +2 -0

index faa4841..8ae8520 100644
@@ -3176,7 +3176,9 @@ static void sky2_reset(struct sky2_hw *hw)
  static void sky2_detach(struct net_device *dev)
  {
  	if (netif_running(dev)) {
+		netif_tx_lock(dev);
  		netif_device_detach(dev);	/* stop txq */
+		netif_tx_unlock(dev);
  		sky2_down(dev);
  	}
  }

This doesn't solve the issue on my system. Just ran a test - same errors. I don't think that the under-load errors I'm seeing involve sky2_detatch. I'm thinking it's related to the dhcp AF_INTERNET fix that solved the lockup under load - that something outside of sky2 is corrupting an skb.

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Signed-off-by Mike McCormack <[email protected]>

---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/sky2.c +2 -0

index faa4841..8ae8520 100644
@@ -3176,7 +3176,9 @@ static void sky2_reset(struct sky2_hw *hw)
 static void sky2_detach(struct net_device *dev)
 {
 	if (netif_running(dev)) {
+		netif_tx_lock(dev);
 		netif_device_detach(dev);	/* stop txq */
+		netif_tx_unlock(dev);
 		sky2_down(dev);
 	}
 }

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Signed-off-by Mike McCormack <[email protected]>

---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/sky2.c +2 -0

index faa4841..8ae8520 100644
@@ -3176,7 +3176,9 @@ static void sky2_reset(struct sky2_hw *hw)
 static void sky2_detach(struct net_device *dev)
 {
 	if (netif_running(dev)) {
+		netif_tx_lock(dev);
 		netif_device_detach(dev);	/* stop txq */
+		netif_tx_unlock(dev);
 		sky2_down(dev);
 	}
 }

Quoted 6 lines

Quoted 6 lines

[ 8673.345873] sky2 eth0: receiver hang detected
[ 8673.350368] sky2 eth0: disabling interface
[ 8673.354749] BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
[ 8673.359748] IP: [<ffffffffa00373d3>] sky2_xmit_frame+0x321/0x5d8 

[sky2]

Mike McCormack wrote, On 12/31/2009 11:55 AM:

Quoted 10 lines

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Could you give some scenario of the oops/fix? Btw, even if it worked, you should use netif_tx_lock_bh version considering sky2_detach use contexts, I guess.

Jarek P.

Quoted 19 lines

Signed-off-by Mike McCormack <[email protected]>

---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/sky2.c +2 -0

index faa4841..8ae8520 100644
@@ -3176,7 +3176,9 @@ static void sky2_reset(struct sky2_hw *hw)
 static void sky2_detach(struct net_device *dev)
 {
 	if (netif_running(dev)) {
+		netif_tx_lock(dev);
 		netif_device_detach(dev);	/* stop txq */
+		netif_tx_unlock(dev);
 		sky2_down(dev);
 	}
 }

Hi Jarek,

This is based on my analysis of the oops at:

http://bugzilla.kernel.org/show_bug.cgi?id=14925

Specifically:

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

[ 8673.345873] sky2 eth0: receiver hang detected
[ 8673.350368] sky2 eth0: disabling interface
[ 8673.354749] BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
[ 8673.359748] IP: [<ffffffffa00373d3>] sky2_xmit_frame+0x321/0x5d8 

[sky2]

netif_device_detach() does not guarantee that all transmits have completed after it returns.

CPU 1 stack will look like:

dev_queue_xmit()

     HARD_TX_LOCK() -> __netif_tx_lock()
     ...
     dev_hard_start_xmit()
        ops->ndo_start_xmit()  -> sky2_xmit_frame()
        sky2_xmit_frame() pushing skb to hardware
          use NULL tx_ring here

CPU 2 stack will look like:

  sky2_restart()
     rtnl_lock()
     sky2_detach()
        netif_device_detach()
        sky2_down()
          printk("sky2 eth0: disabling interface")
          ...
          sky2_free_buffers(sky2);
            sky2->tx_ring = NULL;
          ...

Another way to solve the problem would be to take the transmit lock in netif_device_detach() to make sure that any in progress transmits have completed before returning.

Note that most of these backtraces are using the nvidia binary only module. This may change the timings and make the sky2 race more likely, or be involved in the "tx timeout" condition that triggers a sky2_restart().

Will test with netif_tx_lock_bh and resubmit.

thanks,

Mike

Quoted 38 lines · Jarek Poplawski

Mike McCormack wrote, On 12/31/2009 11:55 AM:

Quoted 10 lines

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Could you give some scenario of the oops/fix? Btw, even if it worked, you should use netif_tx_lock_bh version considering sky2_detach use contexts, I guess.

Jarek P.

Quoted 19 lines

Signed-off-by Mike McCormack <[email protected]>

---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/sky2.c +2 -0

index faa4841..8ae8520 100644
@@ -3176,7 +3176,9 @@ static void sky2_reset(struct sky2_hw *hw)
 static void sky2_detach(struct net_device *dev)
 {
 	if (netif_running(dev)) {
+		netif_tx_lock(dev);
 		netif_device_detach(dev);	/* stop txq */
+		netif_tx_unlock(dev);
 		sky2_down(dev);
 	}
 }

Well, that didn't fix it. Oops attached, looks pretty much the same to me.

Another way to solve the problem would be to take the transmit lock in netif_device_detach() to make sure that any in progress transmits have completed before returning.

Yes, it seems this lock might be needed around this place, but I'd like to check another idea: if it's not about awakening too early. Berck, could you try this patch?

---

 drivers/net/sky2.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/sky2.c +1 -1

index 1c01b96..31f1629 100644
@@ -1844,7 +1844,7 @@ static void sky2_tx_complete(struct sky2_port *sky2, u16 done)
 	sky2->tx_cons = idx;
 	smp_mb();
 
-	if (tx_avail(sky2) > MAX_SKB_TX_LE + 4)
+	if (tx_avail(sky2) > MAX_SKB_TX_LE + 4 && netif_device_present(dev))
 		netif_wake_queue(dev);
 }
 

Quoted 3 lines · Jarek Poplawski

Yes, it seems this lock might be needed around this place, but I'd like to check another idea: if it's not about awakening too early. Berck, could you try this patch?

Okay, after running with that for several days I have not gotten the oops. That doesn't mean that I won't tomorrow, but I've gotten several of these:

[45621.704025] sky2 eth0: hung mac 124:21 fifo 195 (127:122)
[45621.704027] sky2 eth0: receiver hang detected
[45621.708524] sky2 eth0: disabling interface
[45621.715229] sky2 eth0: enabling interface
[45624.862111] sky2 eth0: Link is up at 1000 Mbps, full duplex, flow
control both
[61024.704036] sky2 eth0: hung mac 124:75 fifo 195 (133:128)
[61024.704039] sky2 eth0: receiver hang detected
[61024.708487] sky2 eth0: disabling interface
[61024.714791] sky2 eth0: enabling interface
[61027.864288] sky2 eth0: Link is up at 1000 Mbps, full duplex, flow

control both

And it hasn't crashed. The "receiver hang detected" would often (but not always) be followed by the oops before.

Berck

Quoted 20 lines · Berck E. Nash

Quoted 3 lines · Jarek Poplawski

Yes, it seems this lock might be needed around this place, but I'd like to check another idea: if it's not about awakening too early. Berck, could you try this patch?

Okay, after running with that for several days I have not gotten the oops. That doesn't mean that I won't tomorrow, but I've gotten several of these:

[45621.704025] sky2 eth0: hung mac 124:21 fifo 195 (127:122)
[45621.704027] sky2 eth0: receiver hang detected
[45621.708524] sky2 eth0: disabling interface
[45621.715229] sky2 eth0: enabling interface
[45624.862111] sky2 eth0: Link is up at 1000 Mbps, full duplex, flow
control both
[61024.704036] sky2 eth0: hung mac 124:75 fifo 195 (133:128)
[61024.704039] sky2 eth0: receiver hang detected
[61024.708487] sky2 eth0: disabling interface
[61024.714791] sky2 eth0: enabling interface
[61027.864288] sky2 eth0: Link is up at 1000 Mbps, full duplex, flow

Quoted 6 lines

control both

And it hasn't crashed. The "receiver hang detected" would often (but not always) be followed by the oops before.

Berck

OK, here it is with some cosmetics; let David decide if it needs more testing.

Thanks everybody,
Jarek P.
------------->

During TX timeout procedure dev could be awaken too early, e.g. by sky2_complete_tx() called from sky2_down(). Then sky2_xmit_frame() can run while buffers are freed causing an oops. This patch fixes it by adding netif_device_present() test in sky2_tx_complete().

Fixes http://bugzilla.kernel.org/show_bug.cgi?id=14925

With debugging by: Mike McCormack <[email protected]>

Reported-by Berck E. Nash <[email protected]>

Tested-by Berck E. Nash <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---

 drivers/net/sky2.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

drivers/net/sky2.c +0 -0

index 1c01b96..2f32fab 100644

Review

Quoted 7 lines

(quoted diff) +3 -1

-	if (tx_avail(sky2) > MAX_SKB_TX_LE + 4)
+	/* Wake unless it's detached, and called e.g. from sky2_down() */
+	if (tx_avail(sky2) > MAX_SKB_TX_LE + 4 &&
+	    likely(netif_device_present(dev)))
 		netif_wake_queue(dev);
 }

The likely() seems unnecessary noise here. It can't possibly matter that much.

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Signed-off-by Mike McCormack <[email protected]>

From: Mike McCormack <[email protected]>
Date: Thu, 31 Dec 2009 19:55:31 +0900

Quoted 11 lines

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Quoted 1 line

Signed-off-by Mike McCormack <[email protected]>

Applied to net-next-2.6

Stephen has asked for some further refinements, once that is all sorted we can think about backporting this to net-2.6 and -stable if necessary.

Quoted 14 lines

From: Mike McCormack <[email protected]>
Date: Thu, 31 Dec 2009 19:55:31 +0900

Quoted 11 lines

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Quoted 8 lines

Quoted 1 line

Signed-off-by Mike McCormack <[email protected]>

Applied to net-next-2.6

Stephen has asked for some further refinements, once that is all sorted we can think about backporting this to net-2.6 and -stable if necessary.

David, I'm not sure you chose the right (working) patch from this thread. Please, reconsider this:
Subject: [PATCH v2] sky2: Fix oops in sky2_xmit_frame() after TX timeout
Date: Mon, 4 Jan 2010 19:48:41 +0100
http://permalink.gmane.org/gmane.linux.network/148160

From: Jarek Poplawski <[email protected]>
Date: Thu, 7 Jan 2010 06:35:46 +0000

Quoted 24 lines · David Miller

Quoted 14 lines

From: Mike McCormack <[email protected]>
Date: Thu, 31 Dec 2009 19:55:31 +0900

Quoted 11 lines

netif_device_detach() does not take the tx_lock, so it's
possible that a call to sky2_xmit_frame is still in progress after netif_device_detach() is complete.

Take netif_tx_lock() to make sure all transmits have
stopped while we're disabling the devices and that no other CPU is still transmitting a frame after we've disabling the device.

Proposed fix for "sky2 panic under load" reported by Berck E. Nash.

Quoted 8 lines

Quoted 1 line

Signed-off-by Mike McCormack <[email protected]>

Applied to net-next-2.6

Stephen has asked for some further refinements, once that is all sorted we can think about backporting this to net-2.6 and -stable if necessary.

Quoted 5 lines

David, I'm not sure you chose the right (working) patch from this thread. Please, reconsider this:
Subject: [PATCH v2] sky2: Fix oops in sky2_xmit_frame() after TX timeout
Date: Mon, 4 Jan 2010 19:48:41 +0100
http://permalink.gmane.org/gmane.linux.network/148160

These are two seperate crashes and two seperate sets of patches.

Mike's patch fixes crashes due to races when bringing the device down or suspending it.

The other patch you reference is handling something different, a crash that happens while the device is staying up.

Right?

Maybe right, but Mike's patch changelog above writes:
"Proposed fix for "sky2 panic under load" reported by Berck E. Nash.", and maybe I missed something, but I can't find any report which acknowledges this patch fixed this or down/suspending problem.

From: Jarek Poplawski <[email protected]>
Date: Thu, 7 Jan 2010 08:15:01 +0000

Quoted 4 lines

Maybe right, but Mike's patch changelog above writes:
"Proposed fix for "sky2 panic under load" reported by Berck E. Nash.", and maybe I missed something, but I can't find any report which acknowledges this patch fixed this or down/suspending problem.

Certainly by my reading Mike's patch does in fact fix a race between device detach and the TX handler, so regardless of whether it fixes actual reported user problems it should at a minimum go into net-next-2.6

And that's _exactly_ what I did, and how I analyzed his work.

From: Jarek Poplawski <[email protected]>
Date: Thu, 7 Jan 2010 08:15:01 +0000

Quoted 4 lines

Maybe right, but Mike's patch changelog above writes:
"Proposed fix for "sky2 panic under load" reported by Berck E. Nash.", and maybe I missed something, but I can't find any report which acknowledges this patch fixed this or down/suspending problem.

Certainly by my reading Mike's patch does in fact fix a race between device detach and the TX handler, so regardless of whether it fixes actual reported user problems it should at a minimum go into net-next-2.6

And that's _exactly_ what I did, and how I analyzed his work.