e100: Fix workqueue race

RFC PATCH In review Alan Cox 3 participants 5 messages Jan 21, 2010 – Jan 22, 2010

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Alan Cox

author

2010-01-21 17:13 UTC #0 lore raw

RFC PATCH 1/1 e100: Fix workqueue race

(Incidentally this doesn't seem to be the only net driver that looks suspect here)

e100: Fix the TX workqueue race

From: Alan Cox <[email protected]>

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

Signed-off-by Alan Cox <[email protected]>

---

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

drivers/net/e100.c +11 -2

index 5c7a155..5e02e4f 100644
@@ -2232,7 +2232,7 @@ err_rx_clean_list:
 	return err;
 }
 
-static void e100_down(struct nic *nic)
+static void e100_do_down(struct nic *nic)
 {
 	/* wait here for poll to complete */
 	napi_disable(&nic->napi);

@@ -2245,6 +2245,15 @@ static void e100_down(struct nic *nic)
 	e100_rx_clean_list(nic);
 }
 
+/* For the non TX timeout case we want to kill the tx timeout before
+   we do this otherwise a parallel tx timeout will make a nasty mess. */
+
+static void e100_down(struct nic *nic)
+{
+	cancel_work_sync(&nic->tx_timeout_task);
+	e100_do_down(nic);
+}
+
 static void e100_tx_timeout(struct net_device *netdev)
 {
 	struct nic *nic = netdev_priv(netdev);

@@ -2261,7 +2270,7 @@ static void e100_tx_timeout_task(struct work_struct *work)
 
 	DPRINTK(TX_ERR, DEBUG, "scb.status=0x%02X\n",
 		ioread8(&nic->csr->scb.status));
-	e100_down(netdev_priv(netdev));
+	e100_do_down(netdev_priv(netdev));
 	e100_up(netdev_priv(netdev));
 }

Stephen Hemminger

2010-01-21 17:20 UTC #1 lore raw

Quoted 5 lines · On Thu, 21 Jan 2010 16:48:01 +0000 Alan Cox <[email protected]> wrote:

(Incidentally this doesn't seem to be the only net driver that looks suspect here)
e100: Fix the TX workqueue race

Quoted 8 lines

From: Alan Cox <[email protected]>
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
Signed-off-by Alan Cox <[email protected]>

Most drivers solve this by getting rtnl_lock in the timeout work function.

Jarek Poplawski

2010-01-22 08:48 UTC #2 lore raw

In reply to Alan Cox

Quoted 33 lines · Alan Cox

(Incidentally this doesn't seem to be the only net driver that looks suspect here)
e100: Fix the TX workqueue race
From: Alan Cox <[email protected]>
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
Signed-off-by Alan Cox <[email protected]>
---
 drivers/net/e100.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)
drivers/net/e100.c +1 -1
index 5c7a155..5e02e4f 100644
@@ -2232,7 +2232,7 @@ err_rx_clean_list:
 	return err;
 }
 
-static void e100_down(struct nic *nic)
+static void e100_do_down(struct nic *nic)
 {
 	/* wait here for poll to complete */
 	napi_disable(&nic->napi);

drivers/net/e100.c static void e100_down(struct nic *nic) · line 2253

Quoted 9 lines

drivers/net/e100.c +6 -0

@@ -2245,6 +2245,15 @@ static void e100_down(struct nic *nic)
 	e100_rx_clean_list(nic);
 }
 
+/* For the non TX timeout case we want to kill the tx timeout before
+   we do this otherwise a parallel tx timeout will make a nasty mess. */
+
+static void e100_down(struct nic *nic)
+{
+	cancel_work_sync(&nic->tx_timeout_task);

Can't tx_timeout_task be triggered just between these two calls here?

Jarek P.

Quoted 16 lines

drivers/net/e100.c +4 -2

+	e100_do_down(nic);
+}
+
 static void e100_tx_timeout(struct net_device *netdev)
 {
 	struct nic *nic = netdev_priv(netdev);

@@ -2261,7 +2270,7 @@ static void e100_tx_timeout_task(struct work_struct *work)
 
 	DPRINTK(TX_ERR, DEBUG, "scb.status=0x%02X\n",
 		ioread8(&nic->csr->scb.status));
-	e100_down(netdev_priv(netdev));
+	e100_do_down(netdev_priv(netdev));
 	e100_up(netdev_priv(netdev));
 }
 
--

Jarek Poplawski

2010-01-22 09:07 UTC #3 lore raw

Quoted 37 lines · Jarek Poplawski

Quoted 36 lines · Alan Cox
(Incidentally this doesn't seem to be the only net driver that looks suspect here)
e100: Fix the TX workqueue race
From: Alan Cox <[email protected]>
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
Signed-off-by Alan Cox <[email protected]>
---
 drivers/net/e100.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)
drivers/net/e100.c +1 -1
index 5c7a155..5e02e4f 100644
@@ -2232,7 +2232,7 @@ err_rx_clean_list:
 	return err;
 }
 
-static void e100_down(struct nic *nic)
+static void e100_do_down(struct nic *nic)
 {
 	/* wait here for poll to complete */
 	napi_disable(&nic->napi);
@@ -2245,6 +2245,15 @@ static void e100_down(struct nic *nic)
 	e100_rx_clean_list(nic);
 }
 

Quoted 8 lines

drivers/net/e100.c static void e100_down(struct nic *nic) · line 2253
Quoted 6 lines
drivers/net/e100.c +6 -0
+/* For the non TX timeout case we want to kill the tx timeout before
+   we do this otherwise a parallel tx timeout will make a nasty mess. */
+
+static void e100_down(struct nic *nic)
+{
+	cancel_work_sync(&nic->tx_timeout_task);
Can't tx_timeout_task be triggered just between these two calls here?

More exactly: except when this is called from dev_close(), where it should work OK. (At least until tx_timeout_task doesn't take any lock held here - especially rtnl_lock.)

Jarek P.

Quoted 17 lines

drivers/net/e100.c +4 -2

+	e100_do_down(nic);
+}
+
 static void e100_tx_timeout(struct net_device *netdev)
 {
 	struct nic *nic = netdev_priv(netdev);

@@ -2261,7 +2270,7 @@ static void e100_tx_timeout_task(struct work_struct *work)
 
 	DPRINTK(TX_ERR, DEBUG, "scb.status=0x%02X\n",
 		ioread8(&nic->csr->scb.status));
-	e100_down(netdev_priv(netdev));
+	e100_do_down(netdev_priv(netdev));
 	e100_up(netdev_priv(netdev));
 }
 
--

Jarek Poplawski

2010-01-22 09:38 UTC #4 lore raw

Quoted 47 lines · Jarek Poplawski

Quoted 37 lines · Jarek Poplawski
Quoted 36 lines · Alan Cox
(Incidentally this doesn't seem to be the only net driver that looks suspect here)
e100: Fix the TX workqueue race
From: Alan Cox <[email protected]>
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
Signed-off-by Alan Cox <[email protected]>
---
 drivers/net/e100.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)
drivers/net/e100.c +1 -1
index 5c7a155..5e02e4f 100644
@@ -2232,7 +2232,7 @@ err_rx_clean_list:
 	return err;
 }
 
-static void e100_down(struct nic *nic)
+static void e100_do_down(struct nic *nic)
 {
 	/* wait here for poll to complete */
 	napi_disable(&nic->napi);
@@ -2245,6 +2245,15 @@ static void e100_down(struct nic *nic)
 	e100_rx_clean_list(nic);
 }
 
Quoted 8 lines
drivers/net/e100.c static void e100_down(struct nic *nic) · line 2253
Quoted 6 lines
drivers/net/e100.c +6 -0
+/* For the non TX timeout case we want to kill the tx timeout before
+   we do this otherwise a parallel tx timeout will make a nasty mess. */
+
+static void e100_down(struct nic *nic)
+{
+	cancel_work_sync(&nic->tx_timeout_task);
Can't tx_timeout_task be triggered just between these two calls here?

Quoted 3 lines

More exactly: except when this is called from dev_close(), where it should work OK. (At least until tx_timeout_task doesn't take any lock held here - especially rtnl_lock.)

Hmm... Even more exactly, since tx_timeout_task can be triggered not only by dev_watchdog(), dev_close() is suspicious too.

Jarek P.