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

[PATCH net-next v3] 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>
---
Changes in v3:
- Advertise NETDEV_XDP_ACT_NDO_XMIT_SG capability.
- Add missing dma_wmb() before setting OWN bit of the first descriptor.
- Link to v2: https://lore.kernel.org/r/20260924-b4-stmmac-xmit-mb-v2-1-003347b7bc25@oss.qualcomm.com

Changes in v2:
- Fix Tx coalescing logic in stmmac_xdp_xmit_xdpf()
- Consider the max between STMMAC_TX_THRESH() and num_frames as
  threshold in stmmac_xdp_xmit_xdpf().
- Link to v1: https://lore.kernel.org/r/20260923-b4-stmmac-xmit-mb-v1-1-a71a1c522142@oss.qualcomm.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 127 ++++++++++++++--------
 drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c  |   2 +-
 2 files changed, 85 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3ad9252bf6ae..f1e9ac2f9859 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5241,77 +5241,118 @@ 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 txq_thr = STMMAC_TX_THRESH(priv);
+	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;
+	unsigned int num_frames = 1;
+	struct dma_desc *desc;
+	int i = 0;
 
-	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) < max(num_frames, txq_thr))
 		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);
-
-		buf_type = STMMAC_TXBUF_T_XDP_TX;
-	}
+	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;
 
-	stmmac_set_tx_dma_entry(tx_q, entry, buf_type, dma_addr, xdpf->len,
-				false);
-	stmmac_set_tx_dma_last_segment(tx_q, entry);
+		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;
 
-	tx_q->xdpf[entry] = xdpf;
+			buf_type = STMMAC_TXBUF_T_XDP_NDO;
+		} else {
+			struct page *page;
 
-	stmmac_set_desc_addr(priv, tx_desc, dma_addr);
+			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_prepare_tx_desc(priv, tx_desc, 1, xdpf->len,
-			       csum, priv->descriptor_mode, true, true,
-			       xdpf->len);
+		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;
+		}
 
-	tx_q->tx_count_frames++;
+		entry = STMMAC_NEXT_ENTRY(entry, priv->dma_conf.dma_tx_size);
+		i++;
+	}
+	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 (priv->tx_coal_frames[queue] &&
+	    (tx_q->tx_count_frames % priv->tx_coal_frames[queue]) < num_frames) {
+		struct stmmac_txq_stats *txq_stats;
 
-	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 xdp_frame are populated.
+	 */
+	desc = stmmac_get_tx_desc(priv, tx_q, first_entry);
+	dma_wmb();
+	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,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
index d7e4db7224b0..9af9e0cb366c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
@@ -135,7 +135,7 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
 		stmmac_xdp_open(dev);
 
 	if (prog)
-		xdp_features_set_redirect_target(dev, false);
+		xdp_features_set_redirect_target(dev, true);
 
 	return 0;
 }

---
base-commit: 42a9fb3382fc2573e92f41d203b095d9a372cfc9
change-id: 20260922-b4-stmmac-xmit-mb-bb78e0684cf4

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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 patch fixes a divide-by-zero crash in the XDP TX path
  without saying so and without a Fixes: tag.
- [Low] In stmmac_xdp_xmit_xdpf(), the new coalescing check reads
  priv->tx_coal_frames[queue] twice with plain loads: once for the zero…

--

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

This commit extends stmmac_xdp_xmit_xdpf() so it can transmit XDP frames
that carry fragments. The frame head and each frag get their own TX
descriptor. The driver now also advertises NDO_XMIT_SG for the redirect
target.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3ad9252bf6aea..f1e9ac2f98598 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5241,77 +5241,118 @@ 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)
>  {

[ ... ]

> +	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 (priv->tx_coal_frames[queue] &&
> +	    (tx_q->tx_count_frames % priv->tx_coal_frames[queue]) < num_frames) {

[Severity: Medium]
The old code here divided by priv->tx_coal_frames[queue] without checking
it for zero. That code came in with be8b38a722e6 ("net: stmmac: Add support
for XDP_TX action").

Can tx_coal_frames[queue] be 0 here? __stmmac_set_coalesce() only rejects
the request when both values are zero:

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

So a command like "ethtool -C ethX tx-usecs 100 tx-frames 0" is accepted
and stores 0 in priv->tx_coal_frames[]. Before this patch, the next XDP_TX
or ndo_xdp_xmit frame would then do a modulo by zero in softirq context.

The new priv->tx_coal_frames[queue] guard fixes that, but the commit
message only describes multi-buff TX support. stmmac_xmit(),
stmmac_tso_xmit() and stmmac_xdp_xmit_zc() already have this check.

Could the fix be split into its own patch with a
"Fixes: be8b38a722e6" tag, so it can reach stable?

[Severity: Low]
This check does two plain loads of priv->tx_coal_frames[queue]: one for the
zero test and one for the divisor.

__stmmac_set_coalesce() writes this field under RTNL. It does not hold the
TX queue lock that stmmac_xdp_xmit() and stmmac_xdp_xmit_back() take, and
it can store 0.

If the compiler reloads the field between the two reads and a 0 is stored
in that window, can the modulo still divide by zero? Would taking a single
snapshot help, for example:

	u32 coal = READ_ONCE(priv->tx_coal_frames[queue]);

and then using coal for both the test and the division?

stmmac_xmit(), stmmac_tso_xmit() and stmmac_xdp_xmit_zc() read the field
twice in the same way.

> +		struct stmmac_txq_stats *txq_stats;
>  
> -	if (set_ic) {
> +		desc = stmmac_get_tx_desc(priv, tx_q, entry);
> +		stmmac_set_tx_ic(priv, desc);
>  		tx_q->tx_count_frames = 0;

[ ... ]

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

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · 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 patch fixes a divide-by-zero crash in the XDP TX path
>   without saying so and without a Fixes: tag.
> - [Low] In stmmac_xdp_xmit_xdpf(), the new coalescing check reads
>   priv->tx_coal_frames[queue] twice with plain loads: once for the zero…
> 
> --
> 
> net: stmmac: add XDP multi-buff support for TX side
> 
> This commit extends stmmac_xdp_xmit_xdpf() so it can transmit XDP frames
> that carry fragments. The frame head and each frag get their own TX
> descriptor. The driver now also advertises NDO_XMIT_SG for the redirect
> target.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 3ad9252bf6aea..f1e9ac2f98598 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5241,77 +5241,118 @@ 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)
> >  {
> 
> [ ... ]
> 
> > +	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 (priv->tx_coal_frames[queue] &&
> > +	    (tx_q->tx_count_frames % priv->tx_coal_frames[queue]) < num_frames) {
> 
> [Severity: Medium]
> The old code here divided by priv->tx_coal_frames[queue] without checking
> it for zero. That code came in with be8b38a722e6 ("net: stmmac: Add support
> for XDP_TX action").
> 
> Can tx_coal_frames[queue] be 0 here? __stmmac_set_coalesce() only rejects
> the request when both values are zero:
> 
> 	if ((ec->tx_coalesce_usecs == 0) &&
> 	    (ec->tx_max_coalesced_frames == 0))
> 		return -EINVAL;
> 
> So a command like "ethtool -C ethX tx-usecs 100 tx-frames 0" is accepted
> and stores 0 in priv->tx_coal_frames[]. Before this patch, the next XDP_TX
> or ndo_xdp_xmit frame would then do a modulo by zero in softirq context.
> 
> The new priv->tx_coal_frames[queue] guard fixes that, but the commit
> message only describes multi-buff TX support. stmmac_xmit(),
> stmmac_tso_xmit() and stmmac_xdp_xmit_zc() already have this check.
> 
> Could the fix be split into its own patch with a
> "Fixes: be8b38a722e6" tag, so it can reach stable?

I guess we can avoid to add the 'Fixes tag' here (and so to backport this
patch) since the issue seems quite a corner case.

> 
> [Severity: Low]
> This check does two plain loads of priv->tx_coal_frames[queue]: one for the
> zero test and one for the divisor.
> 
> __stmmac_set_coalesce() writes this field under RTNL. It does not hold the
> TX queue lock that stmmac_xdp_xmit() and stmmac_xdp_xmit_back() take, and
> it can store 0.
> 
> If the compiler reloads the field between the two reads and a 0 is stored
> in that window, can the modulo still divide by zero? Would taking a single
> snapshot help, for example:
> 
> 	u32 coal = READ_ONCE(priv->tx_coal_frames[queue]);
> 
> and then using coal for both the test and the division?
> 
> stmmac_xmit(), stmmac_tso_xmit() and stmmac_xdp_xmit_zc() read the field
> twice in the same way.

I agree there is a small race here, but the same issue is present in
stmmac_xmit() and in stmmac_tso_xmit(). I think we can fix them with a single
dedicated patch.

Regards,
Lorenzo

> 
> > +		struct stmmac_txq_stats *txq_stats;
> >  
> > -	if (set_ic) {
> > +		desc = stmmac_get_tx_desc(priv, tx_q, entry);
> > +		stmmac_set_tx_ic(priv, desc);
> >  		tx_q->tx_count_frames = 0;
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-b4-stmmac-xmit-mb-v3-1-ca08f029e81c%40oss.qualcomm.com

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