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

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

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

stmmac_tso_xmit() fills the MSS context descriptor and stores the new MSS
in tx_q->mss right away, but the descriptor only gets its OWN bit much
later, right before the frame is handed to the DMA. Every error path in
between - the dma_map_single() of the linear part and the
skb_frag_dma_map() of each fragment - returns with tx_q->mss already
updated while the MAC is still programmed with the previous MSS.

The abandoned context descriptor is never handed to the DMA:
stmmac_set_mss() does not set the OWN bit, and the error paths return
before stmmac_flush_tx_descriptors(), which is the only place that
advances the TX tail pointer. When a later xmit advances the tail pointer
past the abandoned slot, the DMA stops on the not-owned context
descriptor and suspends; stmmac_tx_clean() then reclaims the slot in
software but stops at the first descriptor the DMA still owns, so the
ring can never wrap around. The queue stalls until the watchdog fires
and stmmac_tx_err() resets the channel, which also clears the stale
tx_q->mss via stmmac_reset_tx_queue().

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 context descriptor is now handled like the data descriptors are:
tx_q->cur_tx is not advanced while it is being filled. Whether the frame
can be queued is only known after every dma_map_single() and
skb_frag_dma_map() has succeeded, so the descriptor stays at the slot
tx_q->cur_tx points to and the index moves past it later, together with
the data descriptors. That also keeps the context descriptor outside the
range stmmac_tx_clean() walks when the ring is cleaned after a failure,
so the error paths have to release it explicitly. A mapping failure
therefore leaves the slot reusable instead of parked in the middle of
the ring as a descriptor the DMA will stop on.

Fixes: f748be531d70 ("stmmac: support new GMAC4")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
Changes in v3:
- Rewrote the failure description in the commit message: after a mapping
  failure the not-owned context descriptor wedges the TX ring (the DMA
  suspends on it, stmmac_tx_clean() pins dirty_tx, only the watchdog
  reset recovers), it does not produce mis-sized segments as previously
  claimed. No code change. (Sashiko AI review)
- Kept Lorenzo's Acked-by, as the patch is unchanged from v2.
- Link: https://lore.kernel.org/all/20260920061609.1919876-1-xiaolinkui@126.com/

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a268cd4acdef..276187f50ee3 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)) {
@@ -4577,6 +4573,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	first_entry = tx_q->cur_tx;
+	if (mss_desc)
+		first_entry = STMMAC_NEXT_ENTRY(first_entry,
+						priv->dma_conf.dma_tx_size);
 	entry = first_entry;
 
 	WARN_ON(tx_q->tx_skbuff[entry]);
@@ -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;
 	}
 
 	if (netif_msg_pktdata(priv)) {
@@ -4745,6 +4745,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 						priv->dma_conf.dma_tx_size);
 	}
 error:
+	if (mss_desc)
+		stmmac_release_tx_desc(priv, mss_desc, priv->descriptor_mode);
+
 	dev_err(priv->device, "Tx dma map failed\n");
 	dev_kfree_skb(skb);
 	priv->xstats.tx_dropped++;
-- 
2.25.1

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