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

[netdev-bot+sashiko@kernel.org]

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