[PATCH net-next 0/4] net: bcmgenet: switch to TX NAPI

[PATCH net-next 0/4] net: bcmgenet: switch to TX NAPI

[Florian Fainelli]

Hi David, Petri,

This patch set updates the GENET driver to switch the transmit to NAPI instead
of using a mix of hard interrupt + RX NAPI reclaim.

Florian Fainelli (4):
  net: bcmgenet: consistently use UMAC_IRQ_RXDMA_MASK
  net: bcmgenet: return number of packets completed in TX reclaim
  net: bcmgenet: reclaim transmitted buffers in NAPI context
  net: bcmgenet: update ring producer index and buffer count in xmit

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 71 ++++++++++++--------------
 1 file changed, 32 insertions(+), 39 deletions(-)

-- 
1.9.1

[PATCH net-next 1/4] net: bcmgenet: consistently use UMAC_IRQ_RXDMA_MASK

[Florian Fainelli]

Define a constant which sets all RXDMA interrupts bits, and use it
consistently throughout the driver to check for RX DMA interrupt bits.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index fdc9ec09e453..ee4d5baf09b6 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -47,6 +47,10 @@
 
 #include "bcmgenet.h"
 
+#define UMAC_IRQ_RXDMA_MASK	(UMAC_IRQ_RXDMA_BDONE | UMAC_IRQ_RXDMA_PDONE)
+#define UMAC_IRQ_TXDMA_MASK	(UMAC_IRQ_TXDMA_BDONE | UMAC_IRQ_TXDMA_PDONE)
+#define UMAC_IRQ_RX_TX_MASK	(UMAC_IRQ_RXDMA_MASK | UMAC_IRQ_TXDMA_MASK)
+
 /* Maximum number of hardware queues, downsized if needed */
 #define GENET_MAX_MQ_CNT	4
 
@@ -1543,9 +1547,9 @@ static int init_umac(struct bcmgenet_priv *priv)
 
 	bcmgenet_intr_disable(priv);
 
-	cpu_mask_clear = UMAC_IRQ_RXDMA_BDONE;
+	cpu_mask_clear = UMAC_IRQ_RXDMA_MASK;
 
-	dev_dbg(kdev, "%s:Enabling RXDMA_BDONE interrupt\n", __func__);
+	dev_dbg(kdev, "%s:Enabling RXDMA interrupts\n", __func__);
 
 	/* Monitor cable plug/unplugged event for internal PHY */
 	if (phy_is_internal(priv->phydev)) {
@@ -1883,7 +1887,7 @@ static int bcmgenet_poll(struct napi_struct *napi, int budget)
 				  priv->rx_c_index, RDMA_CONS_INDEX);
 	if (work_done < budget) {
 		napi_complete(napi);
-		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_BDONE,
+		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
 					 INTRL2_CPU_MASK_CLEAR);
 	}
 
@@ -1958,13 +1962,13 @@ static irqreturn_t bcmgenet_isr0(int irq, void *dev_id)
 	netif_dbg(priv, intr, priv->dev,
 		  "IRQ=0x%x\n", priv->irq0_stat);
 
-	if (priv->irq0_stat & (UMAC_IRQ_RXDMA_BDONE | UMAC_IRQ_RXDMA_PDONE)) {
+	if (priv->irq0_stat & UMAC_IRQ_RXDMA_MASK) {
 		/* We use NAPI(software interrupt throttling, if
 		 * Rx Descriptor throttling is not used.
 		 * Disable interrupt, will be enabled in the poll method.
 		 */
 		if (likely(napi_schedule_prep(&priv->napi))) {
-			bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_BDONE,
+			bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
 						 INTRL2_CPU_MASK_SET);
 			__napi_schedule(&priv->napi);
 		}
-- 
1.9.1

[PATCH net-next 2/4] net: bcmgenet: return number of packets completed in TX reclaim

[Florian Fainelli]

In preparation for reclaiming transmitted buffers in NAPI context,
update __bcmgenet_tx_reclaim() and bcmgenet_tx_reclaim() to return the
number of packets completed per call.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index ee4d5baf09b6..70f2fb366375 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -876,14 +876,14 @@ static inline void bcmgenet_tx_ring_int_disable(struct bcmgenet_priv *priv,
 }
 
 /* Unlocked version of the reclaim routine */
-static void __bcmgenet_tx_reclaim(struct net_device *dev,
-				  struct bcmgenet_tx_ring *ring)
+static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
+					  struct bcmgenet_tx_ring *ring)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	int last_tx_cn, last_c_index, num_tx_bds;
 	struct enet_cb *tx_cb_ptr;
 	struct netdev_queue *txq;
-	unsigned int bds_compl;
+	unsigned int bds_compl, pkts_compl = 0;
 	unsigned int c_index;
 
 	/* Compute how many buffers are transmitted since last xmit call */
@@ -928,6 +928,7 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
 		}
 		dev->stats.tx_packets++;
 		ring->free_bds += bds_compl;
+		pkts_compl += bds_compl;
 
 		last_c_index++;
 		last_c_index &= (num_tx_bds - 1);
@@ -936,20 +937,25 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
 	if (ring->free_bds > (MAX_SKB_FRAGS + 1))
 		ring->int_disable(priv, ring);
 
-	if (netif_tx_queue_stopped(txq))
+	if (netif_tx_queue_stopped(txq) && pkts_compl)
 		netif_tx_wake_queue(txq);
 
 	ring->c_index = c_index;
+
+	return pkts_compl;
 }
 
-static void bcmgenet_tx_reclaim(struct net_device *dev,
-				struct bcmgenet_tx_ring *ring)
+static unsigned int bcmgenet_tx_reclaim(struct net_device *dev,
+					struct bcmgenet_tx_ring *ring)
 {
 	unsigned long flags;
+	unsigned int pkts_compl;
 
 	spin_lock_irqsave(&ring->lock, flags);
-	__bcmgenet_tx_reclaim(dev, ring);
+	pkts_compl = __bcmgenet_tx_reclaim(dev, ring);
 	spin_unlock_irqrestore(&ring->lock, flags);
+
+	return pkts_compl;
 }
 
 static void bcmgenet_tx_reclaim_all(struct net_device *dev)
-- 
1.9.1

[PATCH net-next 3/4] net: bcmgenet: reclaim transmitted buffers in NAPI context

[Florian Fainelli]

The GENET driver is currently reclaiming transmitted buffers from hard
interrupt context in bcmgenet_isr0 as well as NAPI context in
bcmgenet_poll, which is not consistent and not ideal. Instead, update
the driver to reclaim transmitted buffers in NAPI context only and
properly switch the TX path to use interrupt mitigation based on NAPI.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 29 +++++++++-----------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 70f2fb366375..d6f4a7ace05e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -934,9 +934,6 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 		last_c_index &= (num_tx_bds - 1);
 	}
 
-	if (ring->free_bds > (MAX_SKB_FRAGS + 1))
-		ring->int_disable(priv, ring);
-
 	if (netif_tx_queue_stopped(txq) && pkts_compl)
 		netif_tx_wake_queue(txq);
 
@@ -1211,10 +1208,8 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	bcmgenet_tdma_ring_writel(priv, ring->index,
 				  ring->prod_index, TDMA_PROD_INDEX);
 
-	if (ring->free_bds <= (MAX_SKB_FRAGS + 1)) {
+	if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
 		netif_tx_stop_queue(txq);
-		ring->int_enable(priv, ring);
-	}
 
 out:
 	spin_unlock_irqrestore(&ring->lock, flags);
@@ -1553,9 +1548,9 @@ static int init_umac(struct bcmgenet_priv *priv)
 
 	bcmgenet_intr_disable(priv);
 
-	cpu_mask_clear = UMAC_IRQ_RXDMA_MASK;
+	cpu_mask_clear = UMAC_IRQ_RX_TX_MASK;
 
-	dev_dbg(kdev, "%s:Enabling RXDMA interrupts\n", __func__);
+	dev_dbg(kdev, "%s:Enabling RXDMA & TXDMA interrupts\n", __func__);
 
 	/* Monitor cable plug/unplugged event for internal PHY */
 	if (phy_is_internal(priv->phydev)) {
@@ -1879,10 +1874,10 @@ static int bcmgenet_poll(struct napi_struct *napi, int budget)
 {
 	struct bcmgenet_priv *priv = container_of(napi,
 			struct bcmgenet_priv, napi);
-	unsigned int work_done;
+	unsigned int work_done, tx_work;
 
 	/* tx reclaim */
-	bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
+	tx_work = bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
 
 	work_done = bcmgenet_desc_rx(priv, budget);
 
@@ -1891,9 +1886,9 @@ static int bcmgenet_poll(struct napi_struct *napi, int budget)
 	priv->rx_c_index &= DMA_C_INDEX_MASK;
 	bcmgenet_rdma_ring_writel(priv, DESC_INDEX,
 				  priv->rx_c_index, RDMA_CONS_INDEX);
-	if (work_done < budget) {
+	if (work_done < budget || tx_work == 0) {
 		napi_complete(napi);
-		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
+		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RX_TX_MASK,
 					 INTRL2_CPU_MASK_CLEAR);
 	}
 
@@ -1968,22 +1963,18 @@ static irqreturn_t bcmgenet_isr0(int irq, void *dev_id)
 	netif_dbg(priv, intr, priv->dev,
 		  "IRQ=0x%x\n", priv->irq0_stat);
 
-	if (priv->irq0_stat & UMAC_IRQ_RXDMA_MASK) {
+	if (priv->irq0_stat & UMAC_IRQ_RX_TX_MASK) {
 		/* We use NAPI(software interrupt throttling, if
 		 * Rx Descriptor throttling is not used.
 		 * Disable interrupt, will be enabled in the poll method.
 		 */
 		if (likely(napi_schedule_prep(&priv->napi))) {
-			bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
+			bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RX_TX_MASK,
 						 INTRL2_CPU_MASK_SET);
 			__napi_schedule(&priv->napi);
 		}
 	}
-	if (priv->irq0_stat &
-			(UMAC_IRQ_TXDMA_BDONE | UMAC_IRQ_TXDMA_PDONE)) {
-		/* Tx reclaim */
-		bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
-	}
+
 	if (priv->irq0_stat & (UMAC_IRQ_PHY_DET_R |
 				UMAC_IRQ_PHY_DET_F |
 				UMAC_IRQ_LINK_UP |
-- 
1.9.1

[PATCH net-next 4/4] net: bcmgenet: update ring producer index and buffer count in xmit

[Florian Fainelli]

There is no need to have both bcmgenet_xmit_single() and
bcmgenet_xmit_frag() perform a free_bds decrement and a prod_index
increment by one. In case one of these functions fails to map a SKB or
fragment for transmit, we will return and exit bcmgenet_xmit() with an
error.

We can therefore safely use our local copy of nr_frags to know by how
much we should decrement the number of free buffers available, and by
how much the producer count must be incremented and do this in the tail
of bcmgenet_xmit().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d6f4a7ace05e..3cd8c25a1120 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1012,11 +1012,6 @@ static int bcmgenet_xmit_single(struct net_device *dev,
 
 	dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, length_status);
 
-	/* Decrement total BD count and advance our write pointer */
-	ring->free_bds -= 1;
-	ring->prod_index += 1;
-	ring->prod_index &= DMA_P_INDEX_MASK;
-
 	return 0;
 }
 
@@ -1054,11 +1049,6 @@ static int bcmgenet_xmit_frag(struct net_device *dev,
 		    (frag->size << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
 		    (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT));
 
-
-	ring->free_bds -= 1;
-	ring->prod_index += 1;
-	ring->prod_index &= DMA_P_INDEX_MASK;
-
 	return 0;
 }
 
@@ -1202,9 +1192,11 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	skb_tx_timestamp(skb);
 
-	/* we kept a software copy of how much we should advance the TDMA
-	 * producer index, now write it down to the hardware
-	 */
+	/* Decrement total BD count and advance our write pointer */
+	ring->free_bds -= nr_frags + 1;
+	ring->prod_index += nr_frags + 1;
+	ring->prod_index &= DMA_P_INDEX_MASK;
+
 	bcmgenet_tdma_ring_writel(priv, ring->index,
 				  ring->prod_index, TDMA_PROD_INDEX);
 
-- 
1.9.1

Re: [PATCH net-next 1/4] net: bcmgenet: consistently use UMAC_IRQ_RXDMA_MASK

[Petri Gynther]

Hi Florian,

On Fri, Oct 24, 2014 at 1:02 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Define a constant which sets all RXDMA interrupts bits, and use it
> consistently throughout the driver to check for RX DMA interrupt bits.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index fdc9ec09e453..ee4d5baf09b6 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -47,6 +47,10 @@
>
>  #include "bcmgenet.h"
>
> +#define UMAC_IRQ_RXDMA_MASK    (UMAC_IRQ_RXDMA_BDONE | UMAC_IRQ_RXDMA_PDONE)
> +#define UMAC_IRQ_TXDMA_MASK    (UMAC_IRQ_TXDMA_BDONE | UMAC_IRQ_TXDMA_PDONE)
> +#define UMAC_IRQ_RX_TX_MASK    (UMAC_IRQ_RXDMA_MASK | UMAC_IRQ_TXDMA_MASK)
> +

How about putting these in bcmgenet.h, just below the other UMAC_IRQ_*
definitions?

>  /* Maximum number of hardware queues, downsized if needed */
>  #define GENET_MAX_MQ_CNT       4
>
> @@ -1543,9 +1547,9 @@ static int init_umac(struct bcmgenet_priv *priv)
>
>         bcmgenet_intr_disable(priv);
>
> -       cpu_mask_clear = UMAC_IRQ_RXDMA_BDONE;
> +       cpu_mask_clear = UMAC_IRQ_RXDMA_MASK;
>
> -       dev_dbg(kdev, "%s:Enabling RXDMA_BDONE interrupt\n", __func__);
> +       dev_dbg(kdev, "%s:Enabling RXDMA interrupts\n", __func__);
>
>         /* Monitor cable plug/unplugged event for internal PHY */
>         if (phy_is_internal(priv->phydev)) {
> @@ -1883,7 +1887,7 @@ static int bcmgenet_poll(struct napi_struct *napi, int budget)
>                                   priv->rx_c_index, RDMA_CONS_INDEX);
>         if (work_done < budget) {
>                 napi_complete(napi);
> -               bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_BDONE,
> +               bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
>                                          INTRL2_CPU_MASK_CLEAR);
>         }
>
> @@ -1958,13 +1962,13 @@ static irqreturn_t bcmgenet_isr0(int irq, void *dev_id)
>         netif_dbg(priv, intr, priv->dev,
>                   "IRQ=0x%x\n", priv->irq0_stat);
>
> -       if (priv->irq0_stat & (UMAC_IRQ_RXDMA_BDONE | UMAC_IRQ_RXDMA_PDONE)) {
> +       if (priv->irq0_stat & UMAC_IRQ_RXDMA_MASK) {
>                 /* We use NAPI(software interrupt throttling, if
>                  * Rx Descriptor throttling is not used.
>                  * Disable interrupt, will be enabled in the poll method.
>                  */
>                 if (likely(napi_schedule_prep(&priv->napi))) {
> -                       bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_BDONE,
> +                       bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
>                                                  INTRL2_CPU_MASK_SET);
>                         __napi_schedule(&priv->napi);
>                 }
> --
> 1.9.1
>

Re: [PATCH net-next 2/4] net: bcmgenet: return number of packets completed in TX reclaim

[Petri Gynther]

Hi Florian,

On Fri, Oct 24, 2014 at 1:02 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> In preparation for reclaiming transmitted buffers in NAPI context,
> update __bcmgenet_tx_reclaim() and bcmgenet_tx_reclaim() to return the
> number of packets completed per call.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index ee4d5baf09b6..70f2fb366375 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -876,14 +876,14 @@ static inline void bcmgenet_tx_ring_int_disable(struct bcmgenet_priv *priv,
>  }
>
>  /* Unlocked version of the reclaim routine */
> -static void __bcmgenet_tx_reclaim(struct net_device *dev,
> -                                 struct bcmgenet_tx_ring *ring)
> +static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
> +                                         struct bcmgenet_tx_ring *ring)
>  {
>         struct bcmgenet_priv *priv = netdev_priv(dev);
>         int last_tx_cn, last_c_index, num_tx_bds;
>         struct enet_cb *tx_cb_ptr;
>         struct netdev_queue *txq;
> -       unsigned int bds_compl;
> +       unsigned int bds_compl, pkts_compl = 0;

bcmgenet_desc_rx() uses "rxpktprocessed", so I'd go with "txpktprocessed" here.

>         unsigned int c_index;
>
>         /* Compute how many buffers are transmitted since last xmit call */
> @@ -928,6 +928,7 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
>                 }
>                 dev->stats.tx_packets++;
>                 ring->free_bds += bds_compl;
> +               pkts_compl += bds_compl;

This change doesn't look correct. The number of cleaned packets is not
necessarily equal to the number of cleaned TxBDs.

I think that the while-loop should be:

        while (last_tx_cn-- > 0) {
                tx_cb_ptr = ring->cbs + last_c_index;

                if (tx_cb_ptr->skb) {
                        pkts_compl++;
                        dev->stats.tx_packets++;
                        dev->stats.tx_bytes += tx_cb_ptr->skb->len;
                        dma_unmap_single(&dev->dev,
                                         dma_unmap_addr(tx_cb_ptr, dma_addr),
                                         tx_cb_ptr->skb->len,
                                         DMA_TO_DEVICE);
                        bcmgenet_free_cb(tx_cb_ptr);
                } else if (dma_unmap_addr(tx_cb_ptr, dma_addr)) {
                        dev->stats.tx_bytes +=
                                dma_unmap_len(tx_cb_ptr, dma_len);
                        dma_unmap_page(&dev->dev,
                                       dma_unmap_addr(tx_cb_ptr, dma_addr),
                                       dma_unmap_len(tx_cb_ptr, dma_len),
                                       DMA_TO_DEVICE);
                        dma_unmap_addr_set(tx_cb_ptr, dma_addr, 0);
                }
                ring->free_bds++;
                last_c_index++;
                last_c_index &= (num_tx_bds - 1);
        }

>
>                 last_c_index++;
>                 last_c_index &= (num_tx_bds - 1);
> @@ -936,20 +937,25 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
>         if (ring->free_bds > (MAX_SKB_FRAGS + 1))
>                 ring->int_disable(priv, ring);
>
> -       if (netif_tx_queue_stopped(txq))
> +       if (netif_tx_queue_stopped(txq) && pkts_compl)

bcmgenet_xmit() stops the Tx queue when:
ring->free_bds <= (MAX_SKB_FRAGS + 1)

So, shouldn't this check be:
netif_tx_queue_stopped(txq) && (ring->free_bds > (MAX_SKB_FRAGS + 1))

Having said that --
Why does the driver stop the Tx queue when there are still
(MAX_SKB_FRAGS + 1) TxBDs left?
It leaves 17 or so TxBDs unused on the Tx ring, and most of the
packets are not fragmented.

I feel that bcmgenet_xmit() should stop the Tx queue only when there
is 1 TxBD left after putting a new packet on the ring.

>                 netif_tx_wake_queue(txq);
>
>         ring->c_index = c_index;
> +
> +       return pkts_compl;
>  }
>
> -static void bcmgenet_tx_reclaim(struct net_device *dev,
> -                               struct bcmgenet_tx_ring *ring)
> +static unsigned int bcmgenet_tx_reclaim(struct net_device *dev,
> +                                       struct bcmgenet_tx_ring *ring)
>  {
>         unsigned long flags;
> +       unsigned int pkts_compl;
>
>         spin_lock_irqsave(&ring->lock, flags);
> -       __bcmgenet_tx_reclaim(dev, ring);
> +       pkts_compl = __bcmgenet_tx_reclaim(dev, ring);
>         spin_unlock_irqrestore(&ring->lock, flags);
> +
> +       return pkts_compl;
>  }
>
>  static void bcmgenet_tx_reclaim_all(struct net_device *dev)
> --
> 1.9.1
>

Re: [PATCH net-next 3/4] net: bcmgenet: reclaim transmitted buffers in NAPI context

[Petri Gynther]

Hi Florian,

On Fri, Oct 24, 2014 at 1:02 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> The GENET driver is currently reclaiming transmitted buffers from hard
> interrupt context in bcmgenet_isr0 as well as NAPI context in
> bcmgenet_poll, which is not consistent and not ideal. Instead, update
> the driver to reclaim transmitted buffers in NAPI context only and
> properly switch the TX path to use interrupt mitigation based on NAPI.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 29 +++++++++-----------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 70f2fb366375..d6f4a7ace05e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -934,9 +934,6 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
>                 last_c_index &= (num_tx_bds - 1);
>         }
>
> -       if (ring->free_bds > (MAX_SKB_FRAGS + 1))
> -               ring->int_disable(priv, ring);
> -
>         if (netif_tx_queue_stopped(txq) && pkts_compl)
>                 netif_tx_wake_queue(txq);
>
> @@ -1211,10 +1208,8 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>         bcmgenet_tdma_ring_writel(priv, ring->index,
>                                   ring->prod_index, TDMA_PROD_INDEX);
>
> -       if (ring->free_bds <= (MAX_SKB_FRAGS + 1)) {
> +       if (ring->free_bds <= (MAX_SKB_FRAGS + 1))

Like I mentioned in patch 2/4 comments, I feel that bcmgenet_xmit()
stops the Tx queue too early.
Too many TxBDs sit unused that non-fragmented frames could use.
I think this should be: if (ring->free_bds <= 1)


>                 netif_tx_stop_queue(txq);
> -               ring->int_enable(priv, ring);
> -       }
>
>  out:
>         spin_unlock_irqrestore(&ring->lock, flags);
> @@ -1553,9 +1548,9 @@ static int init_umac(struct bcmgenet_priv *priv)
>
>         bcmgenet_intr_disable(priv);
>
> -       cpu_mask_clear = UMAC_IRQ_RXDMA_MASK;
> +       cpu_mask_clear = UMAC_IRQ_RX_TX_MASK;
>
> -       dev_dbg(kdev, "%s:Enabling RXDMA interrupts\n", __func__);
> +       dev_dbg(kdev, "%s:Enabling RXDMA & TXDMA interrupts\n", __func__);
>
>         /* Monitor cable plug/unplugged event for internal PHY */
>         if (phy_is_internal(priv->phydev)) {
> @@ -1879,10 +1874,10 @@ static int bcmgenet_poll(struct napi_struct *napi, int budget)
>  {
>         struct bcmgenet_priv *priv = container_of(napi,
>                         struct bcmgenet_priv, napi);
> -       unsigned int work_done;
> +       unsigned int work_done, tx_work;
>
>         /* tx reclaim */
> -       bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
> +       tx_work = bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
>
>         work_done = bcmgenet_desc_rx(priv, budget);
>
> @@ -1891,9 +1886,9 @@ static int bcmgenet_poll(struct napi_struct *napi, int budget)
>         priv->rx_c_index &= DMA_C_INDEX_MASK;
>         bcmgenet_rdma_ring_writel(priv, DESC_INDEX,
>                                   priv->rx_c_index, RDMA_CONS_INDEX);
> -       if (work_done < budget) {
> +       if (work_done < budget || tx_work == 0) {

Imagine interface with a lot of Rx traffic but no Tx (e.g. UDP receiver).
For that case, work_done == budget and tx_work == 0.

Your change ends up completing NAPI when it shouldn't.

>                 napi_complete(napi);
> -               bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
> +               bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RX_TX_MASK,
>                                          INTRL2_CPU_MASK_CLEAR);
>         }
>
> @@ -1968,22 +1963,18 @@ static irqreturn_t bcmgenet_isr0(int irq, void *dev_id)
>         netif_dbg(priv, intr, priv->dev,
>                   "IRQ=0x%x\n", priv->irq0_stat);
>
> -       if (priv->irq0_stat & UMAC_IRQ_RXDMA_MASK) {
> +       if (priv->irq0_stat & UMAC_IRQ_RX_TX_MASK) {
>                 /* We use NAPI(software interrupt throttling, if
>                  * Rx Descriptor throttling is not used.
>                  * Disable interrupt, will be enabled in the poll method.
>                  */
>                 if (likely(napi_schedule_prep(&priv->napi))) {
> -                       bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_MASK,
> +                       bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RX_TX_MASK,
>                                                  INTRL2_CPU_MASK_SET);
>                         __napi_schedule(&priv->napi);
>                 }
>         }
> -       if (priv->irq0_stat &
> -                       (UMAC_IRQ_TXDMA_BDONE | UMAC_IRQ_TXDMA_PDONE)) {
> -               /* Tx reclaim */
> -               bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
> -       }
> +
>         if (priv->irq0_stat & (UMAC_IRQ_PHY_DET_R |
>                                 UMAC_IRQ_PHY_DET_F |
>                                 UMAC_IRQ_LINK_UP |
> --
> 1.9.1
>

Re: [PATCH net-next 4/4] net: bcmgenet: update ring producer index and buffer count in xmit

[Petri Gynther]

Looks good.

On Fri, Oct 24, 2014 at 1:02 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> There is no need to have both bcmgenet_xmit_single() and
> bcmgenet_xmit_frag() perform a free_bds decrement and a prod_index
> increment by one. In case one of these functions fails to map a SKB or
> fragment for transmit, we will return and exit bcmgenet_xmit() with an
> error.
>
> We can therefore safely use our local copy of nr_frags to know by how
> much we should decrement the number of free buffers available, and by
> how much the producer count must be incremented and do this in the tail
> of bcmgenet_xmit().
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Petri Gynther <pgynther@google.com>

> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index d6f4a7ace05e..3cd8c25a1120 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1012,11 +1012,6 @@ static int bcmgenet_xmit_single(struct net_device *dev,
>
>         dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, length_status);
>
> -       /* Decrement total BD count and advance our write pointer */
> -       ring->free_bds -= 1;
> -       ring->prod_index += 1;
> -       ring->prod_index &= DMA_P_INDEX_MASK;
> -
>         return 0;
>  }
>
> @@ -1054,11 +1049,6 @@ static int bcmgenet_xmit_frag(struct net_device *dev,
>                     (frag->size << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
>                     (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT));
>
> -
> -       ring->free_bds -= 1;
> -       ring->prod_index += 1;
> -       ring->prod_index &= DMA_P_INDEX_MASK;
> -
>         return 0;
>  }
>
> @@ -1202,9 +1192,11 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>
>         skb_tx_timestamp(skb);
>
> -       /* we kept a software copy of how much we should advance the TDMA
> -        * producer index, now write it down to the hardware
> -        */
> +       /* Decrement total BD count and advance our write pointer */
> +       ring->free_bds -= nr_frags + 1;
> +       ring->prod_index += nr_frags + 1;
> +       ring->prod_index &= DMA_P_INDEX_MASK;
> +
>         bcmgenet_tdma_ring_writel(priv, ring->index,
>                                   ring->prod_index, TDMA_PROD_INDEX);
>
> --
> 1.9.1
>

Re: [PATCH net-next 2/4] net: bcmgenet: return number of packets completed in TX reclaim

[Florian Fainelli]

On 10/24/2014 03:20 PM, Petri Gynther wrote:
> Hi Florian,
> 
> On Fri, Oct 24, 2014 at 1:02 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> In preparation for reclaiming transmitted buffers in NAPI context,
>> update __bcmgenet_tx_reclaim() and bcmgenet_tx_reclaim() to return the
>> number of packets completed per call.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 20 +++++++++++++-------
>>  1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index ee4d5baf09b6..70f2fb366375 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -876,14 +876,14 @@ static inline void bcmgenet_tx_ring_int_disable(struct bcmgenet_priv *priv,
>>  }
>>
>>  /* Unlocked version of the reclaim routine */
>> -static void __bcmgenet_tx_reclaim(struct net_device *dev,
>> -                                 struct bcmgenet_tx_ring *ring)
>> +static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
>> +                                         struct bcmgenet_tx_ring *ring)
>>  {
>>         struct bcmgenet_priv *priv = netdev_priv(dev);
>>         int last_tx_cn, last_c_index, num_tx_bds;
>>         struct enet_cb *tx_cb_ptr;
>>         struct netdev_queue *txq;
>> -       unsigned int bds_compl;
>> +       unsigned int bds_compl, pkts_compl = 0;
> 
> bcmgenet_desc_rx() uses "rxpktprocessed", so I'd go with "txpktprocessed" here.

Sure.

> 
>>         unsigned int c_index;
>>
>>         /* Compute how many buffers are transmitted since last xmit call */
>> @@ -928,6 +928,7 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
>>                 }
>>                 dev->stats.tx_packets++;
>>                 ring->free_bds += bds_compl;
>> +               pkts_compl += bds_compl;
> 
> This change doesn't look correct. The number of cleaned packets is not
> necessarily equal to the number of cleaned TxBDs.

Right, that should be a +1 increment, it does not matter because what we
want to know is 0 versus anything else.

> 
> I think that the while-loop should be:
> 
>         while (last_tx_cn-- > 0) {
>                 tx_cb_ptr = ring->cbs + last_c_index;
> 
>                 if (tx_cb_ptr->skb) {
>                         pkts_compl++;
>                         dev->stats.tx_packets++;
>                         dev->stats.tx_bytes += tx_cb_ptr->skb->len;
>                         dma_unmap_single(&dev->dev,
>                                          dma_unmap_addr(tx_cb_ptr, dma_addr),
>                                          tx_cb_ptr->skb->len,
>                                          DMA_TO_DEVICE);
>                         bcmgenet_free_cb(tx_cb_ptr);
>                 } else if (dma_unmap_addr(tx_cb_ptr, dma_addr)) {
>                         dev->stats.tx_bytes +=
>                                 dma_unmap_len(tx_cb_ptr, dma_len);
>                         dma_unmap_page(&dev->dev,
>                                        dma_unmap_addr(tx_cb_ptr, dma_addr),
>                                        dma_unmap_len(tx_cb_ptr, dma_len),
>                                        DMA_TO_DEVICE);
>                         dma_unmap_addr_set(tx_cb_ptr, dma_addr, 0);
>                 }
>                 ring->free_bds++;
>                 last_c_index++;
>                 last_c_index &= (num_tx_bds - 1);
>         }
> 
>>
>>                 last_c_index++;
>>                 last_c_index &= (num_tx_bds - 1);
>> @@ -936,20 +937,25 @@ static void __bcmgenet_tx_reclaim(struct net_device *dev,
>>         if (ring->free_bds > (MAX_SKB_FRAGS + 1))
>>                 ring->int_disable(priv, ring);
>>
>> -       if (netif_tx_queue_stopped(txq))
>> +       if (netif_tx_queue_stopped(txq) && pkts_compl)
> 
> bcmgenet_xmit() stops the Tx queue when:
> ring->free_bds <= (MAX_SKB_FRAGS + 1)
> 
> So, shouldn't this check be:
> netif_tx_queue_stopped(txq) && (ring->free_bds > (MAX_SKB_FRAGS + 1))
> 
> Having said that --
> Why does the driver stop the Tx queue when there are still
> (MAX_SKB_FRAGS + 1) TxBDs left?
> It leaves 17 or so TxBDs unused on the Tx ring, and most of the
> packets are not fragmented.

Right, this is something I am still trying to understand. One one hand
it does make sense to stop the queue on MAX_SKB_FRAGS + 1 just to be
safe and allow for a full-size fragment to be pushed on the next xmit()
call. But by the same token, bcmgenet_xmit() will check for the actual
fragment size early on.

I think the only one which is valid is the one at the end of
bcmgenet_xmit(), this one in bcmgenet_tx_reclaim() should just verify
that we have released some packets and that the queue was previous stopped.

> 
> I feel that bcmgenet_xmit() should stop the Tx queue only when there
> is 1 TxBD left after putting a new packet on the ring.
> 
>>                 netif_tx_wake_queue(txq);
>>
>>         ring->c_index = c_index;
>> +
>> +       return pkts_compl;
>>  }
>>
>> -static void bcmgenet_tx_reclaim(struct net_device *dev,
>> -                               struct bcmgenet_tx_ring *ring)
>> +static unsigned int bcmgenet_tx_reclaim(struct net_device *dev,
>> +                                       struct bcmgenet_tx_ring *ring)
>>  {
>>         unsigned long flags;
>> +       unsigned int pkts_compl;
>>
>>         spin_lock_irqsave(&ring->lock, flags);
>> -       __bcmgenet_tx_reclaim(dev, ring);
>> +       pkts_compl = __bcmgenet_tx_reclaim(dev, ring);
>>         spin_unlock_irqrestore(&ring->lock, flags);
>> +
>> +       return pkts_compl;
>>  }
>>
>>  static void bcmgenet_tx_reclaim_all(struct net_device *dev)
>> --
>> 1.9.1
>>