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

[Linkui Xiao <xiaolinkui@126.com>]

 > [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