[PATCH net v2 0/2] net: bcmgenet: fix tx stalls on 32 descriptor rings

[PATCH net v2 0/2] net: bcmgenet: fix tx stalls on 32 descriptor rings

[Nicolai Buchwitz]

Found this while fixing the clashiko findings on v6 of my MTU series. After
those fixes this bug would be easier to hit. At the moment it needs a larger
CONFIG_MAX_SKB_FRAGS than the default.

Queues 1 to 4 hold 32 descriptors, which is less than the Tx path assumes
in two places with CONFIG_MAX_SKB_FRAGS >= 31.

The wake threshold wants MAX_SKB_FRAGS + 1 free descriptors, which is not
possible on these rings. Therefore the queue stops on the first packet
without any recovery.

Same situation for the admission check, which wants one descriptor more than
the frame needs. As a result a 31 fragment frame is refused, even if the
ring is empty.

Patch 1 caps the threshold, patch 2 flattens those frames.

Patch 1 was tested on a Raspberry Pi CM4 (BCM2711, genet v5). The 31 fragment
case was flagged by Clashiko in v1 and seems reasonable.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Changes in v2:
- New patch 2 for the admission check (clashiko)
- Link to v1: https://patch.msgid.link/20261106-nb-genet-txq-thld-net-v1-1-53bbac647b71@tipi-net.de

Patch 2 could also clear NETIF_F_SG in ndo_features_check like bnge does. I
kept it in xmit because the limit is the ring size.

To: Doug Berger <opendmb@gmail.com>
To: Florian Fainelli <florian.fainelli@broadcom.com>
To: Nicolai Buchwitz <nb@tipi-net.de>
To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Nikolay Aleksandrov <razor@blackwall.org>
To: Jason Xing <kerneljasonxing@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Nicolai Buchwitz (2):
      net: bcmgenet: cap the Tx queue threshold at the ring size
      net: bcmgenet: flatten frames that cannot fit the ring

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 22 ++++++++++++++++++++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 2 files changed, 21 insertions(+), 2 deletions(-)
---
base-commit: 1d5e2c57e9ffc134e14b3f837c8d7c53b508bd22
change-id: 20261106-nb-genet-txq-thld-net-e0864f235d22

Best regards,
--  
Nicolai Buchwitz <nb@tipi-net.de>

[PATCH net v2 1/2] net: bcmgenet: cap the Tx queue threshold at the ring size

[Nicolai Buchwitz]

A Tx queue stops when MAX_SKB_FRAGS + 1 or fewer descriptors are free and
wakes once there are more. Queues 1 to 4 hold 32 descriptors, so with
CONFIG_MAX_SKB_FRAGS at 31 they would need 33 to wake. As a result the
first packet stops the queue and it stays stopped:

  eth0: NETDEV WATCHDOG: CPU: 0: transmit queue 3 timed out 2004 ms
  eth0: NETDEV WATCHDOG: CPU: 1: transmit queue 4 timed out 2004 ms

On a Raspberry Pi CM4 only queue 0 carried traffic afterwards. The other
four counted an error on nearly every packet.

Fix this and cap the threshold at what the ring can free.

Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 6 ++++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 4d2be2a21ed1b..9049822012d13 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2030,7 +2030,7 @@ static int bcmgenet_tx_poll(struct napi_struct *napi, int budget)
 
 	spin_lock(&ring->lock);
 	__bcmgenet_tx_reclaim(ring->priv->dev, ring);
-	if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
+	if (ring->free_bds > ring->stop_thld) {
 		txq = netdev_get_tx_queue(ring->priv->dev, ring->index);
 		netif_tx_wake_queue(txq);
 	}
@@ -2235,7 +2235,7 @@ 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))
+	if (ring->free_bds <= ring->stop_thld)
 		netif_tx_stop_queue(txq);
 
 	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
@@ -2742,6 +2742,8 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 	ring->index = index;
 	ring->cbs = priv->tx_cbs + start_ptr;
 	ring->size = size;
+	/* An empty ring must be able to wake the queue */
+	ring->stop_thld = min_t(unsigned int, MAX_SKB_FRAGS + 1, size - 1);
 	ring->clean_ptr = start_ptr;
 	ring->c_index = 0;
 	ring->free_bds = size;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index e48ce39d3e7c8..6ab3f73475550 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -546,6 +546,7 @@ struct bcmgenet_tx_ring {
 	unsigned int	index;		/* ring index */
 	struct enet_cb	*cbs;		/* tx ring buffer control block*/
 	unsigned int	size;		/* size of each tx ring */
+	unsigned int	stop_thld;	/* queue stops at this many free bds or fewer */
 	unsigned int    clean_ptr;      /* Tx ring clean pointer */
 	unsigned int	c_index;	/* last consumer index of each ring*/
 	unsigned int	free_bds;	/* # of free bds for each ring */

-- 
2.53.0

[PATCH net v2 2/2] net: bcmgenet: flatten frames that cannot fit the ring

[Nicolai Buchwitz]

bcmgenet_xmit() wants one descriptor more than the frame needs. A 31
fragment frame needs 32 descriptors, so it is refused on a 32 descriptor
ring, even if the ring is empty.

As a result nothing is in flight which could wake the queue again. The
watchdog can only hand the same frame back, therefore the queue stays
stopped.

This needs CONFIG_MAX_SKB_FRAGS >= 31 and a sender which fragments that
finely, for example MSG_ZEROCOPY.

Linearize those frames instead.

Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 9049822012d13..61f5a4c2f0cd9 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2143,6 +2143,22 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	nr_frags = skb_shinfo(skb)->nr_frags;
 
+	/* The check below would always refuse this frame */
+	if (unlikely(nr_frags + 1 >= ring->size)) {
+		skb = skb_share_check(skb, GFP_ATOMIC);
+		if (skb && skb_linearize(skb)) {
+			dev_kfree_skb_any(skb);
+			skb = NULL;
+		}
+		if (!skb) {
+			spin_lock(&ring->lock);
+			BCMGENET_STATS64_INC((&ring->stats64), dropped);
+			ret = NETDEV_TX_OK;
+			goto out;
+		}
+		nr_frags = 0;
+	}
+
 	spin_lock(&ring->lock);
 	if (ring->free_bds <= (nr_frags + 1)) {
 		if (!netif_tx_queue_stopped(txq))

-- 
2.53.0

Re: [PATCH net v2 2/2] net: bcmgenet: flatten frames that cannot fit the ring

[Florian Fainelli]

On 11/9/26 13:53, Nicolai Buchwitz wrote:
> bcmgenet_xmit() wants one descriptor more than the frame needs. A 31
> fragment frame needs 32 descriptors, so it is refused on a 32 descriptor
> ring, even if the ring is empty.
> 
> As a result nothing is in flight which could wake the queue again. The
> watchdog can only hand the same frame back, therefore the queue stays
> stopped.
> 
> This needs CONFIG_MAX_SKB_FRAGS >= 31 and a sender which fragments that
> finely, for example MSG_ZEROCOPY.
> 
> Linearize those frames instead.
> 
> Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian