[PATCH 3.16 012/294] net: bcmgenet: rewrite bcmgenet_rx_refill()

[Ben Hutchings]

3.16.50-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Petri Gynther <pgynther@xxxxxxxxxx>

commit d6707bec598649450ee0887bf11896e525777874 upstream.

Currently, bcmgenet_desc_rx() calls bcmgenet_rx_refill() at the end of
Rx packet processing loop, after the current Rx packet has already been
passed to napi_gro_receive(). However, bcmgenet_rx_refill() might fail
to allocate a new Rx skb, thus leaving a hole on the Rx queue where no
valid Rx buffer exists.

To eliminate this situation:
1. Rewrite bcmgenet_rx_refill() to retain the current Rx skb on the Rx
   queue if a new replacement Rx skb can't be allocated and DMA-mapped.
   In this case, the data on the current Rx skb is effectively dropped.
2. Modify bcmgenet_desc_rx() to call bcmgenet_rx_refill() at the top of
   Rx packet processing loop, so that the new replacement Rx skb is
   already in place before the current Rx skb is processed.

Signed-off-by: Petri Gynther <pgynther@xxxxxxxxxx>
Tested-by: Jaedon Shin <jaedon.shin@xxxxxxxxx>--
Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
Tested-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
[bwh: Backported to 3.16:
 - There's no alloc_rx_buff_failed statistic
 - Adjust context]
Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
---
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1275,33 +1275,41 @@ out:
 	return ret;
 }
 
-
-static int bcmgenet_rx_refill(struct bcmgenet_priv *priv,
-				struct enet_cb *cb)
+static struct sk_buff *bcmgenet_rx_refill(struct bcmgenet_priv *priv,
+					  struct enet_cb *cb)
 {
 	struct device *kdev = &priv->pdev->dev;
 	struct sk_buff *skb;
+	struct sk_buff *rx_skb;
 	dma_addr_t mapping;
-	int ret;
 
+	/* Allocate a new Rx skb */
 	skb = netdev_alloc_skb(priv->dev,
 				priv->rx_buf_len + SKB_ALIGNMENT);
-	if (!skb)
-		return -ENOMEM;
+	if (!skb) {
+		netif_err(priv, rx_err, priv->dev,
+			  "%s: Rx skb allocation failed\n", __func__);
+		return NULL;
+	}
 
-	/* a caller did not release this control block */
-	WARN_ON(cb->skb != NULL);
-	cb->skb = skb;
-	mapping = dma_map_single(kdev, skb->data,
-			priv->rx_buf_len, DMA_FROM_DEVICE);
-	ret = dma_mapping_error(kdev, mapping);
-	if (ret) {
-		bcmgenet_free_cb(cb);
+	/* DMA-map the new Rx skb */
+	mapping = dma_map_single(kdev, skb->data, priv->rx_buf_len,
+				 DMA_FROM_DEVICE);
+	if (dma_mapping_error(kdev, mapping)) {
+		dev_kfree_skb_any(skb);
 		netif_err(priv, rx_err, priv->dev,
-				"%s DMA map failed\n", __func__);
-		return ret;
+			  "%s: Rx skb DMA mapping failed\n", __func__);
+		return NULL;
 	}
 
+	/* Grab the current Rx skb from the ring and DMA-unmap it */
+	rx_skb = cb->skb;
+	if (likely(rx_skb))
+		dma_unmap_single(kdev, dma_unmap_addr(cb, dma_addr),
+				 priv->rx_buf_len, DMA_FROM_DEVICE);
+
+	/* Put the new Rx skb on the ring */
+	cb->skb = skb;
 	dma_unmap_addr_set(cb, dma_addr, mapping);
 	/* assign packet, prepare descriptor, and advance pointer */
 
@@ -1314,7 +1322,8 @@ static int bcmgenet_rx_refill(struct bcm
 	priv->rx_bd_assign_ptr = priv->rx_bds +
 		(priv->rx_bd_assign_index * DMA_DESC_SIZE);
 
-	return 0;
+	/* Return the current Rx skb to caller */
+	return rx_skb;
 }
 
 /* bcmgenet_desc_rx - descriptor based rx process.
@@ -1329,7 +1338,7 @@ static unsigned int bcmgenet_desc_rx(str
 	struct sk_buff *skb;
 	u32 dma_length_status;
 	unsigned long dma_flag;
-	int len, err;
+	int len;
 	unsigned int rxpktprocessed = 0, rxpkttoprocess;
 	unsigned int p_index;
 	unsigned int chksum_ok = 0;
@@ -1351,26 +1360,14 @@ static unsigned int bcmgenet_desc_rx(str
 			(rxpktprocessed < budget)) {
 
 		cb = &priv->rx_cbs[priv->rx_read_ptr];
-		skb = cb->skb;
+		skb = bcmgenet_rx_refill(priv, cb);
 
-		/* We do not have a backing SKB, so we do not have a
-		 * corresponding DMA mapping for this incoming packet since
-		 * bcmgenet_rx_refill always either has both skb and mapping or
-		 * none.
-		 */
 		if (unlikely(!skb)) {
 			dev->stats.rx_dropped++;
 			dev->stats.rx_errors++;
-			goto refill;
+			goto next;
 		}
 
-		/* Unmap the packet contents such that we can use the
-		 * RSV from the 64 bytes descriptor when enabled and save
-		 * a 32-bits register read
-		 */
-		dma_unmap_single(kdev, dma_unmap_addr(cb, dma_addr),
-				priv->rx_buf_len, DMA_FROM_DEVICE);
-
 		if (!priv->desc_64b_en) {
 			dma_length_status = dmadesc_get_length_status(priv,
 							priv->rx_bds +
@@ -1398,10 +1395,10 @@ static unsigned int bcmgenet_desc_rx(str
 					"Droping fragmented packet!\n");
 			dev->stats.rx_dropped++;
 			dev->stats.rx_errors++;
-			dev_kfree_skb_any(cb->skb);
-			cb->skb = NULL;
-			goto refill;
+			dev_kfree_skb_any(skb);
+			goto next;
 		}
+
 		/* report errors */
 		if (unlikely(dma_flag & (DMA_RX_CRC_ERROR |
 						DMA_RX_OV |
@@ -1420,11 +1417,8 @@ static unsigned int bcmgenet_desc_rx(str
 				dev->stats.rx_length_errors++;
 			dev->stats.rx_dropped++;
 			dev->stats.rx_errors++;
-
-			/* discard the packet and advance consumer index.*/
-			dev_kfree_skb_any(cb->skb);
-			cb->skb = NULL;
-			goto refill;
+			dev_kfree_skb_any(skb);
+			goto next;
 		} /* error packet */
 
 		chksum_ok = (dma_flag & priv->dma_rx_chk_bit) &&
@@ -1457,15 +1451,9 @@ static unsigned int bcmgenet_desc_rx(str
 
 		/* Notify kernel */
 		napi_gro_receive(&priv->napi, skb);
-		cb->skb = NULL;
 		netif_dbg(priv, rx_status, dev, "pushed up to kernel\n");
 
-		/* refill RX path on the current control block */
-refill:
-		err = bcmgenet_rx_refill(priv, cb);
-		if (err)
-			netif_err(priv, rx_err, dev, "Rx refill failed\n");
-
+next:
 		rxpktprocessed++;
 		priv->rx_read_ptr++;
 		priv->rx_read_ptr &= (priv->num_rx_bds - 1);
@@ -1478,7 +1466,7 @@ refill:
 static int bcmgenet_alloc_rx_buffers(struct bcmgenet_priv *priv)
 {
 	struct enet_cb *cb;
-	int ret = 0;
+	struct sk_buff *skb;
 	int i;
 
 	netif_dbg(priv, hw, priv->dev, "%s:\n", __func__);
@@ -1486,16 +1474,14 @@ static int bcmgenet_alloc_rx_buffers(str
 	/* loop here for each buffer needing assign */
 	for (i = 0; i < priv->num_rx_bds; i++) {
 		cb = &priv->rx_cbs[priv->rx_bd_assign_index];
-		if (cb->skb)
-			continue;
-
-		ret = bcmgenet_rx_refill(priv, cb);
-		if (ret)
-			break;
-
+		skb = bcmgenet_rx_refill(priv, cb);
+		if (skb)
+			dev_kfree_skb_any(skb);
+		if (!cb->skb)
+			return -ENOMEM;
 	}
 
-	return ret;
+	return 0;
 }
 
 static void bcmgenet_free_rx_buffers(struct bcmgenet_priv *priv)