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

Hi Lorenzo,

thanks for the review.

On 2026/9/18 01:37, Lorenzo Bianconi wrote:
>> From: Linkui Xiao <[email protected]>
>>
>> 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.
>>
>> Only update tx_q->mss once the context descriptor has been given to the
>> DMA so that the cached value always describes what the hardware is
>> actually programmed with.
>>
>> Fixes: f748be531d70 ("stmmac: support new GMAC4")
>> Cc: [email protected]
>> Signed-off-by: Linkui Xiao <[email protected]>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index af2d38a2bb3d..a8f94cd6abb4 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -4563,7 +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]);
>> @@ -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;
> 
> I think the issue is real. A couple of comments:
> - do you think we should run stmmac_release_tx_desc() on mss descritpr in order
>    to clean it up?
> - I guess we should use the same approach used for data descriptor and advance
>    tx_q->cur_tx when there are no other possible error condition. What do you
>    think?

I started from your second suggestion, because that is the one that 
makes the
error handling consistent: tx_q->cur_tx is no longer advanced while the
context descriptor is being filled, it moves past it together with the data
descriptors, exactly like stmmac_xmit() does. On failure the ring index is
therefore still where it was and there is nothing to unwind.

That in turn makes your first suggestion mandatory rather than optional.
stmmac_tx_clean() walks the ring until it reaches tx_q->cur_tx, and the
context descriptor now sits exactly at the slot cur_tx points to, so the
cleaner cannot reach it and a descriptor tagged with CTXT|TCMSSV would stay
in the ring until the slot is reused. The error paths release it explicitly
instead, using the same helper the cleaner uses for descriptors. OWN is 
never
granted on that descriptor on these paths, so zeroing des0-des3 cannot race
with the DMA engine.

Concretely, v2:

  - keeps tx_q->cur_tx untouched while the context descriptor is filled 
and lets
    it advance together with the data descriptors;

  - moves first_entry past the context slot, so the error_dma_unmap 
unwind loop
    does not call stmmac_free_tx_buffer() on a slot that has no skb 
attached, and
    is_last_segment = CIRC_CNT(tx_q->cur_tx, first_entry, ...) == 1 
keeps the
    correct value for single descriptor frames;

  - updates tx_q->mss only after stmmac_set_tx_owner(mss_desc);

  - releases the context descriptor at the common "error:" label, which 
covers
    both the dma_map_single() of the linear part and the 
skb_frag_dma_map() of
    every fragment.

Nothing else changes. first_tx is sampled before the context descriptor is
allocated, so the tx_packets accounting used by the coalescing logic is
unaffected, and stmmac_tso_get_num_desc() already counts the context 
descriptor,
so the stmmac_tx_avail() check is unchanged as well. The
WARN_ON(tx_q->tx_skbuff[tx_q->cur_tx]) that followed the old cur_tx 
update is
dropped: it checked the first data slot, which the WARN_ON on
tx_q->tx_skbuff[entry] right after the first_entry assignment already 
covers.

v2 follows in a moment.

Regards,
Linkui

> 
> Regards,
> Lorenzo
> 
>>   	}
>>   
>>   	if (netif_msg_pktdata(priv)) {
>> -- 
>> 2.25.1
>>
>>