Re: [PATCH net v3] net: stmmac: do not cache the new TSO MSS before it reaches the DMA

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] Deferring the `tx_q->mss = mss` publication to the end of
  stmmac_tso_xmit() (stmmac_main.c:4715, right after…

--

net: stmmac: do not cache the new TSO MSS before it reaches the DMA

The patch moves the tx_q->mss update in stmmac_tso_xmit() from the point
where the MSS context descriptor is filled to the point where its OWN bit is
set, and keeps the context descriptor at the slot tx_q->cur_tx points to so
the error paths can release it explicitly.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..830e80cee6dca 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4563,10 +4563,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  			mss_desc = &tx_q->dma_tx[tx_q->cur_tx];
>  
>  		stmmac_set_mss(priv, mss_desc, mss);
> -		tx_q->mss = mss;
> -		tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx,
> -						priv->dma_conf.dma_tx_size);
> -		WARN_ON(tx_q->tx_skbuff[tx_q->cur_tx]);
>  	}
>  
>  	if (netif_msg_tx_queued(priv)) {

[ ... ]

> @@ -4714,6 +4713,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  		 */
>  		dma_wmb();
>  		stmmac_set_tx_owner(priv, mss_desc);
> +		tx_q->mss = mss;
>  	}

[Severity: Medium]
Can this new write position re-publish a stale non-zero tx_q->mss after a
concurrent TX channel reset has invalidated it?

stmmac_tso_xmit() runs under __netif_tx_lock only, but stmmac_tx_err() is
called directly from hard IRQ context and takes no TX queue lock:

stmmac_msi_intr_tx()
    } else if (unlikely(status == tx_hard_error)) {
        stmmac_tx_err(priv, chan);
    }

stmmac_dma_interrupt()
        } else if (unlikely(status[chan] == tx_hard_error)) {
            stmmac_tx_err(priv, chan);
        }

stmmac_tx_err() reinitialises the channel and clears the cached value:

stmmac_tx_err()
    stmmac_stop_tx_dma(priv, chan);
    dma_free_tx_skbufs(priv, &priv->dma_conf, chan);
    stmmac_clear_tx_descriptors(priv, &priv->dma_conf, chan);
    stmmac_reset_tx_queue(priv, chan);
    stmmac_init_tx_chan(...);
    stmmac_start_tx_dma(priv, chan);

stmmac_reset_tx_queue()
    tx_q->cur_tx = 0;
    tx_q->dirty_tx = 0;
    tx_q->mss = 0;

If that interrupt lands after the descriptors have been filled but before
the deferred update, the transmit path still executes:

        dma_wmb();
        stmmac_set_tx_owner(priv, mss_desc);
        tx_q->mss = mss;

which leaves a non-zero cached MSS for a channel that was just
re-initialised and never programmed with it. The next TSO frame with the
same gso_size then takes the mss == tx_q->mss path and emits no context
descriptor. With the old placement the write happened early, so a later
reset always won and the cached value ended up as 0.

Does this interact with the commit message statement "Update tx_q->mss only
once the context descriptor has been given to the DMA, so that the cached
value always describes what the hardware is actually programmed with"? The
tx_q->mss = 0 in stmmac_reset_tx_queue() suggests the driver assumes the
MAC loses its latched MSS across the reset, in which case that invariant
still does not hold for this interleaving.

The missing serialisation of stmmac_tx_err() against the TX path is older
than this patch and already perturbs cur_tx/dirty_tx, but the stale
non-zero MSS outcome comes from the new write placement. Would it make
sense to note this, or to keep the cache coherent some other way?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922124408.645496-1-xiaolinkui%40126.com