Skip to contentSkip to conversation

net: calxedaxgmac: remove NETIF_F_FRAGLIST setting

PATCH Superseded Rob Herring 3 participants 14 messages Aug 30, 2013

Subsystems (2)
Networking Network drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Rob Herring

author
#0 lore raw

PATCH v2 1/11 net: calxedaxgmac: remove NETIF_F_FRAGLIST setting

From: Rob Herring <[email protected]>

The xgmac does not actually handle frag lists, so this option should not be set. It does not appear to have had any impact though.

Reported-by Lennert Buytenhek <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/calxeda/xgmac.c +1 -1
index 7cb148c..71f6720 100644
@@ -1759,7 +1759,7 @@ static int xgmac_probe(struct platform_device *pdev)
 	if (device_can_wakeup(priv->device))
 		priv->wolopts = WAKE_MAGIC;	/* Magic Frame as default */
 
-	ndev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA;
+	ndev->hw_features = NETIF_F_SG | NETIF_F_HIGHDMA;
 	if (readl(priv->base + XGMAC_DMA_HW_FEATURE) & DMA_HW_FEAT_TXCOESEL)
 		ndev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				     NETIF_F_RXCSUM;
Signature · 1 line
1.8.1.2
author
#1 lore raw

PATCH v2 2/11 net: calxedaxgmac: read correct field in xgmac_desc_get_buf_len

From: Rob Herring <[email protected]>

xgmac_desc_get_buf_len appears to have a copy/paste error. flags is the wrong field to read. We should be reading buf_size field. cpu_to_le32 should also be le32_to_cpu. This never really mattered as this function is only used for DMA mapping calls which happen to be nops with coherent DMA.

Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/calxeda/xgmac.c +1 -1
index 71f6720..d41af68 100644
@@ -421,7 +421,7 @@ static inline void desc_set_buf_len(struct xgmac_dma_desc *p, u32 buf_sz)
 
 static inline int desc_get_buf_len(struct xgmac_dma_desc *p)
 {
-	u32 len = cpu_to_le32(p->flags);
+	u32 len = le32_to_cpu(p->buf_size);
 	return (len & DESC_BUFFER1_SZ_MASK) +
 		((len & DESC_BUFFER2_SZ_MASK) >> DESC_BUFFER2_SZ_OFFSET);
 }
Signature · 1 line
1.8.1.2
author
#2 lore raw

PATCH v2 3/11 net: calxedaxgmac: fix race between xgmac_tx_complete and xgmac_tx_err

In reply to Rob Herring

From: Rob Herring <[email protected]>

It is possible for the xgmac_tx_complete to run concurrently with xgmac_tx_err since there are no locks. Fix this by moving the tx error handling to a workqueue so we can disable napi while we reset the transmitter.

Reported-by Andreas Herrmann <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 38 +++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 20 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +18 -20
index d41af68..df7e3e2 100644
@@ -393,6 +393,7 @@ struct xgmac_priv {
 	char rx_pause;
 	char tx_pause;
 	int wolopts;
+	struct work_struct tx_timeout_work;
 };
 
 /* XGMAC Configuration Settings */

@@ -897,21 +898,18 @@ static void xgmac_tx_complete(struct xgmac_priv *priv)
 		netif_wake_queue(priv->dev);
 }
 
-/**
- * xgmac_tx_err:
- * @priv: pointer to the private device structure
- * Description: it cleans the descriptors and restarts the transmission
- * in case of errors.
- */
-static void xgmac_tx_err(struct xgmac_priv *priv)
+static void xgmac_tx_timeout_work(struct work_struct *work)
 {
-	u32 reg, value, inten;
+	u32 reg, value;
+	struct xgmac_priv *priv =
+		container_of(work, struct xgmac_priv, tx_timeout_work);
 
-	netif_stop_queue(priv->dev);
+	napi_disable(&priv->napi);
 
-	inten = readl(priv->base + XGMAC_DMA_INTR_ENA);
 	writel(0, priv->base + XGMAC_DMA_INTR_ENA);
 
+	netif_tx_lock(priv->dev);
+
 	reg = readl(priv->base + XGMAC_DMA_CONTROL);
 	writel(reg & ~DMA_CONTROL_ST, priv->base + XGMAC_DMA_CONTROL);
 	do {

@@ -927,9 +925,15 @@ static void xgmac_tx_err(struct xgmac_priv *priv)
 
 	writel(DMA_STATUS_TU | DMA_STATUS_TPS | DMA_STATUS_NIS | DMA_STATUS_AIS,
 		priv->base + XGMAC_DMA_STATUS);
-	writel(inten, priv->base + XGMAC_DMA_INTR_ENA);
 
+	netif_tx_unlock(priv->dev);
 	netif_wake_queue(priv->dev);
+
+	napi_enable(&priv->napi);
+
+	/* Enable interrupts */
+	writel(DMA_INTR_DEFAULT_MASK, priv->base + XGMAC_DMA_STATUS);
+	writel(DMA_INTR_DEFAULT_MASK, priv->base + XGMAC_DMA_INTR_ENA);
 }
 
 static int xgmac_hw_init(struct net_device *dev)

@@ -1225,9 +1229,7 @@ static int xgmac_poll(struct napi_struct *napi, int budget)
 static void xgmac_tx_timeout(struct net_device *dev)
 {
 	struct xgmac_priv *priv = netdev_priv(dev);
-
-	/* Clear Tx resources and restart transmitting again */
-	xgmac_tx_err(priv);
+	schedule_work(&priv->tx_timeout_work);
 }
 
 /**

@@ -1366,7 +1368,6 @@ static irqreturn_t xgmac_pmt_interrupt(int irq, void *dev_id)
 static irqreturn_t xgmac_interrupt(int irq, void *dev_id)
 {
 	u32 intr_status;
-	bool tx_err = false;
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct xgmac_priv *priv = netdev_priv(dev);
 	struct xgmac_extra_stats *x = &priv->xstats;

@@ -1396,16 +1397,12 @@ static irqreturn_t xgmac_interrupt(int irq, void *dev_id)
 		if (intr_status & DMA_STATUS_TPS) {
 			netdev_err(priv->dev, "transmit process stopped\n");
 			x->tx_process_stopped++;
-			tx_err = true;
+			schedule_work(&priv->tx_timeout_work);
 		}
 		if (intr_status & DMA_STATUS_FBI) {
 			netdev_err(priv->dev, "fatal bus error\n");
 			x->fatal_bus_error++;
-			tx_err = true;
 		}
-
-		if (tx_err)
-			xgmac_tx_err(priv);
 	}
 
 	/* TX/RX NORMAL interrupts */

@@ -1708,6 +1705,7 @@ static int xgmac_probe(struct platform_device *pdev)
 	ndev->netdev_ops = &xgmac_netdev_ops;
 	SET_ETHTOOL_OPS(ndev, &xgmac_ethtool_ops);
 	spin_lock_init(&priv->stats_lock);
+	INIT_WORK(&priv->tx_timeout_work, xgmac_tx_timeout_work);
 
 	priv->device = &pdev->dev;
 	priv->dev = ndev;
Signature · 1 line
1.8.1.2
author
#3 lore raw

PATCH v2 4/11 net: calxedaxgmac: fix possible skb free before tx complete

In reply to Rob Herring

From: Rob Herring <[email protected]>

The TX completion code may have freed an skb before the entire sg list was transmitted. The DMA unmap calls for the fragments could also get skipped. Now set the skb pointer on every entry in the ring, not just the head of the sg list. We then use the FS (first segment) bit in the descriptors to determine skb head vs. fragment.

This also fixes similar bug in xgmac_free_tx_skbufs where clean-up of a sg list that wraps at the end of the ring buffer would not get unmapped.

Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 55 ++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 31 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +24 -31
index df7e3e2..64854ad 100644
@@ -470,6 +470,11 @@ static inline int desc_get_tx_ls(struct xgmac_dma_desc *p)
 	return le32_to_cpu(p->flags) & TXDESC_LAST_SEG;
 }
 
+static inline int desc_get_tx_fs(struct xgmac_dma_desc *p)
+{
+	return le32_to_cpu(p->flags) & TXDESC_FIRST_SEG;
+}
+
 static inline u32 desc_get_buf_addr(struct xgmac_dma_desc *p)
 {
 	return le32_to_cpu(p->buf1_addr);

@@ -796,7 +801,7 @@ static void xgmac_free_rx_skbufs(struct xgmac_priv *priv)
 
 static void xgmac_free_tx_skbufs(struct xgmac_priv *priv)
 {
-	int i, f;
+	int i;
 	struct xgmac_dma_desc *p;
 
 	if (!priv->tx_skbuff)

@@ -807,16 +812,15 @@ static void xgmac_free_tx_skbufs(struct xgmac_priv *priv)
 			continue;
 
 		p = priv->dma_tx + i;
-		dma_unmap_single(priv->device, desc_get_buf_addr(p),
-				 desc_get_buf_len(p), DMA_TO_DEVICE);
-
-		for (f = 0; f < skb_shinfo(priv->tx_skbuff[i])->nr_frags; f++) {
-			p = priv->dma_tx + i++;
+		if (desc_get_tx_fs(p))
+			dma_unmap_single(priv->device, desc_get_buf_addr(p),
+					 desc_get_buf_len(p), DMA_TO_DEVICE);
+		else
 			dma_unmap_page(priv->device, desc_get_buf_addr(p),
 				       desc_get_buf_len(p), DMA_TO_DEVICE);
-		}
 
-		dev_kfree_skb_any(priv->tx_skbuff[i]);
+		if (desc_get_tx_ls(p))
+			dev_kfree_skb_any(priv->tx_skbuff[i]);
 		priv->tx_skbuff[i] = NULL;
 	}
 }

@@ -853,8 +857,6 @@ static void xgmac_free_dma_desc_rings(struct xgmac_priv *priv)
  */
 static void xgmac_tx_complete(struct xgmac_priv *priv)
 {
-	int i;
-
 	while (dma_ring_cnt(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ)) {
 		unsigned int entry = priv->tx_tail;
 		struct sk_buff *skb = priv->tx_skbuff[entry];

@@ -864,33 +866,24 @@ static void xgmac_tx_complete(struct xgmac_priv *priv)
 		if (desc_get_owner(p))
 			break;
 
-		/* Verify tx error by looking at the last segment */
-		if (desc_get_tx_ls(p))
-			desc_get_tx_status(priv, p);
-
 		netdev_dbg(priv->dev, "tx ring: curr %d, dirty %d\n",
 			priv->tx_head, priv->tx_tail);
 
-		dma_unmap_single(priv->device, desc_get_buf_addr(p),
-				 desc_get_buf_len(p), DMA_TO_DEVICE);
-
-		priv->tx_skbuff[entry] = NULL;
-		priv->tx_tail = dma_ring_incr(entry, DMA_TX_RING_SZ);
-
-		if (!skb) {
-			continue;
-		}
-
-		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
-			entry = priv->tx_tail = dma_ring_incr(priv->tx_tail,
-							      DMA_TX_RING_SZ);
-			p = priv->dma_tx + priv->tx_tail;
-
+		if (desc_get_tx_fs(p))
+			dma_unmap_single(priv->device, desc_get_buf_addr(p),
+					 desc_get_buf_len(p), DMA_TO_DEVICE);
+		else
 			dma_unmap_page(priv->device, desc_get_buf_addr(p),
 				       desc_get_buf_len(p), DMA_TO_DEVICE);
+
+		/* Check tx error on the last segment */
+		if (desc_get_tx_ls(p)) {
+			desc_get_tx_status(priv, p);
+			dev_kfree_skb(skb);
 		}
 
-		dev_kfree_skb(skb);
+		priv->tx_skbuff[entry] = NULL;
+		priv->tx_tail = dma_ring_incr(entry, DMA_TX_RING_SZ);
 	}
 
 	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) >

@@ -1110,7 +1103,7 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		entry = dma_ring_incr(entry, DMA_TX_RING_SZ);
 		desc = priv->dma_tx + entry;
-		priv->tx_skbuff[entry] = NULL;
+		priv->tx_skbuff[entry] = skb;
 
 		desc_set_buf_addr_and_size(desc, paddr, len);
 		if (i < (nfrags - 1))
Signature · 1 line
1.8.1.2
author
#4 lore raw

PATCH v2 5/11 net: calxedaxgmac: update ring buffer tx_head after barriers

In reply to Rob Herring

From: Rob Herring <[email protected]>

Ensure that the descriptor writes are visible before the ring buffer head is updated. Since writel is a barrier, we can simply update the head after the writel.

Reported-by Lennert Buytenhek <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/calxeda/xgmac.c +2 -1
index 64854ad..f630855 100644
@@ -1121,9 +1121,10 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	wmb();
 	desc_set_tx_owner(first, desc_flags | TXDESC_FIRST_SEG);
 
+	writel(1, priv->base + XGMAC_DMA_TX_POLL);
+
 	priv->tx_head = dma_ring_incr(entry, DMA_TX_RING_SZ);
 
-	writel(1, priv->base + XGMAC_DMA_TX_POLL);
 	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) <
 	    MAX_SKB_FRAGS)
 		netif_stop_queue(dev);
Signature · 1 line
1.8.1.2
author
#5 lore raw

PATCH v2 6/11 net: calxedaxgmac: fix race with tx queue stop/wake

In reply to Rob Herring

From: Rob Herring <[email protected]>

Since the xgmac transmit start and completion work locklessly, it is possible for xgmac_xmit to stop the tx queue after the xgmac_tx_complete has run resulting in the tx queue never being woken up. Fix this by ensuring that ring buffer index updates are visible and recheck the ring space after stopping the queue.

The implementation used here was copied from
drivers/net/ethernet/broadcom/tg3.c.

Signed-off-by Rob Herring <[email protected]>

---
v2:
- drop netif_tx_lock
- use netif_start_queue instead of netif_wake_queue

 drivers/net/ethernet/calxeda/xgmac.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +15 -5
index f630855..54be55b 100644
@@ -410,6 +410,9 @@ struct xgmac_priv {
 #define dma_ring_space(h, t, s)	CIRC_SPACE(h, t, s)
 #define dma_ring_cnt(h, t, s)	CIRC_CNT(h, t, s)
 
+#define tx_dma_ring_space(p) \
+	dma_ring_space((p)->tx_head, (p)->tx_tail, DMA_TX_RING_SZ)
+
 /* XGMAC Descriptor Access Helpers */
 static inline void desc_set_buf_len(struct xgmac_dma_desc *p, u32 buf_sz)
 {

@@ -886,8 +889,10 @@ static void xgmac_tx_complete(struct xgmac_priv *priv)
 		priv->tx_tail = dma_ring_incr(entry, DMA_TX_RING_SZ);
 	}
 
-	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) >
-	    MAX_SKB_FRAGS)
+	/* Ensure tx_tail is visible to xgmac_xmit */
+	smp_mb();
+	if (unlikely(netif_queue_stopped(priv->dev) &&
+	    (tx_dma_ring_space(priv) > MAX_SKB_FRAGS)))
 		netif_wake_queue(priv->dev);
 }
 

@@ -1125,10 +1130,15 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	priv->tx_head = dma_ring_incr(entry, DMA_TX_RING_SZ);
 
-	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) <
-	    MAX_SKB_FRAGS)
+	/* Ensure tx_head update is visible to tx completion */
+	smp_mb();
+	if (unlikely(tx_dma_ring_space(priv) < MAX_SKB_FRAGS)) {
 		netif_stop_queue(dev);
-
+		/* Ensure netif_stop_queue is visible to tx completion */
+		smp_mb();
+		if (tx_dma_ring_space(priv) > MAX_SKB_FRAGS)
+			netif_start_queue(dev);
+	}
 	return NETDEV_TX_OK;
 }
Signature · 1 line
1.8.1.2
#6 lore raw

Re: [PATCH v2 06/11] net: calxedaxgmac: fix race with tx queue stop/wake

Quoted 16 lines · Rob Herring

From: Rob Herring <[email protected]>

Since the xgmac transmit start and completion work locklessly, it is possible for xgmac_xmit to stop the tx queue after the xgmac_tx_complete has run resulting in the tx queue never being woken up. Fix this by ensuring that ring buffer index updates are visible and recheck the ring space after stopping the queue.

The implementation used here was copied from
drivers/net/ethernet/broadcom/tg3.c.

Signed-off-by Rob Herring <[email protected]>

---
v2:
- drop netif_tx_lock
- use netif_start_queue instead of netif_wake_queue

[...]

drivers/net/ethernet/calxeda/xgmac.c static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev) · line 1135

Quoted 9 lines
drivers/net/ethernet/calxeda/xgmac.c +3 -2
@@ -1125,10 +1130,15 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	priv->tx_head = dma_ring_incr(entry, DMA_TX_RING_SZ);
 
-	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) <
-	    MAX_SKB_FRAGS)
+	/* Ensure tx_head update is visible to tx completion */
+	smp_mb();
+	if (unlikely(tx_dma_ring_space(priv) < MAX_SKB_FRAGS)) {

[...]

On re-reading, I see there's an off-by-one bug lingering here. You use up to 1 + MAX_SKB_FRAGS descriptors per skb. So you must stop the queue when the ring space is <= MAX_SKB_FRAGS.

Ben.

Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
author
#7 lore raw

PATCH v3 6/11 net: calxedaxgmac: fix race with tx queue stop/wake

In reply to Rob Herring

From: Rob Herring <[email protected]>

Since the xgmac transmit start and completion work locklessly, it is possible for xgmac_xmit to stop the tx queue after the xgmac_tx_complete has run resulting in the tx queue never being woken up. Fix this by ensuring that ring buffer index updates are visible and recheck the ring space after stopping the queue. Also fix an off-by-one bug where we need to stop the queue when the ring buffer space is equal to MAX_SKB_FRAGS.

The implementation used here was copied from
drivers/net/ethernet/broadcom/tg3.c.

Signed-off-by Rob Herring <[email protected]>

---
v3:
- Fix off-by-one bug in queue stopping

v2:
- drop netif_tx_lock
- use netif_start_queue instead of netif_wake_queue

 drivers/net/ethernet/calxeda/xgmac.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +15 -5
index f630855..5d0b61a 100644
@@ -410,6 +410,9 @@ struct xgmac_priv {
 #define dma_ring_space(h, t, s)	CIRC_SPACE(h, t, s)
 #define dma_ring_cnt(h, t, s)	CIRC_CNT(h, t, s)
 
+#define tx_dma_ring_space(p) \
+	dma_ring_space((p)->tx_head, (p)->tx_tail, DMA_TX_RING_SZ)
+
 /* XGMAC Descriptor Access Helpers */
 static inline void desc_set_buf_len(struct xgmac_dma_desc *p, u32 buf_sz)
 {

@@ -886,8 +889,10 @@ static void xgmac_tx_complete(struct xgmac_priv *priv)
 		priv->tx_tail = dma_ring_incr(entry, DMA_TX_RING_SZ);
 	}
 
-	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) >
-	    MAX_SKB_FRAGS)
+	/* Ensure tx_tail is visible to xgmac_xmit */
+	smp_mb();
+	if (unlikely(netif_queue_stopped(priv->dev) &&
+	    (tx_dma_ring_space(priv) > MAX_SKB_FRAGS)))
 		netif_wake_queue(priv->dev);
 }
 

@@ -1125,10 +1130,15 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	priv->tx_head = dma_ring_incr(entry, DMA_TX_RING_SZ);
 
-	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) <
-	    MAX_SKB_FRAGS)
+	/* Ensure tx_head update is visible to tx completion */
+	smp_mb();
+	if (unlikely(tx_dma_ring_space(priv) <= MAX_SKB_FRAGS)) {
 		netif_stop_queue(dev);
-
+		/* Ensure netif_stop_queue is visible to tx completion */
+		smp_mb();
+		if (tx_dma_ring_space(priv) > MAX_SKB_FRAGS)
+			netif_start_queue(dev);
+	}
 	return NETDEV_TX_OK;
 }
Signature · 1 line
1.8.1.2
maintainer
#8 lore raw

Re: [PATCH v3 06/11] net: calxedaxgmac: fix race with tx queue stop/wake

When you post a new version of a patch, you must repost the entire series, not just the patch which is changing.

Thank you.

author
#9 lore raw

PATCH v2 7/11 net: calxedaxgmac: enable interrupts after napi_enable

In reply to Rob Herring

From: Rob Herring <[email protected]>

Fix a race condition where the interrupt handler may have called napi_schedule before napi_enable is called. This would disable interrupts and never actually schedule napi to run.

Reported-by Lennert Buytenhek <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +5 -3
index 54be55b..530b594 100644
@@ -959,9 +959,7 @@ static int xgmac_hw_init(struct net_device *dev)
 		DMA_BUS_MODE_FB | DMA_BUS_MODE_ATDS | DMA_BUS_MODE_AAL;
 	writel(value, ioaddr + XGMAC_DMA_BUS_MODE);
 
-	/* Enable interrupts */
-	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_STATUS);
-	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_INTR_ENA);
+	writel(0, ioaddr + XGMAC_DMA_INTR_ENA);
 
 	/* Mask power mgt interrupt */
 	writel(XGMAC_INT_STAT_PMTIM, ioaddr + XGMAC_INT_STAT);

@@ -1029,6 +1027,10 @@ static int xgmac_open(struct net_device *dev)
 	napi_enable(&priv->napi);
 	netif_start_queue(dev);
 
+	/* Enable interrupts */
+	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_STATUS);
+	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_INTR_ENA);
+
 	return 0;
 }
Signature · 1 line
1.8.1.2
author
#10 lore raw

PATCH v2 8/11 net: calxedaxgmac: fix various errors in xgmac_set_rx_mode

In reply to Rob Herring

From: Rob Herring <[email protected]>

Fix xgmac_set_rx_mode to handle several conditions that were not handled correctly as Lennert Buytenhek describes:

If we have, say, 100 unicast addresses, and 5 multicast addresses, the unicast address count check will evaluate to true, and set use_hash to true. The multicast address check will however evaluate to false, and use_hash won't be set to true again, and XGMAC_FRAME_FILTER_HMC won't be OR'd into XGMAC_FRAME_FILTER, but since use_hash was still true from the unicast check, netdev_for_each_mc_addr() will program the multicast addresses into the hash table instead of using the MAC address registers, but since the HMC bit wasn't set, the hash table won't be checked for multicast addresses on receive, and we'll stop receiving multicast packets entirely.

Also, there is no code that zeroes out MAC address registers reg..31 at the end of this function, meaning that under the right conditions, unicast/multicast addresses that were previously in the filter but were then deleted won't be cleared out.

Reported-by Lennert Buytenhek <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +13 -4
index 530b594..a7cb18d 100644
@@ -618,10 +618,15 @@ static void xgmac_set_mac_addr(void __iomem *ioaddr, unsigned char *addr,
 {
 	u32 data;
 
-	data = (addr[5] << 8) | addr[4] | (num ? XGMAC_ADDR_AE : 0);
-	writel(data, ioaddr + XGMAC_ADDR_HIGH(num));
-	data = (addr[3] << 24) | (addr[2] << 16) | (addr[1] << 8) | addr[0];
-	writel(data, ioaddr + XGMAC_ADDR_LOW(num));
+	if (addr) {
+		data = (addr[5] << 8) | addr[4] | (num ? XGMAC_ADDR_AE : 0);
+		writel(data, ioaddr + XGMAC_ADDR_HIGH(num));
+		data = (addr[3] << 24) | (addr[2] << 16) | (addr[1] << 8) | addr[0];
+		writel(data, ioaddr + XGMAC_ADDR_LOW(num));
+	} else {
+		writel(0, ioaddr + XGMAC_ADDR_HIGH(num));
+		writel(0, ioaddr + XGMAC_ADDR_LOW(num));
+	}
 }
 
 static void xgmac_get_mac_addr(void __iomem *ioaddr, unsigned char *addr,

@@ -1294,6 +1299,8 @@ static void xgmac_set_rx_mode(struct net_device *dev)
 	if ((netdev_mc_count(dev) + reg - 1) > XGMAC_MAX_FILTER_ADDR) {
 		use_hash = true;
 		value |= XGMAC_FRAME_FILTER_HMC | XGMAC_FRAME_FILTER_HPF;
+	} else {
+		use_hash = false;
 	}
 	netdev_for_each_mc_addr(ha, dev) {
 		if (use_hash) {

@@ -1310,6 +1317,8 @@ static void xgmac_set_rx_mode(struct net_device *dev)
 	}
 
 out:
+	for (i = reg; i < XGMAC_MAX_FILTER_ADDR; i++)
+		xgmac_set_mac_addr(ioaddr, NULL, reg);
 	for (i = 0; i < XGMAC_NUM_HASH; i++)
 		writel(hash_filter[i], ioaddr + XGMAC_HASH(i));
Signature · 1 line
1.8.1.2
author
#11 lore raw

PATCH v2 9/11 net: calxedaxgmac: remove some unused statistic counters

In reply to Rob Herring

From: Rob Herring <[email protected]>

rx_sa_filter_fail and tx_undeflow events are unused and impossible to occur based on how the h/w is used. We never filter on source MAC address and TX store and forward mode prevents underflow events.

Reported-by Lennert Buytenhek <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +0 -3
index a7cb18d..4ecf937 100644
@@ -353,11 +353,9 @@ struct xgmac_extra_stats {
 	/* Receive errors */
 	unsigned long rx_watchdog;
 	unsigned long rx_da_filter_fail;
-	unsigned long rx_sa_filter_fail;
 	unsigned long rx_payload_error;
 	unsigned long rx_ip_header_error;
 	/* Tx/Rx IRQ errors */
-	unsigned long tx_undeflow;
 	unsigned long tx_process_stopped;
 	unsigned long rx_buf_unav;
 	unsigned long rx_process_stopped;

@@ -1581,7 +1579,6 @@ static const struct xgmac_stats xgmac_gstrings_stats[] = {
 	XGMAC_STAT(rx_payload_error),
 	XGMAC_STAT(rx_ip_header_error),
 	XGMAC_STAT(rx_da_filter_fail),
-	XGMAC_STAT(rx_sa_filter_fail),
 	XGMAC_STAT(fatal_bus_error),
 	XGMAC_HW_STAT(rx_watchdog, XGMAC_MMC_RXWATCHDOG),
 	XGMAC_HW_STAT(tx_vlan, XGMAC_MMC_TXVLANFRAME),
Signature · 1 line
1.8.1.2
author
#12 lore raw

PATCH v2 10/11 net: calxedaxgmac: fix rx DMA mapping API size mismatches

In reply to Rob Herring

From: Rob Herring <[email protected]>

Fix the mismatch in the DMA mapping and unmapping sizes for receive. The unmap size must be equal to the map size and should not be the actual received frame length. The map size should also be adjusted by the NET_IP_ALIGN size since the h/w buffer size (dma_buf_sz) includes this offset.

Also, add a missing dma_mapping_error check in xgmac_rx_refill.

Reported-by Lennert Buytenhek <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +12 -6
index 4ecf937..1af20ce 100644
@@ -695,9 +695,14 @@ static void xgmac_rx_refill(struct xgmac_priv *priv)
 			if (unlikely(skb == NULL))
 				break;
 
-			priv->rx_skbuff[entry] = skb;
 			paddr = dma_map_single(priv->device, skb->data,
-					       bufsz, DMA_FROM_DEVICE);
+					       priv->dma_buf_sz - NET_IP_ALIGN,
+					       DMA_FROM_DEVICE);
+			if (dma_mapping_error(priv->device, paddr)) {
+				dev_kfree_skb_any(skb);
+				break;
+			}
+			priv->rx_skbuff[entry] = skb;
 			desc_set_buf_addr(p, paddr, priv->dma_buf_sz);
 		}
 

@@ -794,13 +799,14 @@ static void xgmac_free_rx_skbufs(struct xgmac_priv *priv)
 		return;
 
 	for (i = 0; i < DMA_RX_RING_SZ; i++) {
-		if (priv->rx_skbuff[i] == NULL)
+		struct sk_buff *skb = priv->rx_skbuff[i];
+		if (skb == NULL)
 			continue;
 
 		p = priv->dma_rx + i;
 		dma_unmap_single(priv->device, desc_get_buf_addr(p),
-				 priv->dma_buf_sz, DMA_FROM_DEVICE);
-		dev_kfree_skb_any(priv->rx_skbuff[i]);
+				 priv->dma_buf_sz - NET_IP_ALIGN, DMA_FROM_DEVICE);
+		dev_kfree_skb_any(skb);
 		priv->rx_skbuff[i] = NULL;
 	}
 }

@@ -1187,7 +1193,7 @@ static int xgmac_rx(struct xgmac_priv *priv, int limit)
 
 		skb_put(skb, frame_len);
 		dma_unmap_single(priv->device, desc_get_buf_addr(p),
-				 frame_len, DMA_FROM_DEVICE);
+				 priv->dma_buf_sz - NET_IP_ALIGN, DMA_FROM_DEVICE);
 
 		skb->protocol = eth_type_trans(skb, priv->dev);
 		skb->ip_summed = ip_checksum;
Signature · 1 line
1.8.1.2
author
#13 lore raw

PATCH v2 11/11 net: calxedaxgmac: fix xgmac_xmit DMA mapping error handling

In reply to Rob Herring

From: Rob Herring <[email protected]>

On a DMA mapping error in xgmac_xmit, we should simply free the skb and return NETDEV_TX_OK rather than -EIO. In the case of errors in mapping frags, we need to undo everything that has been setup.

Reported-by Andreas Herrmann <[email protected]>
Signed-off-by Rob Herring <[email protected]>

---
v2: No change

 drivers/net/ethernet/calxeda/xgmac.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
drivers/net/ethernet/calxeda/xgmac.c +26 -5
index 1af20ce..d18b7ac 100644
@@ -466,6 +466,13 @@ static inline void desc_set_tx_owner(struct xgmac_dma_desc *p, u32 flags)
 	p->flags = cpu_to_le32(tmpflags);
 }
 
+static inline void desc_clear_tx_owner(struct xgmac_dma_desc *p)
+{
+	u32 tmpflags = le32_to_cpu(p->flags);
+	tmpflags &= TXDESC_END_RING;
+	p->flags = cpu_to_le32(tmpflags);
+}
+
 static inline int desc_get_tx_ls(struct xgmac_dma_desc *p)
 {
 	return le32_to_cpu(p->flags) & TXDESC_LAST_SEG;

@@ -1100,7 +1107,7 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	paddr = dma_map_single(priv->device, skb->data, len, DMA_TO_DEVICE);
 	if (dma_mapping_error(priv->device, paddr)) {
 		dev_kfree_skb(skb);
-		return -EIO;
+		return NETDEV_TX_OK;
 	}
 	priv->tx_skbuff[entry] = skb;
 	desc_set_buf_addr_and_size(desc, paddr, len);

@@ -1112,10 +1119,8 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		paddr = skb_frag_dma_map(priv->device, frag, 0, len,
 					 DMA_TO_DEVICE);
-		if (dma_mapping_error(priv->device, paddr)) {
-			dev_kfree_skb(skb);
-			return -EIO;
-		}
+		if (dma_mapping_error(priv->device, paddr))
+			goto dma_err;
 
 		entry = dma_ring_incr(entry, DMA_TX_RING_SZ);
 		desc = priv->dma_tx + entry;

@@ -1151,6 +1156,22 @@ static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 			netif_start_queue(dev);
 	}
 	return NETDEV_TX_OK;
+
+dma_err:
+	entry = priv->tx_head;
+	for ( ; i > 0; i--) {
+		entry = dma_ring_incr(entry, DMA_TX_RING_SZ);
+		desc = priv->dma_tx + entry;
+		priv->tx_skbuff[entry] = NULL;
+		dma_unmap_page(priv->device, desc_get_buf_addr(desc),
+			       desc_get_buf_len(desc), DMA_TO_DEVICE);
+		desc_clear_tx_owner(desc);
+	}
+	desc = first;
+	dma_unmap_single(priv->device, desc_get_buf_addr(desc),
+			 desc_get_buf_len(desc), DMA_TO_DEVICE);
+	dev_kfree_skb(skb);
+	return NETDEV_TX_OK;
 }
 
 static int xgmac_rx(struct xgmac_priv *priv, int limit)
Signature · 1 line
1.8.1.2