[PATCH net v2 0/4] fix queue lock up and reduce timeouts

[PATCH net v2 0/4] fix queue lock up and reduce timeouts

[Justin Chen]

We have been seeing reports of logs like this.
# [   39.713199] bcmgenet 1001300000.ethernet eth0: NETDEV WATCHDOG: CPU: 0: transmit queue 2 timed out 7991 ms
[   41.761198] bcmgenet 1001300000.ethernet eth0: NETDEV WATCHDOG: CPU: 0: transmit queue 2 timed out 10039 ms
[   43.745198] bcmgenet 1001300000.ethernet eth0: NETDEV WATCHDOG: CPU: 0: transmit queue 2 timed out 12023 ms
[   45.729198] bcmgenet 1001300000.ethernet eth0: NETDEV WATCHDOG: CPU: 0: transmit queue 2 timed out 14007 ms

We have two issues. The persistent queue timeouts and the eventual lock up of the entire transmit.

This is due to two issues.
- We have a small queue that is prone to timeouts. We optimize this by relaxing the tx queue stop conditions.
- We have a racing and leaking timeout handler that is also fixed.

v2
- These patches stayed the same.
  net: bcmgenet: fix off-by-one in bcmgenet_put_txcb
  net: bcmgenet: fix leaking free_bds
- New patches that fix a few other slightly related issues.
  net: bcmgenet: fix off-by-one in bcmgenet_put_txcb
  net: bcmgenet: relax the xmit ring full case

Justin Chen (4):
  net: bcmgenet: fix off-by-one in bcmgenet_put_txcb
  net: bcmgenet: fix leaking free_bds
  net: bcmgenet: fix racing timeout handler
  net: bcmgenet: relax the xmit ring full case

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

-- 
2.34.1

[PATCH net v2 1/4] net: bcmgenet: fix off-by-one in bcmgenet_put_txcb

[Justin Chen]

The write_ptr points to the next open tx_cb. We want to return the
tx_cb that gets rewinded, so we must rewind the pointer first then
return the tx_cb that it points to. That way the txcb can be correctly
cleaned up.

Fixes: 876dbadd53a7 ("net: bcmgenet: Fix unmapping of fragments in bcmgenet_xmit()")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 482a31e7b72b..0f6e4baba25b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1819,15 +1819,15 @@ static struct enet_cb *bcmgenet_put_txcb(struct bcmgenet_priv *priv,
 {
 	struct enet_cb *tx_cb_ptr;
 
-	tx_cb_ptr = ring->cbs;
-	tx_cb_ptr += ring->write_ptr - ring->cb_ptr;
-
 	/* Rewinding local write pointer */
 	if (ring->write_ptr == ring->cb_ptr)
 		ring->write_ptr = ring->end_ptr;
 	else
 		ring->write_ptr--;
 
+	tx_cb_ptr = ring->cbs;
+	tx_cb_ptr += ring->write_ptr - ring->cb_ptr;
+
 	return tx_cb_ptr;
 }
 
-- 
2.34.1

[PATCH net v2 2/4] net: bcmgenet: fix leaking free_bds

[Justin Chen]

While reclaiming the tx queue we fast forward the write pointer to
drop any data in flight. These dropped frames are not added back
to the pool of free bds. We also need to tell the netdev that we
are dropping said data.

Fixes: f1bacae8b655 ("net: bcmgenet: support reclaiming unsent Tx packets")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Tested-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 0f6e4baba25b..e89126a0c20e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1985,6 +1985,7 @@ static unsigned int bcmgenet_tx_reclaim(struct net_device *dev,
 		drop = (ring->prod_index - ring->c_index) & DMA_C_INDEX_MASK;
 		released += drop;
 		ring->prod_index = ring->c_index & DMA_C_INDEX_MASK;
+		ring->free_bds += drop;
 		while (drop--) {
 			cb_ptr = bcmgenet_put_txcb(priv, ring);
 			skb = cb_ptr->skb;
@@ -1996,6 +1997,7 @@ static unsigned int bcmgenet_tx_reclaim(struct net_device *dev,
 		}
 		if (skb)
 			dev_consume_skb_any(skb);
+		netdev_tx_reset_queue(netdev_get_tx_queue(dev, ring->index));
 		bcmgenet_tdma_ring_writel(priv, ring->index,
 					  ring->prod_index, TDMA_PROD_INDEX);
 		wr_ptr = ring->write_ptr * WORDS_PER_BD(priv);
-- 
2.34.1

[PATCH net v2 3/4] net: bcmgenet: fix racing timeout handler

[Justin Chen]

The bcmgenet_timeout handler tries to take down all tx queues when
a single queue times out. This is over zealous and causes many race
conditions with queues that are still chugging along. Instead lets
only restart the timed out queue.

Fixes: 13ea657806cf ("net: bcmgenet: improve TX timeout")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Tested-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 22 ++++++++-----------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index e89126a0c20e..54f71b1e85fc 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3477,27 +3477,23 @@ static void bcmgenet_dump_tx_queue(struct bcmgenet_tx_ring *ring)
 static void bcmgenet_timeout(struct net_device *dev, unsigned int txqueue)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
-	u32 int1_enable = 0;
-	unsigned int q;
+	struct bcmgenet_tx_ring *ring = &priv->tx_rings[txqueue];
+	struct netdev_queue *txq = netdev_get_tx_queue(dev, txqueue);
 
 	netif_dbg(priv, tx_err, dev, "bcmgenet_timeout\n");
 
-	for (q = 0; q <= priv->hw_params->tx_queues; q++)
-		bcmgenet_dump_tx_queue(&priv->tx_rings[q]);
-
-	bcmgenet_tx_reclaim_all(dev);
+	bcmgenet_dump_tx_queue(ring);
 
-	for (q = 0; q <= priv->hw_params->tx_queues; q++)
-		int1_enable |= (1 << q);
+	bcmgenet_tx_reclaim(dev, ring, true);
 
-	/* Re-enable TX interrupts if disabled */
-	bcmgenet_intrl2_1_writel(priv, int1_enable, INTRL2_CPU_MASK_CLEAR);
+	/* Re-enable the TX interrupt for this ring */
+	bcmgenet_intrl2_1_writel(priv, 1 << txqueue, INTRL2_CPU_MASK_CLEAR);
 
-	netif_trans_update(dev);
+	txq_trans_cond_update(txq);
 
-	BCMGENET_STATS64_INC((&priv->tx_rings[txqueue].stats64), errors);
+	BCMGENET_STATS64_INC((&ring->stats64), errors);
 
-	netif_tx_wake_all_queues(dev);
+	netif_tx_wake_queue(txq);
 }
 
 #define MAX_MDF_FILTER	17
-- 
2.34.1

[PATCH net v2 4/4] net: bcmgenet: relax the xmit ring full case

[Justin Chen]

We have a queue size of 32. If a packet is multiple fragments we can
run through this queue really quickly. Currently we stop the xmit
queue at SKB_FRAG_SIZE which by default can take up half the queue.
Instead lets just look at the incoming packet and freeze the queue
if the incoming packet has more fragments than free_bds. This will
relieve some of the queue timeouts we have been seeing.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 54f71b1e85fc..a1aa1278842e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2018,10 +2018,10 @@ static int bcmgenet_tx_poll(struct napi_struct *napi, int budget)
 
 	spin_lock(&ring->lock);
 	work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
-	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
-		txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
+	txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
+	if (netif_tx_queue_stopped(txq))
 		netif_tx_wake_queue(txq);
-	}
+
 	spin_unlock(&ring->lock);
 
 	if (work_done == 0) {
@@ -2224,9 +2224,6 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	netdev_tx_sent_queue(txq, GENET_CB(skb)->bytes_sent);
 
-	if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
-		netif_tx_stop_queue(txq);
-
 	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
 		/* Packets are ready, update producer index */
 		bcmgenet_tdma_ring_writel(priv, ring->index,
-- 
2.34.1

Re: [PATCH net v2 1/4] net: bcmgenet: fix off-by-one in bcmgenet_put_txcb

[Nicolai Buchwitz]

On 1.4.2026 02:38, Justin Chen wrote:
> The write_ptr points to the next open tx_cb. We want to return the
> tx_cb that gets rewinded, so we must rewind the pointer first then
> return the tx_cb that it points to. That way the txcb can be correctly
> cleaned up.
> 
> Fixes: 876dbadd53a7 ("net: bcmgenet: Fix unmapping of fragments in 
> bcmgenet_xmit()")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 482a31e7b72b..0f6e4baba25b 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1819,15 +1819,15 @@ static struct enet_cb *bcmgenet_put_txcb(struct 
> bcmgenet_priv *priv,
>  {
>  	struct enet_cb *tx_cb_ptr;
> 
> -	tx_cb_ptr = ring->cbs;
> -	tx_cb_ptr += ring->write_ptr - ring->cb_ptr;
> -
>  	/* Rewinding local write pointer */
>  	if (ring->write_ptr == ring->cb_ptr)
>  		ring->write_ptr = ring->end_ptr;
>  	else
>  		ring->write_ptr--;
> 
> +	tx_cb_ptr = ring->cbs;
> +	tx_cb_ptr += ring->write_ptr - ring->cb_ptr;
> +
>  	return tx_cb_ptr;
>  }

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks

Nicolai

Re: [PATCH net v2 4/4] net: bcmgenet: relax the xmit ring full case

[Nicolai Buchwitz]

On 1.4.2026 02:38, Justin Chen wrote:
> We have a queue size of 32. If a packet is multiple fragments we can
> run through this queue really quickly. Currently we stop the xmit
> queue at SKB_FRAG_SIZE which by default can take up half the queue.
> Instead lets just look at the incoming packet and freeze the queue
> if the incoming packet has more fragments than free_bds. This will
> relieve some of the queue timeouts we have been seeing.
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 54f71b1e85fc..a1aa1278842e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2018,10 +2018,10 @@ static int bcmgenet_tx_poll(struct napi_struct 
> *napi, int budget)
> 
>  	spin_lock(&ring->lock);
>  	work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
> -	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
> -		txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
> +	txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
> +	if (netif_tx_queue_stopped(txq))
>  		netif_tx_wake_queue(txq);
> -	}
> +
>  	spin_unlock(&ring->lock);
> 
>  	if (work_done == 0) {
> @@ -2224,9 +2224,6 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
> *skb, struct net_device *dev)
> 
>  	netdev_tx_sent_queue(txq, GENET_CB(skb)->bytes_sent);
> 
> -	if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
> -		netif_tx_stop_queue(txq);
> -

With this removed, the only queue stop is the NETDEV_TX_BUSY path at
entry. That means every ring-full hits BUSY and requeues rather than
stopping proactively on the previous transmit.

Would it work to keep the proactive stop but base it on the actual
packet size instead of MAX_SKB_FRAGS?

	if (ring->free_bds <= (nr_frags + 1))
		netif_tx_stop_queue(txq);

That avoids reserving half the ring for the worst case while still
preventing BUSY from becoming the normal path.

>  	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
>  		/* Packets are ready, update producer index */
>  		bcmgenet_tdma_ring_writel(priv, ring->index,

This wakes unconditionally even if only 1 BD was reclaimed. Combined
with the above, it can create a tight stop/wake/BUSY thrash when the
ring is near-full.

Thanks

Nicolai

Re: [PATCH net v2 4/4] net: bcmgenet: relax the xmit ring full case

[Justin Chen]

On 4/1/26 12:47 AM, Nicolai Buchwitz wrote:
> On 1.4.2026 02:38, Justin Chen wrote:
>> We have a queue size of 32. If a packet is multiple fragments we can
>> run through this queue really quickly. Currently we stop the xmit
>> queue at SKB_FRAG_SIZE which by default can take up half the queue.
>> Instead lets just look at the incoming packet and freeze the queue
>> if the incoming packet has more fragments than free_bds. This will
>> relieve some of the queue timeouts we have been seeing.
>>
>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>> Signed-off-by: Justin Chen <justin.chen@broadcom.com>
>> ---
>>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/ 
>> net/ethernet/broadcom/genet/bcmgenet.c
>> index 54f71b1e85fc..a1aa1278842e 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> @@ -2018,10 +2018,10 @@ static int bcmgenet_tx_poll(struct napi_struct 
>> *napi, int budget)
>>
>>      spin_lock(&ring->lock);
>>      work_done = __bcmgenet_tx_reclaim(ring->priv->dev, ring);
>> -    if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
>> -        txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
>> +    txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
>> +    if (netif_tx_queue_stopped(txq))
>>          netif_tx_wake_queue(txq);
>> -    }
>> +
>>      spin_unlock(&ring->lock);
>>
>>      if (work_done == 0) {
>> @@ -2224,9 +2224,6 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
>> *skb, struct net_device *dev)
>>
>>      netdev_tx_sent_queue(txq, GENET_CB(skb)->bytes_sent);
>>
>> -    if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
>> -        netif_tx_stop_queue(txq);
>> -
> 
> With this removed, the only queue stop is the NETDEV_TX_BUSY path at
> entry. That means every ring-full hits BUSY and requeues rather than
> stopping proactively on the previous transmit.
> 
> Would it work to keep the proactive stop but base it on the actual
> packet size instead of MAX_SKB_FRAGS?
> 
>      if (ring->free_bds <= (nr_frags + 1))
>          netif_tx_stop_queue(txq);
> 
> That avoids reserving half the ring for the worst case while still
> preventing BUSY from becoming the normal path.
> 
>>      if (!netdev_xmit_more() || netif_xmit_stopped(txq))
>>          /* Packets are ready, update producer index */
>>          bcmgenet_tdma_ring_writel(priv, ring->index,
> 
> This wakes unconditionally even if only 1 BD was reclaimed. Combined
> with the above, it can create a tight stop/wake/BUSY thrash when the
> ring is near-full.
> 

That is fair. Let me drop this patch entirely. Seems like we are trading 
one problem for another problem. Maybe removing the priority queues 
entirely is the way to go. I will wait for the XDP patches to be applied 
first to avoid conflicts.

Thanks,
Justin

> Thanks
> 
> Nicolai