Skip to contentSkip to conversation

skge: Fix race in tx path

RFC PATCH In review Bart Van Assche 3 participants 5 messages Feb 24, 2012 – Mar 5, 2012

Subsystems (3)
Networking Network drivers Marvell gigabit Ethernet drivers (skge/sky2)
lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

RFC PATCH 1/1 skge: Fix race in tx path

Make sure that tx_ring.to_use is updated before skge_tx_done() reads it.

Fixes the following BUG on my setup:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000525
IP: [<ffffffff8130c95e>] net_rx_action+0x9e/0x290
Call Trace:
 [<ffffffff8104cfb1>] ? __do_softirq+0x81/0x250
 [<ffffffff8104cffd>] __do_softirq+0xcd/0x250
 [<ffffffff8104d297>] run_ksoftirqd+0x117/0x1e0
 [<ffffffff8104d180>] ? __do_softirq+0x250/0x250
 [<ffffffff81069786>] kthread+0x96/0xa0
 [<ffffffff813e8534>] kernel_thread_helper+0x4/0x10
 [<ffffffff813de75d>] ? retint_restore_args+0xe/0xe
 [<ffffffff810696f0>] ? __init_kthread_worker+0x70/0x70
 [<ffffffff813e8530>] ? gs_change+0xb/0xb
Signed-off-by Bart Van Assche <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc Jeff Garzik <[email protected]>

---
Note: I'm posting this patch as an RFC since I'm not a network driver expert.

 drivers/net/ethernet/marvell/skge.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
drivers/net/ethernet/marvell/skge.c +2 -4
index dea0cb4..2d805b9 100644
@@ -2793,7 +2793,8 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 		tf->control |= BMU_EOF | BMU_IRQ_EOF;
 	}
 	/* Make sure all the descriptors written */
-	wmb();
+	skge->tx_ring.to_use = e->next;
+	smp_wmb();
 	td->control = BMU_OWN | BMU_SW | BMU_STF | control | len;
 	wmb();
 

@@ -2803,9 +2804,6 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 		     "tx queued, slot %td, len %d\n",
 		     e - skge->tx_ring.start, skb->len);
 
-	skge->tx_ring.to_use = e->next;
-	smp_wmb();
-
 	if (skge_avail(&skge->tx_ring) <= TX_LOW_WATER) {
 		netdev_dbg(dev, "transmit queue full\n");
 		netif_stop_queue(dev);
Signature · 1 line
1.7.3.4
#1 lore raw

Bart Van Assche <[email protected]> :
[...]

drivers/net/ethernet/marvell/skge.c static netdev_tx_t skge_xmit_frame(struct sk_buff *skb, · line 2799

Quoted 13 lines
drivers/net/ethernet/marvell/skge.c +2 -1
index dea0cb4..2d805b9 100644
@@ -2793,7 +2793,8 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 		tf->control |= BMU_EOF | BMU_IRQ_EOF;
 	}
 	/* Make sure all the descriptors written */
-	wmb();
+	skge->tx_ring.to_use = e->next;
+	smp_wmb();
 	td->control = BMU_OWN | BMU_SW | BMU_STF | control | len;
 	wmb();

With this change it seems possible for skge_tx_done to unmap a buffer before it is sent when the requests for skge_xmit_frame are close enough.

Signature · 1 line
Ueimor
author
#2 lore raw
Quoted 2 lines

With this change it seems possible for skge_tx_done to unmap a buffer before it is sent when the requests for skge_xmit_frame are close enough.

Thanks for the feedback. Does the patch below look better ?

---
 drivers/net/ethernet/marvell/skge.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/net/ethernet/marvell/skge.c +3 -3
index 33947ac..55723b5 100644
@@ -2800,15 +2800,15 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 
 	netdev_sent_queue(dev, skb->len);
 
+	skge->tx_ring.to_use = e->next;
+	smp_wmb();
+
 	skge_write8(hw, Q_ADDR(txqaddr[skge->port], Q_CSR), CSR_START);
 
 	netif_printk(skge, tx_queued, KERN_DEBUG, skge->netdev,
 		     "tx queued, slot %td, len %d\n",
 		     e - skge->tx_ring.start, skb->len);
 
-	skge->tx_ring.to_use = e->next;
-	smp_wmb();
-
 	if (skge_avail(&skge->tx_ring) <= TX_LOW_WATER) {
 		netdev_dbg(dev, "transmit queue full\n");
 		netif_stop_queue(dev);
Signature · 1 line
1.7.3.4
#3 lore raw

Bart Van Assche <[email protected]> :

Quoted 4 lines
Quoted 2 lines

With this change it seems possible for skge_tx_done to unmap a buffer before it is sent when the requests for skge_xmit_frame are close enough.

Thanks for the feedback. Does the patch below look better ?

Yes.

Signature · 1 line
Ueimor
#4 lore raw
In reply to Bart Van Assche
Quoted 22 lines · On Mon, 27 Feb 2012 19:18:58 +0000 Bart Van Assche <[email protected]> wrote:
Quoted 2 lines

With this change it seems possible for skge_tx_done to unmap a buffer before it is sent when the requests for skge_xmit_frame are close enough.

Thanks for the feedback. Does the patch below look better ?

---
 drivers/net/ethernet/marvell/skge.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/net/ethernet/marvell/skge.c +3 -0
index 33947ac..55723b5 100644
@@ -2800,15 +2800,15 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
 
 	netdev_sent_queue(dev, skb->len);
 
+	skge->tx_ring.to_use = e->next;
+	smp_wmb();
+
 	skge_write8(hw, Q_ADDR(txqaddr[skge->port], Q_CSR), CSR_START);

drivers/net/ethernet/marvell/skge.c static netdev_tx_t skge_xmit_frame(struct sk_buff *skb, · line 2814

Quoted 10 lines
drivers/net/ethernet/marvell/skge.c +0 -3
 	netif_printk(skge, tx_queued, KERN_DEBUG, skge->netdev,
 		     "tx queued, slot %td, len %d\n",
 		     e - skge->tx_ring.start, skb->len);
 
-	skge->tx_ring.to_use = e->next;
-	smp_wmb();
-
 	if (skge_avail(&skge->tx_ring) <= TX_LOW_WATER) {
 		netdev_dbg(dev, "transmit queue full\n");
 		netif_stop_queue(dev);

I don't see how the problem you are seeing.
If the race happens with old code:

ring element is setup and started.
IRQ happens
........ skge_tx_done()
starts cleaning stops prematurely
ring_to_use is updated
one skb stays stuck in ring until next TX clean

So the race would just leave one skb behind (until next transmit). Looking at tg3, you will see similar code.
More likely mmiowb() is needd at end of tx.