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

[PATCH net v2] 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 later reclaimed by stmmac_tx_clean().

The next skb carrying the same MSS then compares equal to the cached
value, so no context descriptor is emitted and the hardware segments the
TCP stream with a stale MSS, generating frames whose payload size does
not match what the stack accounted for.

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.

Fixes: f748be531d70 ("stmmac: support new GMAC4")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
Changes in v2:
- Do not advance tx_q->cur_tx while the context descriptor is filled, as
  for the data descriptors, and release the context descriptor on the
  error paths instead of leaving it to stmmac_tx_clean(). (Lorenzo Bianconi)

 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 af2d38a2bb3d..e2e680dd980c 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 v2] net: stmmac: do not cache the new TSO MSS before it reaches the DMA

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

> 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 later reclaimed by stmmac_tx_clean().
> 
> The next skb carrying the same MSS then compares equal to the cached
> value, so no context descriptor is emitted and the hardware segments the
> TCP stream with a stale MSS, generating frames whose payload size does
> not match what the stack accounted for.
> 
> 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.
> 
> 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 v2:
> - Do not advance tx_q->cur_tx while the context descriptor is filled, as
>   for the data descriptors, and release the context descriptor on the
>   error paths instead of leaving it to stmmac_tx_clean(). (Lorenzo Bianconi)
> 
>  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 af2d38a2bb3d..e2e680dd980c 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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net v2] 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] The commit message of "net: stmmac: do not cache the new TSO
  MSS before it reaches the DMA" describes a failure mode that cannot…

--

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() so it happens
only after the MSS context descriptor gets its OWN bit, keeps tx_q->cur_tx
parked on the context descriptor slot while the frame is being built, and
releases that descriptor explicitly on the error paths.

The code change itself looks fine; the question below is only about the
changelog.

>     The next skb carrying the same MSS then compares equal to the cached
>     value, so no context descriptor is emitted and the hardware segments the
>     TCP stream with a stale MSS, generating frames whose payload size does
>     not match what the stack accounted for.

[Severity: Medium]
Can that on-the-wire symptom actually be observed before this patch?

Before the patch, the abandoned context descriptor sits at the slot that
tx_q->cur_tx had when stmmac_set_mss() was called, and cur_tx was already
advanced past it, so every later frame is queued behind that descriptor.
Neither dwmac4_set_mss_ctxt() nor dwmac4_release_tx_desc() ever sets the
OWN bit, so the slot stays at OWN=0.

The error paths in stmmac_tso_xmit() return without calling
stmmac_flush_tx_descriptors(), and that is the only place the tail pointer
is written:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_flush_tx_descriptors() {
	...
	stmmac_set_queue_tx_tail_ptr(priv, tx_q, queue, tx_q->cur_tx);
	...
}

so the engine stays parked on the abandoned context descriptor. When a
later xmit bumps the tail, the DMA re-fetches that slot, finds OWN=0 and
suspends again.

Meanwhile stmmac_tx_clean() reclaims the OWN=0 context slot in software and
then stops at the first descriptor the DMA still owns:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_tx_clean() {
	...
		if (unlikely(status & tx_dma_own))
			break;
	...
}

which pins dirty_tx just past the stuck slot, so cur_tx can never wrap
around and rewrite it.

Doesn't that mean no frame queued after the mapping failure is segmented at
all, stale MSS or otherwise, and that the real pre-patch consequence is a
wedged tx ring recovered only by the watchdog / stmmac_tx_err() path, which
resets the channel and clears tx_q->mss via stmmac_reset_tx_queue()?

The patch does fix that by keeping tx_q->cur_tx on the context slot and
releasing mss_desc on the error paths, but the changelog does not mention
the stall. Since this carries a Fixes: tag and Cc: stable, could the
changelog describe the tx stall and controller reset instead of the
mis-sized segment description?

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

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

[Linkui Xiao]

- [Medium] The commit message of "net: stmmac: do not cache the new TSO
   MSS before it reaches the DMA" describes a failure mode that cannot…

Agreed -- thanks for the analysis, the on-the-wire symptom I described
cannot happen. After a mapping failure the abandoned context descriptor
stays at OWN=0, the error paths never advance the tail pointer, and once
a later xmit publishes it the DMA suspends on the not-owned descriptor;
stmmac_tx_clean() reclaims the slot in software but pins dirty_tx at the
first descriptor the DMA still owns, so no later frame is ever segmented
at all -- stale MSS or otherwise. The real pre-patch consequence is the
wedged TX ring you describe, recovered only by the reset path
(stmmac_tx_err() -> stmmac_reset_tx_queue(), which also clears the stale
tx_q->mss; reachable from both the watchdog and the DMA error interrupt).

I will respin with the commit message rewritten to describe the stall;
the code change is unchanged (keeping tx_q->cur_tx on the context slot
and releasing it on the error paths is what prevents the wedge).

pw-bot: cr