Re: [PATCH net] net: stmmac: fix TX descriptor availability check for TSO traffic

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0 (1)

- [High] stmmac_tso_xmit() now demands the exact descriptor count
  returned by stmmac_tso_get_num_desc() (stmmac_main.c:4543-4544) but…

Pre-existing issues:
- [High] proto_hdr_len in stmmac_tso_xmit() is declared as u8
  (stmmac_main.c:4524) but is assigned the size_t result of…
- [Low] The stmmac_tso_xmit() kernel-doc states "mss is fixed when enable
  tso, so w/o programming the TDES3 ctx field." (stmmac_main.c:4512),…

--

net: stmmac: fix TX descriptor availability check for TSO traffic

This replaces the (skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1
estimate in stmmac_tso_xmit() with stmmac_tso_get_num_desc(), which counts
the header descriptor, the linear payload, every fragment and the MSS
context descriptor. The new count is then used for the free descriptor
check before the ring is programmed.

A few questions below.

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but the new
comment here now contradicts the stmmac_tso_xmit() kernel-doc that follows
a few lines below, which still says:

	 * mss is fixed when enable tso, so w/o programming the TDES3 ctx field.

stmmac_tso_xmit() does program a context descriptor whenever the skb MSS
differs from the cached queue MSS:

	if (mss != tx_q->mss) {
		...
		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);

Would it make sense to drop or update that kernel-doc sentence while
touching this area?

[Severity: High]
This isn't a bug introduced by this patch, but the new pay_len computation
inherits an existing truncation. proto_hdr_len is declared u8 while
stmmac_tso_header_size() returns a size_t (skb_transport_offset() +
sizeof(struct udphdr), or skb_tcp_all_headers()), and
stmmac_tso_valid_packet() explicitly admits headers up to 1023 bytes into
the hardware TSO path:

	return header_len <= 1023 && gso_size >= 64 &&
	       header_len + gso_size < 16383;

For a header of 256..1023 bytes the store wraps modulo 256 (486 becomes
230, 256 becomes 0). The truncated value then defines the hardware
contract, both for the payload start address:

	stmmac_tso_allocator(priv, &entry, des + proto_hdr_len, pay_len,
			     (nfrags == 0), queue);

and for the descriptor lengths:

	stmmac_prepare_tso_tx_desc(priv, first, 1, proto_hdr_len, 0, 1,
				   is_last_segment, hdr / 4,
				   skb->len - proto_hdr_len);

so the tail of the real header is handed to the engine as TCP/UDP payload
while TDES3 declares more payload than the following descriptors describe
(a header of exactly 256 also yields a zero length buffer1 with TSE set).
A TCP over IPv6 socket carrying a few hundred bytes of sticky
destination/hop-by-hop options makes skb_tcp_all_headers() exceed 255 while
staying under 1023, so such an skb reaches stmmac_tso_xmit().

For the new pay_len the error biases upward, so the ring accounting stays
safe, but should proto_hdr_len be widened to unsigned int (or size_t) since
this patch now builds the descriptor count on it too?

 	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
 		hdr = sizeof(struct udphdr);
 	else
 		hdr = tcp_hdrlen(skb);
 
-	/* Desc availability based on threshold should be enough safe */
-	if (unlikely(stmmac_tx_avail(priv, queue) <
-		(((skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1)))) {
+	ndesc = stmmac_tso_get_num_desc(tx_q, skb, pay_len);
+	if (unlikely(stmmac_tx_avail(priv, queue) < ndesc)) {
 		if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
 			netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
 								queue));

[Severity: High]
The new requirement is exact, but the TX flow control watermarks were left
unchanged. Can this make the "Tx Ring full when queue awake" branch
reachable for ordinary TSO traffic?

stmmac_tso_get_num_desc() returns at least nfrags + 2 for any TSO skb that
has a linear payload, so with MAX_SKB_FRAGS = 17 plus an MSS change it asks
for 20 descriptors, and up to roughly 23 when individual frags exceed
TSO_MAX_BUFF_SIZE.

The tail of stmmac_tso_xmit() still stops the queue based on the old
assumption:

	if (unlikely(stmmac_tx_avail(priv, queue) <= (MAX_SKB_FRAGS + 1))) {
		netif_dbg(priv, hw, priv->dev, "%s: stop transmitted packets\n",
			  __func__);
		netif_tx_stop_queue(netdev_get_tx_queue(priv->dev, queue));
	}

so the queue is deliberately left awake with as few as 19 free slots, while
the next TSO skb may need more. Does that leave a window where the stack
submits an skb that does not fit, hitting the un-ratelimited netdev_err()
in the ndo_start_xmit hot path together with netif_tx_stop_queue() and a
NETDEV_TX_BUSY requeue?

The non-TSO path keeps the invariant the watermark was designed for,
stmmac_xmit() needs nfrags + 1, which is always satisfied when the queue is
awake:

	if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {

The only place the queue is re-armed is stmmac_tx_clean():

	if (unlikely(netif_tx_queue_stopped(netdev_get_tx_queue(priv->dev, queue))) &&
	    stmmac_tx_avail(priv, queue) > STMMAC_TX_THRESH(priv)) {
		...
		netif_tx_wake_queue(netdev_get_tx_queue(priv->dev, queue));

STMMAC_TX_THRESH() is dma_tx_size / 4, which is 16 on the 64 entry ring
stmmac_set_ringparam() accepts (DMA_MIN_TX_SIZE in common.h). On such a
ring the wake threshold is itself below the new requirement, so can the
queue be woken while the skb still does not fit, producing a repeating
stop/wake/TX_BUSY cycle with repeated error logging?

Should the stop and wake watermarks be raised to the worst case TSO
descriptor count, or the skb linearized/pushed to software GSO, as part of
this change?

 @@ -4532,8 +4553,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}

One more on this branch: it sets XOFF without re-checking availability and
without arming a wake source. If the ring drains concurrently
(dirty_tx == cur_tx, no further TX completion and no NAPI run), does the
XOFF state persist until dev_watchdog fires ndo_tx_timeout?

Since the patch is tagged for stable, would it be worth describing the new
NETDEV_TX_BUSY and error logging behaviour in the commit message?

 
-	pay_len = skb_headlen(skb) - proto_hdr_len; /* no frags */
-
 	mss = skb_shinfo(skb)->gso_size;
 
 	/* set new MSS value if needed */

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907-stmmac-fix-tso-nfrags-check-v1-1-328459906cdb%40oss.qualcomm.com