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

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

[Lorenzo Bianconi]

stmmac_tso_xmit() estimates the number of free TX descriptors required by
a TSO skb as:

	(skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1

which assumes the payload is split into TSO_MAX_BUFF_SIZE chunks. This
underestimates the descriptors actually consumed by stmmac_tso_allocator(),
since each fragment is mapped individually and so it needs at least one
descriptor regardless of its size. Moreover, one descriptor is used for
the L2/L3/L4 headers and, when the MSS changes, one more is consumed for
the MSS context descriptor.
For a highly fragmented TSO skb the check can therefore pass even when the
ring has too few free slots. stmmac_tso_allocator() then writes past the
available descriptors, overwriting descriptors still owned by the DMA
engine, corrupting the TX ring.
Add stmmac_tso_get_num_desc() to compute the exact number of descriptors
needed for the header, the linear payload and each fragment, plus the MSS
context descriptor when required, and use it in the availability check.

Fixes: f748be531d70 ("stmmac: support new GMAC4")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 ++++++++++++++++++-----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b..3566b33bc34e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4454,6 +4454,26 @@ static bool stmmac_tso_valid_packet(struct sk_buff *skb)
 	       header_len + gso_size < 16383;
 }
 
+static int stmmac_tso_get_num_desc(struct stmmac_tx_queue *tx_q,
+				   struct sk_buff *skb, u32 pay_len)
+{
+	int i, ndesc = 1;
+
+	/* head payload */
+	ndesc += DIV_ROUND_UP(pay_len, TSO_MAX_BUFF_SIZE);
+	/* frag payload */
+	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+
+		ndesc += DIV_ROUND_UP(skb_frag_size(frag),
+				      TSO_MAX_BUFF_SIZE);
+	}
+	/* MSS update requires a new descriptor */
+	ndesc += !!(skb_shinfo(skb)->gso_size != tx_q->mss);
+
+	return ndesc;
+}
+
 /**
  *  stmmac_tso_xmit - Tx entry point of the driver for oversized frames (TSO)
  *  @skb : the socket buffer
@@ -4497,10 +4517,10 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct stmmac_priv *priv = netdev_priv(dev);
 	unsigned int first_entry, entry, tx_packets;
 	struct stmmac_txq_stats *txq_stats;
+	int i, first_tx, nfrags, ndesc;
 	struct stmmac_tx_queue *tx_q;
 	bool set_ic, is_last_segment;
 	u32 pay_len, mss, queue;
-	int i, first_tx, nfrags;
 	u8 proto_hdr_len, hdr;
 	dma_addr_t des;
 
@@ -4513,14 +4533,15 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Compute header lengths */
 	proto_hdr_len = stmmac_tso_header_size(skb);
+	pay_len = skb_headlen(skb) - proto_hdr_len; /* no frags */
+
 	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));
@@ -4532,8 +4553,6 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}
 
-	pay_len = skb_headlen(skb) - proto_hdr_len; /* no frags */
-
 	mss = skb_shinfo(skb)->gso_size;
 
 	/* set new MSS value if needed */

---
base-commit: 38b6be101006d3e7af972999f45d4f1e8250587a
change-id: 20260907-stmmac-fix-tso-nfrags-check-21ecfa2ac05b

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24656b35350b1..3566b33bc34e5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4454,6 +4454,26 @@ static bool stmmac_tso_valid_packet(struct sk_buff *skb)
>  	       header_len + gso_size < 16383;
>  }
>  
> +static int stmmac_tso_get_num_desc(struct stmmac_tx_queue *tx_q,
> +				   struct sk_buff *skb, u32 pay_len)
> +{
> +	int i, ndesc = 1;
> +
> +	/* head payload */
> +	ndesc += DIV_ROUND_UP(pay_len, TSO_MAX_BUFF_SIZE);
> +	/* frag payload */
> +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> +		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> +
> +		ndesc += DIV_ROUND_UP(skb_frag_size(frag),
> +				      TSO_MAX_BUFF_SIZE);
> +	}
> +	/* MSS update requires a new descriptor */
> +	ndesc += !!(skb_shinfo(skb)->gso_size != tx_q->mss);

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

> +
> +	return ndesc;
> +}
> +
>  /**
>   *  stmmac_tso_xmit - Tx entry point of the driver for oversized frames (TSO)
>   *  @skb : the socket buffer
> @@ -4497,10 +4517,10 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	unsigned int first_entry, entry, tx_packets;
>  	struct stmmac_txq_stats *txq_stats;
> +	int i, first_tx, nfrags, ndesc;
>  	struct stmmac_tx_queue *tx_q;
>  	bool set_ic, is_last_segment;
>  	u32 pay_len, mss, queue;
> -	int i, first_tx, nfrags;
>  	u8 proto_hdr_len, hdr;
>  	dma_addr_t des;
>  
> @@ -4513,14 +4533,15 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	/* Compute header lengths */
>  	proto_hdr_len = stmmac_tso_header_size(skb);
> +	pay_len = skb_headlen(skb) - proto_hdr_len; /* no frags */

[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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 10132 bytes --]

> 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.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 24656b35350b1..3566b33bc34e5 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -4454,6 +4454,26 @@ static bool stmmac_tso_valid_packet(struct sk_buff *skb)
> >  	       header_len + gso_size < 16383;
> >  }
> >  
> > +static int stmmac_tso_get_num_desc(struct stmmac_tx_queue *tx_q,
> > +				   struct sk_buff *skb, u32 pay_len)
> > +{
> > +	int i, ndesc = 1;
> > +
> > +	/* head payload */
> > +	ndesc += DIV_ROUND_UP(pay_len, TSO_MAX_BUFF_SIZE);
> > +	/* frag payload */
> > +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> > +		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> > +
> > +		ndesc += DIV_ROUND_UP(skb_frag_size(frag),
> > +				      TSO_MAX_BUFF_SIZE);
> > +	}
> > +	/* MSS update requires a new descriptor */
> > +	ndesc += !!(skb_shinfo(skb)->gso_size != tx_q->mss);
> 
> [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?

As pointed out by Sashiko, this issue is not introduced by this patch and it
can be fixed with a dedicated patch.

> 
> > +
> > +	return ndesc;
> > +}
> > +
> >  /**
> >   *  stmmac_tso_xmit - Tx entry point of the driver for oversized frames (TSO)
> >   *  @skb : the socket buffer
> > @@ -4497,10 +4517,10 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >  	unsigned int first_entry, entry, tx_packets;
> >  	struct stmmac_txq_stats *txq_stats;
> > +	int i, first_tx, nfrags, ndesc;
> >  	struct stmmac_tx_queue *tx_q;
> >  	bool set_ic, is_last_segment;
> >  	u32 pay_len, mss, queue;
> > -	int i, first_tx, nfrags;
> >  	u8 proto_hdr_len, hdr;
> >  	dma_addr_t des;
> >  
> > @@ -4513,14 +4533,15 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> >  
> >  	/* Compute header lengths */
> >  	proto_hdr_len = stmmac_tso_header_size(skb);
> > +	pay_len = skb_headlen(skb) - proto_hdr_len; /* no frags */
> 
> [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?

As pointed out by Sashiko, this issue is not introduced by this patch and it
can be fixed with a dedicated patch.

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

The goal of this patch is to fix the memory corruption triggered by a TSO skb
that does not fit in the tx queue available memory space.
The case described above will be managed by the new condition added at the
beginning of stmmac_tso_xmit(). I think we can change the stop/wake tx queue
watermark with a dedicated patch that we do not need to backport (I guess this
is mostly an optimization). However, if you prefer to have it in a single patch,
I will add it in this one.

> 
> >  @@ -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?

I guess this corner case has not been introduced by this patch since the same
approach is present in stmmac_xmit(). I think it should be addressed with a
dedicated patch. Moreover, is the corner case fixed by the ndo_tx_timeout
callback?

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 07 Sep 2026 23:46:45 +0200 you wrote:
> stmmac_tso_xmit() estimates the number of free TX descriptors required by
> a TSO skb as:
> 
> 	(skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1
> 
> which assumes the payload is split into TSO_MAX_BUFF_SIZE chunks. This
> underestimates the descriptors actually consumed by stmmac_tso_allocator(),
> since each fragment is mapped individually and so it needs at least one
> descriptor regardless of its size. Moreover, one descriptor is used for
> the L2/L3/L4 headers and, when the MSS changes, one more is consumed for
> the MSS context descriptor.
> For a highly fragmented TSO skb the check can therefore pass even when the
> ring has too few free slots. stmmac_tso_allocator() then writes past the
> available descriptors, overwriting descriptors still owned by the DMA
> engine, corrupting the TX ring.
> Add stmmac_tso_get_num_desc() to compute the exact number of descriptors
> needed for the header, the linear payload and each fragment, plus the MSS
> context descriptor when required, and use it in the availability check.
> 
> [...]

Here is the summary with links:
  - [net] net: stmmac: fix TX descriptor availability check for TSO traffic
    https://git.kernel.org/netdev/net/c/5e38d732ec67

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html