[PATCH net-next] net: stmmac: add XDP multi-buff support for TX side

[PATCH net-next] net: stmmac: add XDP multi-buff support for TX side

[Lorenzo Bianconi]

Extend stmmac_xdp_xmit_xdpf() to transmit XDP frames with fragments
(multi-buff). Each buffer, i.e. the frame head and every frag, is mapped
and programmed into a dedicated TX descriptor.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 123 ++++++++++++++--------
 1 file changed, 80 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bf9e7e4cb1c3..41ccde6371ec 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5239,77 +5239,114 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
 static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
 				struct xdp_frame *xdpf, bool dma_map)
 {
-	struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[queue];
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_frame(xdpf);
 	struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
 	bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;
+	unsigned int first_entry = tx_q->cur_tx;
 	unsigned int entry = tx_q->cur_tx;
-	enum stmmac_txbuf_type buf_type;
-	struct dma_desc *tx_desc;
-	dma_addr_t dma_addr;
-	bool set_ic;
+	int i = 0, num_frames = 1;
+	struct dma_desc *desc;
 
-	if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
+	if (unlikely(xdp_frame_has_frags(xdpf)))
+		num_frames += sinfo->nr_frags;
+
+	if (stmmac_tx_avail(priv, queue) < num_frames)
 		return STMMAC_XDP_CONSUMED;
 
 	if (priv->est && priv->est->enable &&
 	    priv->est->max_sdu[queue] &&
-	    xdpf->len > priv->est->max_sdu[queue]) {
+	    xdp_get_frame_len(xdpf) > priv->est->max_sdu[queue]) {
 		priv->xstats.max_sdu_txq_drop[queue]++;
 		return STMMAC_XDP_CONSUMED;
 	}
 
-	tx_desc = stmmac_get_tx_desc(priv, tx_q, entry);
-	if (dma_map) {
-		dma_addr = dma_map_single(priv->device, xdpf->data,
-					  xdpf->len, DMA_TO_DEVICE);
-		if (dma_mapping_error(priv->device, dma_addr))
-			return STMMAC_XDP_CONSUMED;
-
-		buf_type = STMMAC_TXBUF_T_XDP_NDO;
-	} else {
-		struct page *page = virt_to_page(xdpf->data);
-
-		dma_addr = page_pool_get_dma_addr(page) + sizeof(*xdpf) +
-			   xdpf->headroom;
-		dma_sync_single_for_device(priv->device, dma_addr,
-					   xdpf->len, DMA_BIDIRECTIONAL);
+	while (true) {
+		skb_frag_t *frag = i ? &sinfo->frags[i - 1] : NULL;
+		int len = frag ? skb_frag_size(frag) : xdpf->len;
+		bool last_frame = i == num_frames - 1;
+		enum stmmac_txbuf_type buf_type;
+		dma_addr_t dma_addr;
 
-		buf_type = STMMAC_TXBUF_T_XDP_TX;
-	}
+		desc = stmmac_get_tx_desc(priv, tx_q, entry);
+		if (dma_map) {
+			if (frag)
+				dma_addr = skb_frag_dma_map(priv->device,
+							    frag, 0, len,
+							    DMA_TO_DEVICE);
+			else
+				dma_addr = dma_map_single(priv->device,
+							  xdpf->data, len,
+							  DMA_TO_DEVICE);
+			if (dma_mapping_error(priv->device, dma_addr))
+				goto error_dma_unmap;
 
-	stmmac_set_tx_dma_entry(tx_q, entry, buf_type, dma_addr, xdpf->len,
-				false);
-	stmmac_set_tx_dma_last_segment(tx_q, entry);
+			buf_type = STMMAC_TXBUF_T_XDP_NDO;
+		} else {
+			struct page *page;
 
-	tx_q->xdpf[entry] = xdpf;
+			page = frag ? skb_frag_page(frag)
+				    : virt_to_page(xdpf->data);
+			dma_addr = page_pool_get_dma_addr(page);
+			if (frag)
+				dma_addr += skb_frag_off(frag);
+			else
+				dma_addr += sizeof(*xdpf) + xdpf->headroom;
+			dma_sync_single_for_device(priv->device, dma_addr,
+						   len, DMA_BIDIRECTIONAL);
+			buf_type = STMMAC_TXBUF_T_XDP_TX;
+		}
 
-	stmmac_set_desc_addr(priv, tx_desc, dma_addr);
+		stmmac_set_tx_dma_entry(tx_q, entry, buf_type, dma_addr, len,
+					dma_map && frag);
+		stmmac_set_desc_addr(priv, desc, dma_addr);
+		stmmac_prepare_tx_desc(priv, desc, !i, len, csum,
+				       priv->descriptor_mode, !!i, last_frame,
+				       xdp_get_frame_len(xdpf));
+		tx_q->xdpf[entry] = last_frame ? xdpf : NULL;
+		if (last_frame) {
+			stmmac_set_tx_dma_last_segment(tx_q, entry);
+			break;
+		}
 
-	stmmac_prepare_tx_desc(priv, tx_desc, 1, xdpf->len,
-			       csum, priv->descriptor_mode, true, true,
-			       xdpf->len);
+		entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+		i++;
+	}
+	tx_q->tx_count_frames += num_frames;
 
-	tx_q->tx_count_frames++;
+	if (!(tx_q->tx_count_frames % priv->tx_coal_frames[queue])) {
+		struct stmmac_txq_stats *txq_stats;
 
-	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
-		set_ic = true;
-	else
-		set_ic = false;
-
-	if (set_ic) {
+		desc = stmmac_get_tx_desc(priv, tx_q, entry);
+		stmmac_set_tx_ic(priv, desc);
 		tx_q->tx_count_frames = 0;
-		stmmac_set_tx_ic(priv, tx_desc);
+
+		txq_stats = &priv->xstats.txq_stats[queue];
 		u64_stats_update_begin(&txq_stats->q_syncp);
 		u64_stats_inc(&txq_stats->q.tx_set_ic_bit);
 		u64_stats_update_end(&txq_stats->q_syncp);
 	}
 
+	/* Set the OWN bit on the first descriptor now that all descriptors
+	 * for this skb are populated.
+	 */
+	desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
+	stmmac_set_tx_owner(priv, desc);
+	tx_q->cur_tx = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
 	stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
 
-	entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
-	tx_q->cur_tx = entry;
-
 	return STMMAC_XDP_TX;
+
+error_dma_unmap:
+	while (first_entry != entry) {
+		desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
+		stmmac_release_tx_desc(priv, desc, priv->descriptor_mode);
+		stmmac_free_tx_buffer(priv, &priv->dma_conf, queue,
+				      first_entry);
+		first_entry = STMMAC_NEXT_ENTRY(first_entry,
+						priv->dma_conf.dma_tx_size);
+	}
+
+	return STMMAC_XDP_CONSUMED;
 }
 
 static int stmmac_xdp_get_tx_queue(struct stmmac_priv *priv,

---
base-commit: 944ae66642b726bd6b25ae71b1e9ff88a0e0bdb0
change-id: 20260922-b4-stmmac-xmit-mb-bb78e0684cf4

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

Re: [PATCH net-next] net: stmmac: add XDP multi-buff support for TX side

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] stmmac: exact-match coalescing logic skips TX completion interrupts
- [High] stmmac: missing memory barrier before setting TX OWN bit
- [Medium] stmmac: XDP frame check removes SKB TX threshold reservation
--

commit 4955fe9fec58eed4009939eaf9ed4fcf2a742ee7
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

net: stmmac: add XDP multi-buff support for TX side

This patch extends stmmac_xdp_xmit_xdpf to transmit XDP frames with fragments
by mapping and programming each buffer into a dedicated TX descriptor.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c31..41ccde6371ecd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5239,77 +5239,114 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
[ ... ]
> -	if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
> +	if (stmmac_tx_avail(priv, queue) < num_frames)

[Severity: Medium]
Could this change lead to TX ring starvation for standard SKB traffic?

The STMMAC_TX_THRESH check was designed to reserve a portion of the shared TX
ring for standard SKB traffic. By replacing the threshold with a check against
just num_frames, heavy XDP loads could exhaust all available TX descriptors,
potentially leaving 0 descriptors for SKBs and monopolizing the TX ring.

>  		return STMMAC_XDP_CONSUMED;
[ ... ]
> +	tx_q->tx_count_frames += num_frames;
>  
> -	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
> -		set_ic = true;
> -	else
> -		set_ic = false;
> -
> -	if (set_ic) {
> +	if (!(tx_q->tx_count_frames % priv->tx_coal_frames[queue])) {

[Severity: High]
Will this exact-match coalescing logic reliably trigger TX completion
interrupts?

When transmitting XDP multi-buffer frames, tx_count_frames increases by
num_frames, which can be greater than 1. The modulo condition relies on an
exact match to 0 to trigger the Interrupt on Completion (IC) bit.

If num_frames causes the counter to overshoot a multiple of tx_coal_frames
(for example, jumping from 24 to 26 when the threshold is 25), the IC bit
will not be set. This could indefinitely delay TX completion handling and
exhaust the TX ring.

> +		struct stmmac_txq_stats *txq_stats;
>  
> +		desc = stmmac_get_tx_desc(priv, tx_q, entry);
> +		stmmac_set_tx_ic(priv, desc);
>  		tx_q->tx_count_frames = 0;
> -		stmmac_set_tx_ic(priv, tx_desc);
> +
> +		txq_stats = &priv->xstats.txq_stats[queue];
>  		u64_stats_update_begin(&txq_stats->q_syncp);
>  		u64_stats_inc(&txq_stats->q.tx_set_ic_bit);
>  		u64_stats_update_end(&txq_stats->q_syncp);
>  	}
>  
> +	/* Set the OWN bit on the first descriptor now that all descriptors
> +	 * for this skb are populated.
> +	 */
> +	desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
> +	stmmac_set_tx_owner(priv, desc);

[Severity: High]
Is a dma_wmb() needed here before setting the TX OWN bit on the first
descriptor?

The new logic loops over fragments, calling stmmac_prepare_tx_desc() with
tx_own=0 for the first descriptor. This effectively bypasses the internal
dma_wmb() inside stmmac_prepare_tx_desc(), which typically requires both
is_fs and tx_own to be true.

Without a preceding dma_wmb() before manually setting the OWN bit here,
the CPU might reorder the write to the first descriptor's OWN bit before
the writes that populated the fragment descriptors. The DMA engine could
then process stale or uninitialized memory.

> +	tx_q->cur_tx = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
>  	stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
>  
> -	entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
> -	tx_q->cur_tx = entry;
> -
>  	return STMMAC_XDP_TX;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-b4-stmmac-xmit-mb-v1-1-a71a1c522142@oss.qualcomm.com?part=1

Re: [PATCH net-next] net: stmmac: add XDP multi-buff support for TX side

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] stmmac: exact-match coalescing logic skips TX completion interrupts
> - [High] stmmac: missing memory barrier before setting TX OWN bit
> - [Medium] stmmac: XDP frame check removes SKB TX threshold reservation
> --
> 
> commit 4955fe9fec58eed4009939eaf9ed4fcf2a742ee7
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> net: stmmac: add XDP multi-buff support for TX side
> 
> This patch extends stmmac_xdp_xmit_xdpf to transmit XDP frames with fragments
> by mapping and programming each buffer into a dedicated TX descriptor.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index bf9e7e4cb1c31..41ccde6371ecd 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5239,77 +5239,114 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> [ ... ]
> > -	if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
> > +	if (stmmac_tx_avail(priv, queue) < num_frames)
> 
> [Severity: Medium]
> Could this change lead to TX ring starvation for standard SKB traffic?
> 
> The STMMAC_TX_THRESH check was designed to reserve a portion of the shared TX
> ring for standard SKB traffic. By replacing the threshold with a check against
> just num_frames, heavy XDP loads could exhaust all available TX descriptors,
> potentially leaving 0 descriptors for SKBs and monopolizing the TX ring.

ack, I will fix it in v2.

> 
> >  		return STMMAC_XDP_CONSUMED;
> [ ... ]
> > +	tx_q->tx_count_frames += num_frames;
> >  
> > -	if (tx_q->tx_count_frames % priv->tx_coal_frames[queue] == 0)
> > -		set_ic = true;
> > -	else
> > -		set_ic = false;
> > -
> > -	if (set_ic) {
> > +	if (!(tx_q->tx_count_frames % priv->tx_coal_frames[queue])) {
> 
> [Severity: High]
> Will this exact-match coalescing logic reliably trigger TX completion
> interrupts?
> 
> When transmitting XDP multi-buffer frames, tx_count_frames increases by
> num_frames, which can be greater than 1. The modulo condition relies on an
> exact match to 0 to trigger the Interrupt on Completion (IC) bit.
> 
> If num_frames causes the counter to overshoot a multiple of tx_coal_frames
> (for example, jumping from 24 to 26 when the threshold is 25), the IC bit
> will not be set. This could indefinitely delay TX completion handling and
> exhaust the TX ring.

ack, I will fix it in v2.

> 
> > +		struct stmmac_txq_stats *txq_stats;
> >  
> > +		desc = stmmac_get_tx_desc(priv, tx_q, entry);
> > +		stmmac_set_tx_ic(priv, desc);
> >  		tx_q->tx_count_frames = 0;
> > -		stmmac_set_tx_ic(priv, tx_desc);
> > +
> > +		txq_stats = &priv->xstats.txq_stats[queue];
> >  		u64_stats_update_begin(&txq_stats->q_syncp);
> >  		u64_stats_inc(&txq_stats->q.tx_set_ic_bit);
> >  		u64_stats_update_end(&txq_stats->q_syncp);
> >  	}
> >  
> > +	/* Set the OWN bit on the first descriptor now that all descriptors
> > +	 * for this skb are populated.
> > +	 */
> > +	desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
> > +	stmmac_set_tx_owner(priv, desc);
> 
> [Severity: High]
> Is a dma_wmb() needed here before setting the TX OWN bit on the first
> descriptor?
> 
> The new logic loops over fragments, calling stmmac_prepare_tx_desc() with
> tx_own=0 for the first descriptor. This effectively bypasses the internal
> dma_wmb() inside stmmac_prepare_tx_desc(), which typically requires both
> is_fs and tx_own to be true.
> 
> Without a preceding dma_wmb() before manually setting the OWN bit here,
> the CPU might reorder the write to the first descriptor's OWN bit before
> the writes that populated the fragment descriptors. The DMA engine could
> then process stale or uninitialized memory.

I do not think we need dma_wmb() here since this is the same logic used in
stmmac_xmit() and the barrier is already in stmmac_flush_tx_descriptors().

Regards,
Lorenzo

> 
> > +	tx_q->cur_tx = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
> >  	stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
> >  
> > -	entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
> > -	tx_q->cur_tx = entry;
> > -
> >  	return STMMAC_XDP_TX;
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260923-b4-stmmac-xmit-mb-v1-1-a71a1c522142@oss.qualcomm.com?part=1

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

Re: [PATCH net-next] net: stmmac: add XDP multi-buff support for TX side

[netdev-bot+sashiko]

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

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

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 says it adds XDP multi-buff TX support, but no
  multi-buff frame can reach the new code.
- [Medium] In stmmac_xdp_xmit_xdpf(), the ring-availability check changed
  from `stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv)`…
- [Medium] stmmac_xdp_xmit_xdpf() now does `tx_q->tx_count_frames +=
  num_frames` but still sets the IC bit only when…
- [Medium] The patch changes how the XDP TX path hands descriptors to the
  DMA, and in doing so drops the dma_wmb() before the OWN bit is set.
- [Low] The new comment in stmmac_xdp_xmit_xdpf() was copied word for
  word from stmmac_xmit() and says the OWN bit is set 'now that all…

Pre-existing issues:
- [High] stmmac_xdp_xmit_xdpf() computes `tx_q->tx_count_frames %
  priv->tx_coal_frames[queue]` without checking that…

--

net: stmmac: add XDP multi-buff support for TX side

This commit extends stmmac_xdp_xmit_xdpf() to transmit XDP frames with
fragments. The frame head and each frag are mapped and programmed into
their own TX descriptor. The OWN bit on the first descriptor is set once
all descriptors are populated.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c31..41ccde6371ecd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5239,77 +5239,114 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
>  static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
>  				struct xdp_frame *xdpf, bool dma_map)
>  {

[ ... ]

> -	if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
> +	if (unlikely(xdp_frame_has_frags(xdpf)))
> +		num_frames += sinfo->nr_frags;
> +

[Severity: Medium]
After this patch, can a frame with frags actually reach this code?

stmmac_xdp_set_prog() still registers the redirect target without SG
support:

drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
    ...
	if (prog)
		xdp_features_set_redirect_target(dev, false);
    ...
}

The probe-time mask in stmmac_dvr_probe() has neither
NETDEV_XDP_ACT_NDO_XMIT_SG nor NETDEV_XDP_ACT_RX_SG:

	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
			     NETDEV_XDP_ACT_XSK_ZEROCOPY;

Because of that, __xdp_enqueue() and is_valid_dst() in kernel/bpf/devmap.c
drop frag frames before ndo_xdp_xmit is called:

	if (unlikely(!(dev->xdp_features & NETDEV_XDP_ACT_NDO_XMIT_SG) &&
		     xdp_frame_has_frags(xdpf)))
		return -EOPNOTSUPP;

The XDP_TX side (stmmac_xdp_xmit_back()->stmmac_xdp_xmit_xdpf()) is
blocked too. The stmmac RX path never builds multi-buff xdp_buffs; there
is no xdp_buff_add_frag() or xdp_buff_set_frags() in the driver. Also,
stmmac_xdp_set_prog() still refuses XDP when mtu > ETH_DATA_LEN.

So none of these can run: the skb_frag_dma_map() branch, the page_pool frag
branch, the multi-descriptor loop, and the error_dma_unmap unwind.

Should this patch also advertise NETDEV_XDP_ACT_NDO_XMIT_SG (and build
multi-buff frames on RX), so that the multi-buff TX support described in
the commit message can be used?

> +	if (stmmac_tx_avail(priv, queue) < num_frames)
>  		return STMMAC_XDP_CONSUMED;

[Severity: Medium]
Is the STMMAC_TX_THRESH() reserve meant to go away for linear frames too?

Before, XDP stopped using the ring once fewer than dma_tx_size / 4
descriptors were free. Now a single-buffer frame (num_frames == 1) can take
every free slot of the TX ring it shares with the stack. The XDP path never
stops the netdev TX queue.

stmmac_tx_clean() wakes the stack queue only above the threshold:

	    stmmac_tx_avail(priv, queue) > STMMAC_TX_THRESH(priv)) {

Once XDP fills the ring, the next stmmac_xmit() or stmmac_tso_xmit() call
with the queue still awake takes the hard error path:

			/* This is a hard error, log it. */
			netdev_err(priv->dev,
				   "%s: Tx Ring full when queue awake\n",
				   __func__);
		}
		return NETDEV_TX_BUSY;

Can sustained XDP_TX or redirect traffic starve stack traffic this way?

The XSK zero-copy path in stmmac_xdp_xmit_zc() uses the same ring and keeps
a reserve for this reason:

		/* We are sharing with slow path and stop XSK TX desc submission when
		 * available TX ring is less than threshold.
		 */
		if (unlikely(stmmac_tx_avail(priv, queue) < STMMAC_TX_XSK_AVAIL) ||

Would a check like avail < max(STMMAC_TX_THRESH(priv), num_frames) keep the
old behaviour and still account for frags?

[ ... ]

> +		stmmac_set_desc_addr(priv, desc, dma_addr);
> +		stmmac_prepare_tx_desc(priv, desc, !i, len, csum,
> +				       priv->descriptor_mode, !!i, last_frame,
> +				       xdp_get_frame_len(xdpf));

[ ... ]

> -	stmmac_prepare_tx_desc(priv, tx_desc, 1, xdpf->len,
> -			       csum, priv->descriptor_mode, true, true,
> -			       xdpf->len);
> +		entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
> +		i++;
> +	}
> +	tx_q->tx_count_frames += num_frames;
> 
> -	tx_q->tx_count_frames++;
> +	if (!(tx_q->tx_count_frames % priv->tx_coal_frames[queue])) {

[Severity: Medium]
Can tx_count_frames step over every multiple of tx_coal_frames here?

tx_count_frames now grows by num_frames, so the exact-multiple test can be
skipped. With the default tx_coal_frames of 25, a step from 24 to 26 misses
25. With an even threshold such as 64, one single-buffer frame followed by
2-descriptor frames keeps the counter odd, so the IC bit is never set.

stmmac_xmit() and stmmac_tso_xmit() already handle batched increments by
checking whether the threshold was crossed:

	else if (tx_packets > priv->tx_coal_frames[queue])
		set_ic = true;
	else if ((tx_q->tx_count_frames %
		  priv->tx_coal_frames[queue]) < tx_packets)
		set_ic = true;

Without a check like that, TX completion reclaim depends on the hrtimer.
__stmmac_set_coalesce() accepts tx-usecs 0 when tx-frames is non-zero, and
in that case stmmac_tx_timer_arm() returns early:

	if (!tx_coal_timer)
		return;

Would completions then stall until something else triggers TX cleaning?

[Severity: High]
This isn't a bug introduced by this patch, but since this line is being
rewritten: can priv->tx_coal_frames[queue] be zero here?

__stmmac_set_coalesce() rejects only the case where both values are zero:

	if ((ec->tx_coalesce_usecs == 0) &&
	    (ec->tx_max_coalesced_frames == 0))
		return -EINVAL;

So "ethtool -C ethX tx-usecs 1000 tx-frames 0" stores tx_coal_frames[queue]
= 0. stmmac_xmit(), stmmac_tso_xmit() and stmmac_xdp_xmit_zc() all guard
the modulo:

	else if (!priv->tx_coal_frames[queue])
		set_ic = false;

With that setting and an XDP program attached, each XDP_TX verdict
(stmmac_xdp_xmit_back()) and each ndo_xdp_xmit redirect (stmmac_xdp_xmit())
would divide by zero in softirq context. That traps on x86.

The old code had the same unguarded "% priv->tx_coal_frames[queue] == 0".
Could a zero check be added while this code is being reworked?

[ ... ]

> +	/* Set the OWN bit on the first descriptor now that all descriptors
> +	 * for this skb are populated.
> +	 */

[Severity: Low]
This isn't a bug, but this comment was copied from stmmac_xmit() and talks
about an skb. This function sends an xdp_frame and its frags.

> +	desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
> +	stmmac_set_tx_owner(priv, desc);

[Severity: Medium]
Does this need a dma_wmb() before stmmac_set_tx_owner()?

The old code called stmmac_prepare_tx_desc() with is_fs = 1 and
tx_own = true. In that case every backend issues a barrier before writing
the OWN word. For example, enh_desc_prepare_tx_desc():

	if (is_fs && tx_own)
		/* When the own bit, for the first frame, has to be set, all
		 * descriptors for the same frame has to be set before, to
		 * avoid race condition.
		 */
		dma_wmb();

The loop now passes tx_own = !!i, which is false for the first descriptor,
so that barrier never runs. The set_tx_owner callbacks that now set OWN are
plain read-modify-writes, for example:

static void dwmac4_set_tx_owner(struct dma_desc *p)
{
	p->des3 |= cpu_to_le32(TDES3_OWN);
}

The common single-buffer XDP_TX and ndo_xdp_xmit case is affected as well.
On dwmac4 and xgmac, the tail pointer update after wmb() in
stmmac_flush_tx_descriptors() hides the missing barrier. dwmac100, dwmac1000
and sun8i have no set_tx_tail_ptr, so a running TX DMA fetches the
descriptor as soon as it sees OWN.

On weakly ordered architectures, can the device see OWN before the buffer
address and length stores of the same descriptor? For multi-buff, can it
see OWN before the following descriptors are written? It could then
transmit from a stale address or length.

stmmac_xmit() has the same pattern, but for the XDP path this is new.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-b4-stmmac-xmit-mb-v1-1-a71a1c522142%40oss.qualcomm.com