[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

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

[Linkui Xiao]

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

Agreed, and v4 no longer moves the store: the value is now dropped on the
paths that never hand the context descriptor to the DMA, which is what the
commit message was claiming in the first place.

Two things are worth separating though.

The interleaving itself is not introduced by the patch.
stmmac_tx_err() is called from the DMA interrupt handler without any
serialisation against ndo_start_xmit(), so a reset can land anywhere
inside stmmac_tso_xmit(), and a reset that lands before the early
"tx_q->mss = mss" -- anywhere between the entry of the function and that
store, which includes stmmac_tso_get_num_desc() and the availability check
-- leaves a non-zero cache for the channel that was just re-initialised
with the old placement as well. So it is not true that "with the old
placement a later reset always won"; what the patch did is widen that
window by roughly the length of the mapping section holding the two
dma_map_single()/skb_frag_dma_map() calls, and that is what the new
placement has to answer for. Which it now does not: v4 keeps the store
where it was and invalidates the cache on the error paths instead. Zero is
what stmmac_reset_tx_queue() leaves behind, so it cannot republish
anything even if the reset lands in between, and the next TSO frame
programs the context descriptor again.

Second, in that interleaving the cached MSS is not the only thing left out
of step: the transmit path keeps running from its local first_entry, writes
tx_q->cur_tx = entry and lets stmmac_flush_tx_descriptors() push the tail,
while stmmac_init_tx_chan() has pointed the channel back at tx_q->dma_tx_phy
-- the software ring state and the hardware no longer agree, which is what
the reset is supposed to establish. Fixing that means the reset must stop
racing ndo_start_xmit() altogether, and it cannot be a lock taken in the
handler: the interrupted transmit path already holds the queue's
_xmit_lock, so the handler would be waiting for the context it interrupted.
That needs the channel reset moved out of hard IRQ context, and I would
rather send it as a separate change than fold it into a -net fix for the
ring wedge.

v4 therefore:

   1. drops tx_q->mss on the error paths of stmmac_tso_xmit() (new),
   2. keeps filling the context descriptor at the slot tx_q->cur_tx points
      to and releasing it explicitly on those paths (unchanged from v3),
   3. leaves the store ahead of the mappings, so the patch no longer changes
      the timing with respect to your interleaving at all.

The subject changed accordingly: the patch no longer moves the store, it
invalidates the cached value. Code changed, so the Acked-by from Lorenzo
Bianconi is not carried over.

pw-bot: cr