Skip to contentSkip to conversation

via-rhine rework

PATCH In review Francois Romieu 2 participants 10 messages May 1, 2015 – May 4, 2015

Subsystems (3)
Networking Network drivers Via rhine network driver
lore
Share & tools
lore

Francois Romieu

author
#0 lore raw

cover via-rhine rework

The series applies against davem-next as of 9dd3c797496affd699805c8a9d8429ad318c892f ("drivers: net: xgene: fix kbuild warnings").

Patches #1..#4 avoid holes in the receive ring.

Patch #5 is a small leftover cleanup for #1..#4.

Patches #6 and #7 are fairly simple barrier stuff.

Patch #8 closes some SMP transmit races - not that anyone really complained about these but it's a bit hard to handwave that they can be safely ignored. Some testing, especially SMP testing of course, would be welcome.

. Changes since #2:
- added dma_rmb barrier in vlan related patch 6.
- s/wmb/dma_wmb/ in (*new*) patch 7 of 8.
- added explicit SMP barriers in (*new*) patch 8 of 8.

. Changes since #1:
- turned wmb() into dma_wmb() as suggested by davem and Alexander Duyck in patch 1 of 6.
- forgot to reset rx_head_desc in rhine_reset_rbufs in patch 4 of 6.
- removed rx_head_desc altogether in (*new*) patch 5 of 6
- remoed some vlan receive uglyness in (*new*) patch 6 of 6.

Francois Romieu (8):
via-rhine: commit receive buffer address before descriptor status update.
via-rhine: allocate and map receive buffer in a single transaction
via-rhine: gotoize rhine_open error path.
via-rhine: forbid holes in the receive descriptor ring.
via-rhine: kiss rx_head_desc goodbye.
via-rhine: add consistent memory barrier in vlan receive code.
via-rhine: dma_wmb transmit barrier.
via-rhine: close SMP transmit races.

 drivers/net/ethernet/via/via-rhine.c | 249 +++++++++++++++++++++++------------
 1 file changed, 162 insertions(+), 87 deletions(-)
Signature · 1 line
Ueimor
author
#1 lore raw

PATCH 1/8 via-rhine: commit receive buffer address before descriptor status update.

Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/ethernet/via/via-rhine.c +1 -0
index de28504..88b45c7 100644
@@ -2075,6 +2075,7 @@ static int rhine_rx(struct net_device *dev, int limit)
 				break;
 			}
 			rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
+			dma_wmb();
 		}
 		rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
 	}
Signature · 1 line
Ueimor
author
#2 lore raw

PATCH 2/8 via-rhine: allocate and map receive buffer in a single transaction

In reply to Francois Romieu

It's used to initialize the receive ring but it will actually shine when the receive poll code is reworked.

Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 57 +++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 14 deletions(-)
drivers/net/ethernet/via/via-rhine.c +43 -14
index 88b45c7..aa398ea 100644
@@ -1213,12 +1213,47 @@ static void free_ring(struct net_device* dev)
 
 }
 
-static void alloc_rbufs(struct net_device *dev)
+struct rhine_skb_dma {
+	struct sk_buff *skb;
+	dma_addr_t dma;
+};
+
+static inline int rhine_skb_dma_init(struct net_device *dev,
+				     struct rhine_skb_dma *sd)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	struct device *hwdev = dev->dev.parent;
+	const int size = rp->rx_buf_sz;
+
+	sd->skb = netdev_alloc_skb(dev, size);
+	if (!sd->skb)
+		return -ENOMEM;
+
+	sd->dma = dma_map_single(hwdev, sd->skb->data, size, DMA_FROM_DEVICE);
+	if (unlikely(dma_mapping_error(hwdev, sd->dma))) {
+		netif_err(rp, drv, dev, "Rx DMA mapping failure\n");
+		dev_kfree_skb_any(sd->skb);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
+					   struct rhine_skb_dma *sd, int entry)
+{
+	rp->rx_skbuff_dma[entry] = sd->dma;
+	rp->rx_skbuff[entry] = sd->skb;
+
+	rp->rx_ring[entry].addr = cpu_to_le32(sd->dma);
+	dma_wmb();
+}
+
+static void alloc_rbufs(struct net_device *dev)
+{
+	struct rhine_private *rp = netdev_priv(dev);
 	dma_addr_t next;
-	int i;
+	int rc, i;
 
 	rp->dirty_rx = rp->cur_rx = 0;
 

@@ -1239,20 +1274,14 @@ static void alloc_rbufs(struct net_device *dev)
 
 	/* Fill in the Rx buffers.  Handle allocation failure gracefully. */
 	for (i = 0; i < RX_RING_SIZE; i++) {
-		struct sk_buff *skb = netdev_alloc_skb(dev, rp->rx_buf_sz);
-		rp->rx_skbuff[i] = skb;
-		if (skb == NULL)
-			break;
+		struct rhine_skb_dma sd;
 
-		rp->rx_skbuff_dma[i] =
-			dma_map_single(hwdev, skb->data, rp->rx_buf_sz,
-				       DMA_FROM_DEVICE);
-		if (dma_mapping_error(hwdev, rp->rx_skbuff_dma[i])) {
-			rp->rx_skbuff_dma[i] = 0;
-			dev_kfree_skb(skb);
+		rc = rhine_skb_dma_init(dev, &sd);
+		if (rc < 0)
 			break;
-		}
-		rp->rx_ring[i].addr = cpu_to_le32(rp->rx_skbuff_dma[i]);
+
+		rhine_skb_dma_nic_store(rp, &sd, i);
+
 		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
 	}
 	rp->dirty_rx = (unsigned int)(i - RX_RING_SIZE);
Signature · 1 line
Ueimor
author
#3 lore raw

PATCH 3/8 via-rhine: gotoize rhine_open error path.

In reply to Francois Romieu
Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/net/ethernet/via/via-rhine.c +10 -6
index aa398ea..91661e0 100644
@@ -1688,15 +1688,14 @@ static int rhine_open(struct net_device *dev)
 
 	rc = request_irq(rp->irq, rhine_interrupt, IRQF_SHARED, dev->name, dev);
 	if (rc)
-		return rc;
+		goto out;
 
 	netif_dbg(rp, ifup, dev, "%s() irq %d\n", __func__, rp->irq);
 
 	rc = alloc_ring(dev);
-	if (rc) {
-		free_irq(rp->irq, dev);
-		return rc;
-	}
+	if (rc < 0)
+		goto out_free_irq;
+
 	alloc_rbufs(dev);
 	alloc_tbufs(dev);
 	rhine_chip_reset(dev);

@@ -1709,7 +1708,12 @@ static int rhine_open(struct net_device *dev)
 
 	netif_start_queue(dev);
 
-	return 0;
+out:
+	return rc;
+
+out_free_irq:
+	free_irq(rp->irq, dev);
+	goto out;
 }
 
 static void rhine_reset_task(struct work_struct *work)
Signature · 1 line
Ueimor
author
#4 lore raw

PATCH 4/8 via-rhine: forbid holes in the receive descriptor ring.

In reply to Francois Romieu

Rationales:
- throttle work under memory pressure
- lower receive descriptor recycling latency for the network adapter
- lower the maintenance burden of uncommon paths

The patch is twofold:
- it fails early if the receive ring can't be completely initialized at dev->open() time
- it drops packets on the floor in the napi receive handler so as to keep the received ring full

Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 103 ++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 50 deletions(-)
drivers/net/ethernet/via/via-rhine.c +53 -50
index 91661e0..8d322bb 100644
@@ -473,7 +473,7 @@ struct rhine_private {
 	/* Frequently used values: keep some adjacent for cache effect. */
 	u32 quirks;
 	struct rx_desc *rx_head_desc;
-	unsigned int cur_rx, dirty_rx;	/* Producer/consumer ring indices */
+	unsigned int cur_rx;
 	unsigned int cur_tx, dirty_tx;
 	unsigned int rx_buf_sz;		/* Based on MTU+slack. */
 	struct rhine_stats rx_stats;

@@ -1239,6 +1239,17 @@ static inline int rhine_skb_dma_init(struct net_device *dev,
 	return 0;
 }
 
+static void rhine_reset_rbufs(struct rhine_private *rp)
+{
+	int i;
+
+	rp->cur_rx = 0;
+	rp->rx_head_desc = rp->rx_ring;
+
+	for (i = 0; i < RX_RING_SIZE; i++)
+		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
+}
+
 static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
 					   struct rhine_skb_dma *sd, int entry)
 {

@@ -1249,16 +1260,15 @@ static inline void rhine_skb_dma_nic_store(struct rhine_private *rp,
 	dma_wmb();
 }
 
-static void alloc_rbufs(struct net_device *dev)
+static void free_rbufs(struct net_device* dev);
+
+static int alloc_rbufs(struct net_device *dev)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	dma_addr_t next;
 	int rc, i;
 
-	rp->dirty_rx = rp->cur_rx = 0;
-
 	rp->rx_buf_sz = (dev->mtu <= 1500 ? PKT_BUF_SZ : dev->mtu + 32);
-	rp->rx_head_desc = &rp->rx_ring[0];
 	next = rp->rx_ring_dma;
 
 	/* Init the ring entries */

@@ -1277,14 +1287,17 @@ static void alloc_rbufs(struct net_device *dev)
 		struct rhine_skb_dma sd;
 
 		rc = rhine_skb_dma_init(dev, &sd);
-		if (rc < 0)
-			break;
+		if (rc < 0) {
+			free_rbufs(dev);
+			goto out;
+		}
 
 		rhine_skb_dma_nic_store(rp, &sd, i);
-
-		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);
 	}
-	rp->dirty_rx = (unsigned int)(i - RX_RING_SIZE);
+
+	rhine_reset_rbufs(rp);
+out:
+	return rc;
 }
 
 static void free_rbufs(struct net_device* dev)

@@ -1696,7 +1709,10 @@ static int rhine_open(struct net_device *dev)
 	if (rc < 0)
 		goto out_free_irq;
 
-	alloc_rbufs(dev);
+	rc = alloc_rbufs(dev);
+	if (rc < 0)
+		goto out_free_ring;
+
 	alloc_tbufs(dev);
 	rhine_chip_reset(dev);
 	rhine_task_enable(rp);

@@ -1711,6 +1727,8 @@ static int rhine_open(struct net_device *dev)
 out:
 	return rc;
 
+out_free_ring:
+	free_ring(dev);
 out_free_irq:
 	free_irq(rp->irq, dev);
 	goto out;

@@ -1733,9 +1751,9 @@ static void rhine_reset_task(struct work_struct *work)
 
 	/* clear all descriptors */
 	free_tbufs(dev);
-	free_rbufs(dev);
 	alloc_tbufs(dev);
-	alloc_rbufs(dev);
+
+	rhine_reset_rbufs(rp);
 
 	/* Reinitialize the hardware. */
 	rhine_chip_reset(dev);

@@ -2033,16 +2051,18 @@ static int rhine_rx(struct net_device *dev, int limit)
 				}
 			}
 		} else {
-			struct sk_buff *skb = NULL;
 			/* Length should omit the CRC */
 			int pkt_len = data_size - 4;
+			struct sk_buff *skb;
 			u16 vlan_tci = 0;
 
 			/* Check if the packet is long enough to accept without
 			   copying to a minimally-sized skbuff. */
-			if (pkt_len < rx_copybreak)
+			if (pkt_len < rx_copybreak) {
 				skb = netdev_alloc_skb_ip_align(dev, pkt_len);
-			if (skb) {
+				if (unlikely(!skb))
+					goto drop;
+
 				dma_sync_single_for_cpu(hwdev,
 							rp->rx_skbuff_dma[entry],
 							rp->rx_buf_sz,

@@ -2051,25 +2071,28 @@ static int rhine_rx(struct net_device *dev, int limit)
 				skb_copy_to_linear_data(skb,
 						 rp->rx_skbuff[entry]->data,
 						 pkt_len);
-				skb_put(skb, pkt_len);
+
 				dma_sync_single_for_device(hwdev,
 							   rp->rx_skbuff_dma[entry],
 							   rp->rx_buf_sz,
 							   DMA_FROM_DEVICE);
 			} else {
+				struct rhine_skb_dma sd;
+
+				if (unlikely(rhine_skb_dma_init(dev, &sd) < 0))
+					goto drop;
+
 				skb = rp->rx_skbuff[entry];
-				if (skb == NULL) {
-					netdev_err(dev, "Inconsistent Rx descriptor chain\n");
-					break;
-				}
-				rp->rx_skbuff[entry] = NULL;
-				skb_put(skb, pkt_len);
+
 				dma_unmap_single(hwdev,
 						 rp->rx_skbuff_dma[entry],
 						 rp->rx_buf_sz,
 						 DMA_FROM_DEVICE);
+				rhine_skb_dma_nic_store(rp, &sd, entry);
 			}
 
+			skb_put(skb, pkt_len);
+
 			if (unlikely(desc_length & DescTag))
 				vlan_tci = rhine_get_vlan_tci(skb, data_size);
 

@@ -2084,36 +2107,17 @@ static int rhine_rx(struct net_device *dev, int limit)
 			rp->rx_stats.packets++;
 			u64_stats_update_end(&rp->rx_stats.syncp);
 		}
+give_descriptor_to_nic:
+		desc->rx_status = cpu_to_le32(DescOwn);
 		entry = (++rp->cur_rx) % RX_RING_SIZE;
 		rp->rx_head_desc = &rp->rx_ring[entry];
 	}
 
-	/* Refill the Rx ring buffers. */
-	for (; rp->cur_rx - rp->dirty_rx > 0; rp->dirty_rx++) {
-		struct sk_buff *skb;
-		entry = rp->dirty_rx % RX_RING_SIZE;
-		if (rp->rx_skbuff[entry] == NULL) {
-			skb = netdev_alloc_skb(dev, rp->rx_buf_sz);
-			rp->rx_skbuff[entry] = skb;
-			if (skb == NULL)
-				break;	/* Better luck next round. */
-			rp->rx_skbuff_dma[entry] =
-				dma_map_single(hwdev, skb->data,
-					       rp->rx_buf_sz,
-					       DMA_FROM_DEVICE);
-			if (dma_mapping_error(hwdev,
-					      rp->rx_skbuff_dma[entry])) {
-				dev_kfree_skb(skb);
-				rp->rx_skbuff_dma[entry] = 0;
-				break;
-			}
-			rp->rx_ring[entry].addr = cpu_to_le32(rp->rx_skbuff_dma[entry]);
-			dma_wmb();
-		}
-		rp->rx_ring[entry].rx_status = cpu_to_le32(DescOwn);
-	}
-
 	return count;
+
+drop:
+	dev->stats.rx_dropped++;
+	goto give_descriptor_to_nic;
 }
 
 static void rhine_restart_tx(struct net_device *dev) {

@@ -2518,9 +2522,8 @@ static int rhine_resume(struct device *device)
 	enable_mmio(rp->pioaddr, rp->quirks);
 	rhine_power_init(dev);
 	free_tbufs(dev);
-	free_rbufs(dev);
 	alloc_tbufs(dev);
-	alloc_rbufs(dev);
+	rhine_reset_rbufs(rp);
 	rhine_task_enable(rp);
 	spin_lock_bh(&rp->lock);
 	init_registers(dev);
Signature · 1 line
Ueimor
author
#5 lore raw

PATCH 5/8 via-rhine: kiss rx_head_desc goodbye.

In reply to Francois Romieu

The driver no longer produces holes in its receive ring so rx_head_desc only duplicates cur_rx.

Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)
drivers/net/ethernet/via/via-rhine.c +3 -10
index 8d322bb..fede9fc 100644
@@ -472,7 +472,6 @@ struct rhine_private {
 
 	/* Frequently used values: keep some adjacent for cache effect. */
 	u32 quirks;
-	struct rx_desc *rx_head_desc;
 	unsigned int cur_rx;
 	unsigned int cur_tx, dirty_tx;
 	unsigned int rx_buf_sz;		/* Based on MTU+slack. */

@@ -1244,7 +1243,6 @@ static void rhine_reset_rbufs(struct rhine_private *rp)
 	int i;
 
 	rp->cur_rx = 0;
-	rp->rx_head_desc = rp->rx_ring;
 
 	for (i = 0; i < RX_RING_SIZE; i++)
 		rp->rx_ring[i].rx_status = cpu_to_le32(DescOwn);

@@ -2000,15 +1998,15 @@ static int rhine_rx(struct net_device *dev, int limit)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	struct device *hwdev = dev->dev.parent;
-	int count;
 	int entry = rp->cur_rx % RX_RING_SIZE;
+	int count;
 
 	netif_dbg(rp, rx_status, dev, "%s(), entry %d status %08x\n", __func__,
-		  entry, le32_to_cpu(rp->rx_head_desc->rx_status));
+		  entry, le32_to_cpu(rp->rx_ring[entry].rx_status));
 
 	/* If EOP is set on the next entry, it's a new packet. Send it up. */
 	for (count = 0; count < limit; ++count) {
-		struct rx_desc *desc = rp->rx_head_desc;
+		struct rx_desc *desc = rp->rx_ring + entry;
 		u32 desc_status = le32_to_cpu(desc->rx_status);
 		u32 desc_length = le32_to_cpu(desc->desc_length);
 		int data_size = desc_status >> 16;

@@ -2026,10 +2024,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 	"entry %#x length %d status %08x!\n",
 					    entry, data_size,
 					    desc_status);
-				netdev_warn(dev,
-					    "Oversized Ethernet frame %p vs %p\n",
-					    rp->rx_head_desc,
-					    &rp->rx_ring[entry]);
 				dev->stats.rx_length_errors++;
 			} else if (desc_status & RxErr) {
 				/* There was a error. */

@@ -2110,7 +2104,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 give_descriptor_to_nic:
 		desc->rx_status = cpu_to_le32(DescOwn);
 		entry = (++rp->cur_rx) % RX_RING_SIZE;
-		rp->rx_head_desc = &rp->rx_ring[entry];
 	}
 
 	return count;
Signature · 1 line
Ueimor
author
#6 lore raw

PATCH 6/8 via-rhine: add consistent memory barrier in vlan receive code.

In reply to Francois Romieu

The NAPI receive path depends on desc->rx_status but it does not enforce any explicit receive barrier.

Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)
drivers/net/ethernet/via/via-rhine.c +14 -8
index fede9fc..8195fcd 100644
@@ -1993,6 +1993,18 @@ static inline u16 rhine_get_vlan_tci(struct sk_buff *skb, int data_size)
 	return be16_to_cpup((__be16 *)trailer);
 }
 
+static inline void rhine_rx_vlan_tag(struct sk_buff *skb, struct rx_desc *desc,
+				     int data_size)
+{
+	dma_rmb();
+	if (unlikely(desc->desc_length & cpu_to_le32(DescTag))) {
+		u16 vlan_tci;
+
+		vlan_tci = rhine_get_vlan_tci(skb, data_size);
+		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
+	}
+}
+
 /* Process up to limit frames from receive ring */
 static int rhine_rx(struct net_device *dev, int limit)
 {

@@ -2008,7 +2020,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 	for (count = 0; count < limit; ++count) {
 		struct rx_desc *desc = rp->rx_ring + entry;
 		u32 desc_status = le32_to_cpu(desc->rx_status);
-		u32 desc_length = le32_to_cpu(desc->desc_length);
 		int data_size = desc_status >> 16;
 
 		if (desc_status & DescOwn)

@@ -2048,7 +2059,6 @@ static int rhine_rx(struct net_device *dev, int limit)
 			/* Length should omit the CRC */
 			int pkt_len = data_size - 4;
 			struct sk_buff *skb;
-			u16 vlan_tci = 0;
 
 			/* Check if the packet is long enough to accept without
 			   copying to a minimally-sized skbuff. */

@@ -2086,14 +2096,10 @@ static int rhine_rx(struct net_device *dev, int limit)
 			}
 
 			skb_put(skb, pkt_len);
-
-			if (unlikely(desc_length & DescTag))
-				vlan_tci = rhine_get_vlan_tci(skb, data_size);
-
 			skb->protocol = eth_type_trans(skb, dev);
 
-			if (unlikely(desc_length & DescTag))
-				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
+			rhine_rx_vlan_tag(skb, desc, data_size);
+
 			netif_receive_skb(skb);
 
 			u64_stats_update_begin(&rp->rx_stats.syncp);
Signature · 1 line
Ueimor
author
#7 lore raw

PATCH 7/8 via-rhine: dma_wmb transmit barrier.

In reply to Francois Romieu

Follow the now usual transmit descriptor update path:
1. content change
2. dma_wmb
3. ownership change

Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/via/via-rhine.c +1 -1
index 8195fcd..86ca8b2 100644
@@ -1849,7 +1849,7 @@ static netdev_tx_t rhine_start_tx(struct sk_buff *skb,
 
 	netdev_sent_queue(dev, skb->len);
 	/* lock eth irq */
-	wmb();
+	dma_wmb();
 	rp->tx_ring[entry].tx_status |= cpu_to_le32(DescOwn);
 	wmb();
Signature · 1 line
Ueimor
author
#8 lore raw

PATCH 8/8 via-rhine: close SMP transmit races.

In reply to Francois Romieu

7ab87ff4c770eed71e3777936299292739fcd0fe ("via-rhine: move work from irq handler to softirq and beyond") forgot to explicitely control the lifespan of the tx_dirty and tx_cur pointers.

Signed-off-by Francois Romieu <[email protected]>
---
 drivers/net/ethernet/via/via-rhine.c | 51 +++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 6 deletions(-)
drivers/net/ethernet/via/via-rhine.c +45 -6
index 86ca8b2..725106f 100644
@@ -1779,6 +1779,11 @@ static void rhine_tx_timeout(struct net_device *dev)
 	schedule_work(&rp->reset_task);
 }
 
+static inline bool rhine_tx_queue_full(struct rhine_private *rp)
+{
+	return (rp->cur_tx - rp->dirty_tx) >= TX_QUEUE_LEN;
+}
+
 static netdev_tx_t rhine_start_tx(struct sk_buff *skb,
 				  struct net_device *dev)
 {

@@ -1854,6 +1859,12 @@ static netdev_tx_t rhine_start_tx(struct sk_buff *skb,
 	wmb();
 
 	rp->cur_tx++;
+	/*
+	 * Nobody wants cur_tx write to rot for ages after the NIC will have
+	 * seen the transmit request, especially as the transmit completion
+	 * handler could miss it.
+	 */
+	smp_wmb();
 
 	/* Non-x86 Todo: explicitly flush cache lines here. */
 

@@ -1866,8 +1877,14 @@ static netdev_tx_t rhine_start_tx(struct sk_buff *skb,
 	       ioaddr + ChipCmd1);
 	IOSYNC;
 
-	if (rp->cur_tx == rp->dirty_tx + TX_QUEUE_LEN)
+	/* dirty_tx may be pessimistically out-of-sync. See rhine_tx. */
+	if (rhine_tx_queue_full(rp)) {
 		netif_stop_queue(dev);
+		smp_rmb();
+		/* Rejuvenate. */
+		if (!rhine_tx_queue_full(rp))
+			netif_wake_queue(dev);
+	}
 
 	netif_dbg(rp, tx_queued, dev, "Transmit frame #%d queued in slot %d\n",
 		  rp->cur_tx - 1, entry);

@@ -1915,13 +1932,24 @@ static void rhine_tx(struct net_device *dev)
 {
 	struct rhine_private *rp = netdev_priv(dev);
 	struct device *hwdev = dev->dev.parent;
-	int txstatus = 0, entry = rp->dirty_tx % TX_RING_SIZE;
 	unsigned int pkts_compl = 0, bytes_compl = 0;
+	unsigned int dirty_tx = rp->dirty_tx;
+	unsigned int cur_tx;
 	struct sk_buff *skb;
 
+	/*
+	 * The race with rhine_start_tx does not matter here as long as the
+	 * driver enforces a value of cur_tx that was relevant when the
+	 * packet was scheduled to the network chipset.
+	 * Executive summary: smp_rmb() balances smp_wmb() in rhine_start_tx.
+	 */
+	smp_rmb();
+	cur_tx = rp->cur_tx;
 	/* find and cleanup dirty tx descriptors */
-	while (rp->dirty_tx != rp->cur_tx) {
-		txstatus = le32_to_cpu(rp->tx_ring[entry].tx_status);
+	while (dirty_tx != cur_tx) {
+		unsigned int entry = dirty_tx % TX_RING_SIZE;
+		u32 txstatus = le32_to_cpu(rp->tx_ring[entry].tx_status);
+
 		netif_dbg(rp, tx_done, dev, "Tx scavenge %d status %08x\n",
 			  entry, txstatus);
 		if (txstatus & DescOwn)

@@ -1970,12 +1998,23 @@ static void rhine_tx(struct net_device *dev)
 		pkts_compl++;
 		dev_consume_skb_any(skb);
 		rp->tx_skbuff[entry] = NULL;
-		entry = (++rp->dirty_tx) % TX_RING_SIZE;
+		dirty_tx++;
 	}
 
+	rp->dirty_tx = dirty_tx;
+	/* Pity we can't rely on the nearby BQL completion implicit barrier. */
+	smp_wmb();
+
 	netdev_completed_queue(dev, pkts_compl, bytes_compl);
-	if ((rp->cur_tx - rp->dirty_tx) < TX_QUEUE_LEN - 4)
+
+	/* cur_tx may be optimistically out-of-sync. See rhine_start_tx. */
+	if (!rhine_tx_queue_full(rp) && netif_queue_stopped(dev)) {
 		netif_wake_queue(dev);
+		smp_rmb();
+		/* Rejuvenate. */
+		if (rhine_tx_queue_full(rp))
+			netif_stop_queue(dev);
+	}
 }
 
 /**
Signature · 1 line
Ueimor
maintainer
#9 lore raw
In reply to Francois Romieu

From: Francois Romieu <[email protected]>
Date: Fri, 1 May 2015 22:14:37 +0200

Quoted 6 lines

The series applies against davem-next as of 9dd3c797496affd699805c8a9d8429ad318c892f ("drivers: net: xgene: fix kbuild warnings").

Patches #1..#4 avoid holes in the receive ring.

Quoted 8 lines

Patch #5 is a small leftover cleanup for #1..#4.

Patches #6 and #7 are fairly simple barrier stuff.

Patch #8 closes some SMP transmit races - not that anyone really complained about these but it's a bit hard to handwave that they can be safely ignored. Some testing, especially SMP testing of course, would be welcome.

Looks great, series applied, thanks a lot!