Re: [PATCH net] net: bcmgenet: cap the Tx queue threshold at the ring size

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch only partly fixes the Tx stall it targets.

--

net: bcmgenet: cap the Tx queue threshold at the ring size

This patch adds a per-ring stop_thld field, capped at size - 1. The field
replaces MAX_SKB_FRAGS + 1 as the threshold for stopping and waking Tx
queues in bcmgenet_xmit() and bcmgenet_tx_poll(). The goal is to stop the
32-descriptor priority rings from staying stopped forever when
CONFIG_MAX_SKB_FRAGS is 31 or larger.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 4d2be2a21ed1b..186d17781b890 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -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;
> +	/* A ring smaller than that would never wake */
> +	ring->stop_thld = min_t(unsigned int, MAX_SKB_FRAGS + 1, size - 1);

[Severity: High]

Does this fully fix the stall on queues 1 to 4 when CONFIG_MAX_SKB_FRAGS
is 31 or more? The wake threshold is now capped, but the per-skb admission
check at the top of bcmgenet_xmit() has not changed:

bcmgenet_xmit() {
    ...
	if (ring->free_bds <= (nr_frags + 1)) {
		if (!netif_tx_queue_stopped(txq))
			netif_tx_stop_queue(txq);
		ret = NETDEV_TX_BUSY;
		goto out;
	}
    ...
}

free_bds can never be larger than ring->size (32). So an skb with
nr_frags >= 31 gets rejected even when the ring is completely empty.

In that case nothing is in flight, so no Tx-done interrupt arrives and
bcmgenet_tx_poll() never wakes the queue. The watchdog is the only wake
source left:

dev_watchdog()
  bcmgenet_timeout()
    bcmgenet_tx_reclaim(dev, ring, true);
    BCMGENET_STATS64_INC((&ring->stats64), errors);
    netif_tx_wake_queue(txq);

The qdisc then retries the same requeued skb and gets NETDEV_TX_BUSY
again, so the queue stops again.

Wouldn't that leave the queue, and every flow hashed to it, stuck for
good? The "NETDEV WATCHDOG: transmit queue N timed out" message and the
error count from the commit message would then come back every
watchdog_timeo.

The core does not keep such skbs away from the driver. The driver
advertises NETIF_F_SG, and it has no ndo_features_check and no
skb_linearize() fallback. Highly fragmented skbs can come from
MSG_ZEROCOPY, from splice, or from forwarded traffic.

Should bcmgenet_xmit() also handle skbs where nr_frags + 1 >= ring->size?
It could linearize or drop them, or clear NETIF_F_SG for them in
ndo_features_check.

>  	ring->clean_ptr = start_ptr;
>  	ring->c_index = 0;
>  	ring->free_bds = size;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-nb-genet-txq-thld-net-v1-1-53bbac647b71%40tipi-net.de