e100: Fix the TX workqueue race

I'd assumed someone would have picked up on this and fixed it using rtnl_lock as was suggested but it seems to have fallen through the cracks ?

Anyway this is I assume what was meant ?

---

Nothing stops the workqueue being left to run in parallel with close or a few other operations. This causes double unmaps and the like.

See kerneloops.org #1041230 for an example

---

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

index 3e8d000..859e833 100644
@@ -2280,8 +2280,13 @@ static void e100_tx_timeout_task(struct work_struct *work)
 
 	netif_printk(nic, tx_err, KERN_DEBUG, nic->netdev,
 		     "scb.status=0x%02X\n", ioread8(&nic->csr->scb.status));
-	e100_down(netdev_priv(netdev));
-	e100_up(netdev_priv(netdev));
+
+	rtnl_lock();
+	if (netif_running(dev)) {
+		e100_down(netdev_priv(netdev));
+		e100_up(netdev_priv(netdev));
+	}
+	rtnl_unlock();
 }
 
 static int e100_loopback_test(struct nic *nic, enum loopback loopback_mode)

Glad someone finally fixed this, it has bugged me for years...

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

From: Alan Cox <[email protected]>
Date: Fri, 23 Apr 2010 15:34:43 +0100

I hope this doesn't deadlock with linkwatch, as that's generally a problem we hit with trying to take RTNL from workqueues in the networking.

Linkwatch takes the RTNL lock, and then can make calls into the driver in it's main work loop.

But since you don't hold any driver locks here (you can't as if we did we couldn't take the RTNL lock here at all) so it should be OK.

I'll apply this to net-2.6, thanks Alan.

From: David Miller <[email protected]>
Date: Fri, 23 Apr 2010 16:31:27 -0700 (PDT)

Nevermind...

Doesn't apply to net-2.6, but even when I fix that up it doesn't even compile. There is no 'dev' variable present etc.

You even use a combination of "dev" and "netdev" in the resulting code block.

If it doesn't even build, I doubt it's been tested either.

Please resolve this and get some testing on it, thanks.

Puzzling as it came from a building -next tree. Will see whats happened next week if I get time, but I'm afraid net stuff isn't a priority - in fact its disappointing that having diagnosed a bug months ago (which was the hard bit) and posted a test patch months ago the maintainers haven't fixed it.

Alan

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

From: Alan Cox <[email protected]>
Date: Sat, 24 Apr 2010 11:36:29 +0100

It's disappointing to me that someone as experienced and skilled as yourself can't generate a clean patch which is 1) against the appropriate tree for a bug fix and 2) actually compiles.

Or is this too much to ask? :-)

No idea why it won't apply - I guess net has diverged from -next in this area. Other problem is not typing "stg ref" before "stg export"

(If it doesn't apply I'll look at it next weekend when -next and -net ought to be back in sync ?)

commit 526fb792b745da7c9532725a1a6ecc83a01110cf

Author: Alan Cox <[email protected]>
Date:   Sat Apr 24 12:09:23 2010 +0100

e100: Fix the TX workqueue race

Nothing stops the workqueue being left to run in parallel with close or a few other operations. This causes double unmaps and the like.

See kerneloops.org #1041230 for an example

index 3e8d000..ef97bfc 100644
@@ -168,6 +168,7 @@
 #include <linux/ethtool.h>
 #include <linux/string.h>
 #include <linux/firmware.h>
+#include <linux/rtnetlink.h>
 #include <asm/unaligned.h>
 
 

@@ -2280,8 +2281,13 @@ static void e100_tx_timeout_task(struct work_struct *work)
 
 	netif_printk(nic, tx_err, KERN_DEBUG, nic->netdev,
 		     "scb.status=0x%02X\n", ioread8(&nic->csr->scb.status));
-	e100_down(netdev_priv(netdev));
-	e100_up(netdev_priv(netdev));
+
+	rtnl_lock();
+	if (netif_running(netdev)) {
+		e100_down(netdev_priv(netdev));
+		e100_up(netdev_priv(netdev));
+	}
+	rtnl_unlock();
 }
 
 static int e100_loopback_test(struct nic *nic, enum loopback loopback_mode)

From: Alan Cox <[email protected]>
Date: Sat, 24 Apr 2010 12:11:27 +0100

It has, the debug print statement above the lines you are changing are completely different.

Please generate this patch against net-2.6 so I can apply it, thanks Alan.

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

From: David Miller <[email protected]>
Date: Sat, 24 Apr 2010 19:58:59 -0700 (PDT)

Nevermind, I took care of this for you.

------------------------------------------------------------------------------
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired