[PATCH net-next v2] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[PATCH net-next v2] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[Lorenzo Bianconi]

Build the xdp_buff by accumulating all the descriptors that make up a
frame, so the XDP program runs on the full (possibly fragmented) packet
instead of just the first buffer. When the frame is not consumed by the
program, assemble the skb from the head buffer and the collected
fragments via napi_build_skb()/xdp_update_skb_frags_info().

To do so, store the in-progress xdp_buff in rx_q->state instead of the
partially built skb, so the accumulated head and fragments survive a
NAPI poll boundary (mid-frame dma_own or dirty_rx break). The state is
saved only while a frame is in progress and cleared once it completes,
leaving it untouched when the poll does not process anything (e.g.
netpoll invoked with a zero budget).

In addition:
- build the skb head with napi_build_skb() passing xdp->frame_sz, so
  skb_shinfo() lands on the same shared_info the fragments were
  accumulated into;
- attach fragments with xdp_buff_add_frag(), which initializes all the
  shared_info fields and takes care of the pfmemalloc bit;
- release all buffers belonging to a frame when it is dropped on RX
  errors, instead of leaking the ones already attached to the xdp_buff;
- drop the whole frame when the number of fragments exceeds
  MAX_SKB_FRAGS, instead of delivering a truncated one;
- skip zero-length fragments, which can happen for non-first
  descriptors when split-header (SPH) is enabled.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
Changes in v2:
- Rely on xdp_buff_add_frag() to create xdp fragments
- Fix bugs in rx_q state recording
- Fix the corner case where we receive more than MAX_SKB_FRAGS fragments
- Cosmetics
- Link to v1: https://lore.kernel.org/r/20260918-stmmac-rx-mb-v1-1-0b4517d404af@oss.qualcomm.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 202 ++++++++++++++--------
 2 files changed, 131 insertions(+), 73 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 4fc96b317d79..69bdbbf4b920 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -132,7 +132,7 @@ struct stmmac_rx_queue {
 	dma_addr_t dma_rx_phy;
 	unsigned int state_saved;
 	struct {
-		struct sk_buff *skb;
+		struct xdp_buff xdp;
 		unsigned int len;
 		unsigned int error;
 	} state;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bf9e7e4cb1c3..7d1149511c55 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1695,7 +1695,8 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
 		if (!buf->sec_page)
 			return -ENOMEM;
 
-		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
+		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page) +
+				buf->page_offset;
 		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
 	} else {
 		buf->sec_page = NULL;
@@ -5140,7 +5141,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 			if (!buf->sec_page)
 				break;
 
-			buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
+			buf->sec_addr = page_pool_get_dma_addr(buf->sec_page) +
+					buf->page_offset;
 		}
 
 		buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
@@ -5735,6 +5737,70 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 	return failure ? limit : (int)count;
 }
 
+static void
+stmmac_xdp_put_buff(struct stmmac_rx_queue *rx_q, struct xdp_buff *xdp,
+		    int sync_len)
+{
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
+	int i;
+
+	if (likely(!xdp_buff_has_frags(xdp)))
+		goto out;
+
+	for (i = 0; i < sinfo->nr_frags; i++)
+		page_pool_put_full_page(rx_q->page_pool,
+					skb_frag_page(&sinfo->frags[i]), true);
+out:
+	page_pool_put_page(rx_q->page_pool, virt_to_head_page(xdp->data),
+			   sync_len, true);
+}
+
+static struct sk_buff *stmmac_build_skb(struct xdp_buff *xdp)
+{
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
+	u32 metasize = xdp->data - xdp->data_meta;
+	struct sk_buff *skb;
+	u8 num_frags = 0;
+
+	if (unlikely(xdp_buff_has_frags(xdp)))
+		num_frags = sinfo->nr_frags;
+
+	skb = napi_build_skb(xdp->data_hard_start, xdp->frame_sz);
+	if (!skb)
+		return NULL;
+
+	skb_mark_for_recycle(skb);
+	skb_reserve(skb, xdp->data - xdp->data_hard_start);
+	skb_put(skb, xdp->data_end - xdp->data);
+	if (metasize)
+		skb_metadata_set(skb, metasize);
+
+	if (unlikely(xdp_buff_has_frags(xdp)))
+		xdp_update_skb_frags_info(skb, num_frags, sinfo->xdp_frags_size,
+					  num_frags * xdp->frame_sz,
+					  xdp_buff_get_skb_flags(xdp));
+	return skb;
+}
+
+static bool stmmac_build_xdp_frags(struct stmmac_priv *priv,
+				   struct stmmac_rx_queue *rx_q,
+				   unsigned int len, struct page *page,
+				   unsigned int offset,
+				   enum dma_data_direction dma_dir,
+				   struct xdp_buff *xdp)
+{
+	dma_addr_t dma_addr = page_pool_get_dma_addr(page) + offset;
+
+	dma_sync_single_for_cpu(priv->device, dma_addr, len, dma_dir);
+	if (!xdp_buff_add_frag(xdp, page_to_netmem(page), offset, len,
+			       xdp->frame_sz)) {
+		page_pool_put_full_page(rx_q->page_pool, page, true);
+		return false;
+	}
+
+	return true;
+}
+
 /**
  * stmmac_rx - manage the receive process
  * @priv: driver private structure
@@ -5756,11 +5822,12 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 	unsigned int desc_size;
 	struct sk_buff *skb = NULL;
 	struct stmmac_xdp_buff ctx;
+	bool first_desc = true;
 	int xdp_status = 0;
 	int bufsz;
 
 	dma_dir = page_pool_get_dma_dir(rx_q->page_pool);
-	bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
+	bufsz = rx_q->napi_skb_frag_size;
 
 	if (netif_msg_rx_status(priv)) {
 		void *rx_head = stmmac_get_rx_desc(priv, rx_q, 0);
@@ -5780,9 +5847,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		u32 hash;
 
 		if (!count && rx_q->state_saved) {
-			skb = rx_q->state.skb;
+			ctx.xdp = rx_q->state.xdp;
 			error = rx_q->state.error;
 			len = rx_q->state.len;
+			first_desc = false;
 		} else {
 			rx_q->state_saved = false;
 			skb = NULL;
@@ -5820,21 +5888,21 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
+
 		if (unlikely(status == discard_frame)) {
-			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
-			buf->page = NULL;
 			error = 1;
 			if (!priv->hwts_rx_en)
 				rx_errors++;
 		}
 
-		if (unlikely(error && (status & rx_not_ls)))
-			goto read_again;
 		if (unlikely(error)) {
-			dev_kfree_skb(skb);
-			skb = NULL;
-			count++;
-			continue;
+			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
+			buf->page = NULL;
+
+			if (status & rx_not_ls)
+				goto read_again;
+
+			goto error_free_frag;
 		}
 
 		/* Buffer is good. Go on. */
@@ -5855,9 +5923,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			}
 		}
 
-		if (!skb) {
-			unsigned int pre_len, sync_len;
-
+		if (first_desc) {
 			dma_sync_single_for_cpu(priv->device, buf->addr,
 						buf1_len, dma_dir);
 			net_prefetch(page_address(buf->page) +
@@ -5866,6 +5932,33 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
 			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
 					 buf->page_offset, buf1_len, true);
+			first_desc = false;
+			buf->page = NULL;
+		} else if (buf1_len) {
+			error |= !stmmac_build_xdp_frags(priv, rx_q, buf1_len,
+							 buf->page,
+							 buf->page_offset,
+							 dma_dir, &ctx.xdp);
+			buf->page = NULL;
+		}
+
+		if (buf2_len) {
+			error |= !stmmac_build_xdp_frags(priv, rx_q, buf2_len,
+							 buf->sec_page,
+							 buf->page_offset,
+							 dma_dir, &ctx.xdp);
+			buf->sec_page = NULL;
+		}
+
+		if (likely(status & rx_not_ls))
+			goto read_again;
+
+		if (unlikely(error))
+			goto error_free_frag;
+
+		first_desc = true;
+		if (!skb) {
+			unsigned int pre_len, sync_len;
 
 			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
 				  buf->page_offset;
@@ -5887,26 +5980,18 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 				unsigned int xdp_res = -PTR_ERR(skb);
 
 				if (xdp_res & STMMAC_XDP_CONSUMED) {
-					page_pool_put_page(rx_q->page_pool,
-							   virt_to_head_page(ctx.xdp.data),
-							   sync_len, true);
-					buf->page = NULL;
+					stmmac_xdp_put_buff(rx_q, &ctx.xdp, sync_len);
 					rx_dropped++;
 
 					/* Clear skb as it was set as
 					 * status by XDP program.
 					 */
 					skb = NULL;
-
-					if (unlikely((status & rx_not_ls)))
-						goto read_again;
-
 					count++;
 					continue;
 				} else if (xdp_res & (STMMAC_XDP_TX |
 						      STMMAC_XDP_REDIRECT)) {
 					xdp_status |= xdp_res;
-					buf->page = NULL;
 					skb = NULL;
 					count++;
 					continue;
@@ -5914,51 +5999,13 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			}
 		}
 
+		skb = stmmac_build_skb(&ctx.xdp);
 		if (!skb) {
-			unsigned int head_pad_len;
-
-			/* XDP program may expand or reduce tail */
-			buf1_len = ctx.xdp.data_end - ctx.xdp.data;
-
-			skb = napi_build_skb(page_address(buf->page),
-					     rx_q->napi_skb_frag_size);
-			if (!skb) {
-				page_pool_recycle_direct(rx_q->page_pool,
-							 buf->page);
-				rx_dropped++;
-				count++;
-				goto drain_data;
-			}
-
-			/* XDP program may adjust header */
-			head_pad_len = ctx.xdp.data - ctx.xdp.data_hard_start;
-			skb_reserve(skb, head_pad_len);
-			skb_put(skb, buf1_len);
-			skb_mark_for_recycle(skb);
-			buf->page = NULL;
-		} else if (buf1_len) {
-			dma_sync_single_for_cpu(priv->device, buf->addr,
-						buf1_len, dma_dir);
-			skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
-					buf->page, buf->page_offset, buf1_len,
-					priv->dma_conf.dma_buf_sz);
-			buf->page = NULL;
-		}
-
-		if (buf2_len) {
-			dma_sync_single_for_cpu(priv->device, buf->sec_addr,
-						buf2_len, dma_dir);
-			skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
-					buf->sec_page, 0, buf2_len,
-					priv->dma_conf.dma_buf_sz);
-			buf->sec_page = NULL;
-		}
-
-drain_data:
-		if (likely(status & rx_not_ls))
-			goto read_again;
-		if (!skb)
+			stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1);
+			rx_dropped++;
+			count++;
 			continue;
+		}
 
 		/* Got entire packet into SKB. Finish it. */
 
@@ -5989,13 +6036,24 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		rx_packets++;
 		rx_bytes += len;
 		count++;
+		continue;
+error_free_frag:
+		if (!first_desc) {
+			stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1);
+			first_desc = true;
+		}
+		dev_kfree_skb(skb);
+		skb = NULL;
+		count++;
 	}
 
-	if (status & rx_not_ls || skb) {
-		rx_q->state_saved = true;
-		rx_q->state.skb = skb;
-		rx_q->state.error = error;
-		rx_q->state.len = len;
+	if (count || !first_desc) {
+		rx_q->state_saved = !first_desc;
+		if (!first_desc) {
+			rx_q->state.xdp = ctx.xdp;
+			rx_q->state.error = error;
+			rx_q->state.len = len;
+		}
 	}
 
 	stmmac_finalize_xdp_rx(priv, xdp_status);

---
base-commit: 8830e65ed46de41f849eefb8ba227d4852c460f6
change-id: 20260918-stmmac-rx-mb-16469a2714ea

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

Re: [PATCH net-next v2] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: missing drop accounting when XDP fragment addition fails
--

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

net: stmmac: rework stmmac_rx to support XDP rx multi-buff

This commit builds the xdp_buff by accumulating descriptors so the XDP
program runs on the full packet. It saves the state in rx_q->state across
NAPI poll boundaries and builds the skb from the head buffer and fragments.

Link: https://lore.kernel.org/r/20260918-stmmac-rx-mb-v1-1-0b4517d404af@oss.qualcomm.com

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c31..7d1149511c55d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5866,6 +5932,33 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
>  			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
>  					 buf->page_offset, buf1_len, true);
> +			first_desc = false;
> +			buf->page = NULL;
> +		} else if (buf1_len) {
> +			error |= !stmmac_build_xdp_frags(priv, rx_q, buf1_len,
> +							 buf->page,
> +							 buf->page_offset,
> +							 dma_dir, &ctx.xdp);

[Severity: Medium]
When stmmac_build_xdp_frags() returns false, the error flag is set.

This could happen when receiving large frames (e.g. Jumbo frames) on hardware
configured with small DMA buffers, leading to many descriptors per frame.

Will this cause the packet to be silently dropped without updating
rx_dropped or rx_errors?

> +			buf->page = NULL;
> +		}
> +
> +		if (buf2_len) {
> +			error |= !stmmac_build_xdp_frags(priv, rx_q, buf2_len,
> +							 buf->sec_page,
> +							 buf->page_offset,
> +							 dma_dir, &ctx.xdp);
> +			buf->sec_page = NULL;
> +		}
> +
> +		if (likely(status & rx_not_ls))
> +			goto read_again;
> +
> +		if (unlikely(error))
> +			goto error_free_frag;

[ ... ]

> @@ -5989,13 +6036,24 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		rx_packets++;
>  		rx_bytes += len;
>  		count++;
> +		continue;
> +error_free_frag:
> +		if (!first_desc) {
> +			stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1);
> +			first_desc = true;
> +		}
> +		dev_kfree_skb(skb);
> +		skb = NULL;
> +		count++;

[Severity: Medium]
If we reach this teardown path due to an error, the frame is consumed and
the polling count is incremented, but no interface drop statistics appear to
be updated.

Could this make debugging MTU or fragmentation issues difficult, since drops
(such as exceeding MAX_SKB_FRAGS) would be invisible to monitoring? Should
rx_dropped or rx_errors be incremented here?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-stmmac-rx-mb-v2-1-6e826e1ff306@oss.qualcomm.com?part=1

Re: [PATCH net-next v2] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] net: stmmac: missing drop accounting when XDP fragment addition fails
> --
> 
> commit ec049e650a3e45a0def65bd264e60d42d99ff7f9
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> net: stmmac: rework stmmac_rx to support XDP rx multi-buff
> 
> This commit builds the xdp_buff by accumulating descriptors so the XDP
> program runs on the full packet. It saves the state in rx_q->state across
> NAPI poll boundaries and builds the skb from the head buffer and fragments.
> 
> Link: https://lore.kernel.org/r/20260918-stmmac-rx-mb-v1-1-0b4517d404af@oss.qualcomm.com
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index bf9e7e4cb1c31..7d1149511c55d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -5866,6 +5932,33 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  			xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
> >  			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
> >  					 buf->page_offset, buf1_len, true);
> > +			first_desc = false;
> > +			buf->page = NULL;
> > +		} else if (buf1_len) {
> > +			error |= !stmmac_build_xdp_frags(priv, rx_q, buf1_len,
> > +							 buf->page,
> > +							 buf->page_offset,
> > +							 dma_dir, &ctx.xdp);
> 
> [Severity: Medium]
> When stmmac_build_xdp_frags() returns false, the error flag is set.
> 
> This could happen when receiving large frames (e.g. Jumbo frames) on hardware
> configured with small DMA buffers, leading to many descriptors per frame.
> 
> Will this cause the packet to be silently dropped without updating
> rx_dropped or rx_errors?

ack, I will fix it in v3.

> 
> > +			buf->page = NULL;
> > +		}
> > +
> > +		if (buf2_len) {
> > +			error |= !stmmac_build_xdp_frags(priv, rx_q, buf2_len,
> > +							 buf->sec_page,
> > +							 buf->page_offset,
> > +							 dma_dir, &ctx.xdp);
> > +			buf->sec_page = NULL;
> > +		}
> > +
> > +		if (likely(status & rx_not_ls))
> > +			goto read_again;
> > +
> > +		if (unlikely(error))
> > +			goto error_free_frag;
> 
> [ ... ]
> 
> > @@ -5989,13 +6036,24 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  		rx_packets++;
> >  		rx_bytes += len;
> >  		count++;
> > +		continue;
> > +error_free_frag:
> > +		if (!first_desc) {
> > +			stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1);
> > +			first_desc = true;
> > +		}
> > +		dev_kfree_skb(skb);
> > +		skb = NULL;
> > +		count++;
> 
> [Severity: Medium]
> If we reach this teardown path due to an error, the frame is consumed and
> the polling count is incremented, but no interface drop statistics appear to
> be updated.
> 
> Could this make debugging MTU or fragmentation issues difficult, since drops
> (such as exceeding MAX_SKB_FRAGS) would be invisible to monitoring? Should
> rx_dropped or rx_errors be incremented here?

ack, I will fix it in v3.

Regards,
Lorenzo

> 
> >  	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260921-stmmac-rx-mb-v2-1-6e826e1ff306@oss.qualcomm.com?part=1

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

Re: [PATCH net-next v2] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[netdev-bot+sashiko]

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

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

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

- [High] stmmac_rx() now assembles multi-buffer (non-linear) xdp_buffs
  and runs the XDP program on them, but (a) ndev->xdp_features in…
- [High] rx_q->state_saved is now overloaded: in stmmac_rx() it means
  'rx_q->state.xdp holds a valid partially assembled xdp_buff whose…
- [High] An in-progress xdp_buff parked in rx_q->state.xdp is the sole
  owner of its head and fragment pages (stmmac_rx() sets buf->page =…
- [Medium] The new error_free_frag label in stmmac_rx() releases the
  accumulated xdp_buff and increments count but never increments…
- [Low] ASSESSED FALSE POSITIVE: the claim that multi-buffer XDP is
  unreachable because stmmac_xdp_set_prog() rejects MTU > ETH_DATA_LEN…
- [Low] The patch silently changes where the hardware DMAs the split-
  header (SPH) secondary buffer: buf->sec_addr becomes…

Pre-existing issues:
- [High] stmmac_rx_buf1_len() returns min(dma_buf_sz, plen) for the last
  descriptor without subtracting the already-accumulated length, so for…
- [High] The manual FCS strip in stmmac_rx() only checks that the last
  descriptor's buffer length is non-zero before subtracting ETH_FCS_LEN,…
- [Medium] The RX error test uses exact equality ('if (unlikely(status ==
  discard_frame))'), but dwmac4_wrback_get_rx_status() sets 'ret =…
- [Medium] stmmac_rx() consumes descriptor write-back fields
  (packet/header length, error and checksum bits) immediately after…

--

net: stmmac: rework stmmac_rx to support XDP rx multi-buff

This reworks stmmac_rx() so that all descriptors of a frame are accumulated
into a single xdp_buff before the XDP program runs, with the in-progress
xdp_buff parked in rx_q->state across NAPI poll boundaries.  The skb is then
assembled with napi_build_skb() plus xdp_update_skb_frags_info().

Link: https://lore.kernel.org/r/20260918-stmmac-rx-mb-v1-1-0b4517d404af@oss.qualcomm.com

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 4fc96b317d79..69bdbbf4b920 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -132,7 +132,7 @@ struct stmmac_rx_queue {
>  	dma_addr_t dma_rx_phy;
>  	unsigned int state_saved;
>  	struct {
> -		struct sk_buff *skb;
> +		struct xdp_buff xdp;
>  		unsigned int len;
>  		unsigned int error;
>  	} state;

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c3..7d1149511c55 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1695,7 +1695,8 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
>  		if (!buf->sec_page)
>  			return -ENOMEM;
>  
> -		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
> +		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page) +
> +				buf->page_offset;
>  		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
>  	} else {
>  		buf->sec_page = NULL;
> @@ -5140,7 +5141,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
>  			if (!buf->sec_page)
>  				break;
>  
> -			buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
> +			buf->sec_addr = page_pool_get_dma_addr(buf->sec_page) +
> +					buf->page_offset;
>  		}

[Severity: Low]
These two hunks change where the hardware DMAs the split-header secondary
buffer, from offset 0 to buf->page_offset, in both stmmac_init_rx_buffers()
and stmmac_rx_refill().  The matching change is in stmmac_rx(), which now
passes buf->page_offset instead of 0 when adding the buf2 fragment.

The commit message enumerates five specific "In addition" items but does not
mention this one.  Could it be listed too?  It affects hardware addressing
for every split-header RX frame, XDP attached or not.

> @@ -5735,6 +5737,70 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  	return failure ? limit : (int)count;
>  }
>  
> +static void
> +stmmac_xdp_put_buff(struct stmmac_rx_queue *rx_q, struct xdp_buff *xdp,
> +		    int sync_len)
> +{
> +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> +	int i;
> +
> +	if (likely(!xdp_buff_has_frags(xdp)))
> +		goto out;
> +
> +	for (i = 0; i < sinfo->nr_frags; i++)
> +		page_pool_put_full_page(rx_q->page_pool,
> +					skb_frag_page(&sinfo->frags[i]), true);
> +out:
> +	page_pool_put_page(rx_q->page_pool, virt_to_head_page(xdp->data),
> +			   sync_len, true);
> +}

[Severity: High]
Does anything release the pages owned by rx_q->state.xdp when the RX
resources are torn down?

Once a descriptor is consumed stmmac_rx() does buf->page = NULL and
buf->sec_page = NULL, so the parked xdp_buff is the only owner of those
pages.  The teardown path only walks buf_pool:

stmmac_disable_rx_queue()
  stmmac_stop_rx_dma()
  __free_dma_rx_desc_resources()
     dma_free_rx_skbufs()        /* buf_pool entries only */
     page_pool_destroy()

so page_pool_destroy() runs with the saved frame's head and fragment pages
still inflight, and the pool shutdown never completes.

Also, stmmac_enable_rx_queue() installs a fresh page_pool but leaves
state_saved set, so a later poll can restore the old xdp_buff and hand
its pages to this function:

	page_pool_put_full_page(rx_q->page_pool, ...)

which would return pages allocated from the destroyed pool into the new
one.  Should state.xdp be drained and state_saved cleared everywhere RX
resources are freed or re-initialized (ethtool ring resize, MTU change,
stmmac_release(), stmmac_xdp_disable_pool())?

[ ... ]

> +static bool stmmac_build_xdp_frags(struct stmmac_priv *priv,
> +				   struct stmmac_rx_queue *rx_q,
> +				   unsigned int len, struct page *page,
> +				   unsigned int offset,
> +				   enum dma_data_direction dma_dir,
> +				   struct xdp_buff *xdp)
> +{
> +	dma_addr_t dma_addr = page_pool_get_dma_addr(page) + offset;
> +
> +	dma_sync_single_for_cpu(priv->device, dma_addr, len, dma_dir);
> +	if (!xdp_buff_add_frag(xdp, page_to_netmem(page), offset, len,
> +			       xdp->frame_sz)) {
> +		page_pool_put_full_page(rx_q->page_pool, page, true);
> +		return false;
> +	}
> +
> +	return true;
> +}

[ ... ]

> @@ -5780,9 +5847,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		u32 hash;
>  
>  		if (!count && rx_q->state_saved) {
> -			skb = rx_q->state.skb;
> +			ctx.xdp = rx_q->state.xdp;
>  			error = rx_q->state.error;
>  			len = rx_q->state.len;
> +			first_desc = false;
>  		} else {
>  			rx_q->state_saved = false;
>  			skb = NULL;

[Severity: High]
Can rx_q->state.xdp be restored here without ever having been written?

state_saved is shared with stmmac_rx_zc(), which still sets it while only
writing error and len:

	if (status & rx_not_ls) {
		rx_q->state_saved = true;
		rx_q->state.error = error;
		rx_q->state.len = len;
	}

Nothing outside the two RX loops clears state_saved, so it survives an
AF_XDP pool detach:

stmmac_xdp_disable_pool()
  stmmac_disable_rx_queue()
  stmmac_enable_rx_queue()
     stmmac_reset_rx_queue()    /* cur_rx/dirty_rx only */

The next non-ZC poll then takes this branch with a zeroed (or stale)
xdp_buff and first_desc = false, and the first descriptor goes through
stmmac_build_xdp_frags() -> xdp_buff_add_frag(), where
xdp_get_shared_info_from_buff() computes:

	xdp->data_hard_start + xdp->frame_sz - SKB_DATA_ALIGN(sizeof(*sinfo))

With data_hard_start == NULL and frame_sz == 0 that write lands at a small
negative address.  If instead the restored frame terminates on the first
descriptor, stmmac_xdp_run_prog() and napi_build_skb() are called with a
NULL buffer.

The same flag also survives stmmac_resume() -> stmmac_reset_queues_param(),
which re-arms every descriptor while leaving state_saved set, so the first
post-resume poll would append unrelated descriptors to the pre-suspend
buff with the stale len and error.

Would it make sense for stmmac_rx_zc() to stop sharing this flag, and for
the teardown/resume paths to clear it?

[ ... ]

> @@ -5820,21 +5888,21 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> +
>  		if (unlikely(status == discard_frame)) {
> -			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> -			buf->page = NULL;
>  			error = 1;
>  			if (!priv->hwts_rx_en)
>  				rx_errors++;
>  		}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
should this be a bitwise test rather than an equality test?

dwmac4_wrback_get_rx_status() can return a composite value:

	ret = discard_frame;
	...
	if (rdes1 & RDES1_IP_HDR_ERROR) {
		x->ip_hdr_err++;
		ret |= csum_none;
	}

A descriptor reporting both gives discard_frame | csum_none, the equality
fails, error stays 0, and the errored frame is assembled and passed up with
CHECKSUM_NONE while rx_errors is not counted.  Would
"status & discard_frame" cover the combined case?

[Severity: Medium]
This isn't introduced by this patch either, but is a barrier missing
between the OWN-bit test earlier in this loop and the consumption of the
write-back fields?

	status = stmmac_rx_status(priv, &priv->xstats, p);
	/* check if managed by the DMA otherwise go ahead */
	if (unlikely(status & dma_own))
		break;

dwmac4_wrback_get_rx_status() even loads des1/des2 before testing
RDES3_OWN, and there is no dma_rmb() before stmmac_rx_buf1_len() /
stmmac_rx_buf2_len() re-read the descriptor.  The TX clean path in this
file has the corresponding barrier with the comment "Make sure descriptor
fields are read after reading the own bit".  This patch makes those
lengths drive xdp_buff_add_frag() sizes, so a stale length would size a
fragment over bytes the DMA has not delivered yet.

[ ... ]

> @@ -5855,9 +5923,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			}
>  		}
>  
> -		if (!skb) {
> -			unsigned int pre_len, sync_len;
> -
> +		if (first_desc) {
>  			dma_sync_single_for_cpu(priv->device, buf->addr,
>  						buf1_len, dma_dir);
>  			net_prefetch(page_address(buf->page) +

[Severity: High]
This problem wasn't introduced by this patch, but the fragment lengths it
now feeds to xdp_buff_add_frag() come from stmmac_rx_buf1_len(), which does
not subtract the accumulated length on the last descriptor:

	plen = stmmac_get_rx_frame_len(priv, p, coe);

	/* First descriptor and last descriptor and not split header */
	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen);

Compare stmmac_rx_buf2_len(), which documents the requirement:

	/* Thus 'plen - len' always gives the correct length of buf2. */

At MTU 1500 dma_buf_sz is 1536 while the MAC accepts much larger frames
(dwmac4 sets GMAC_CONFIG_JE unconditionally, XGMAC programs GPSL to 16368),
so a 3000-byte frame arrives as 1536 bytes with rx_not_ls plus 1464 bytes
on the last descriptor, yet the helper returns min(1536, 3000) = 1536.

Does that make stmmac_build_xdp_frags() attach a 1532-byte fragment (after
the FCS strip) and stmmac_build_skb() deliver a 3068-byte skb, whose
trailing bytes are the previous contents of the recycled page_pool page?
len and rx_bytes would be off by the same amount.

[Severity: High]
Also pre-existing rather than new here, but the manual FCS strip just above
only checks for a non-zero length before subtracting:

		if (buf2_len) {
			buf2_len -= ETH_FCS_LEN;
			len -= ETH_FCS_LEN;
		} else if (buf1_len) {
			buf1_len -= ETH_FCS_LEN;

With split-header active the last descriptor's payload length is computed
as "plen - len", which a remote peer can make 1, 2 or 3 by choosing the
frame length.  Does the unsigned subtraction then wrap to roughly 4 GiB,
and does that value reach:

	dma_sync_single_for_cpu(priv->device, dma_addr, len, dma_dir);
	if (!xdp_buff_add_frag(xdp, page_to_netmem(page), offset, len,
			       xdp->frame_sz)) {

xdp_buff_add_frag() validates the fragment count but not that the size
fits the allocation, so the huge value would also be recorded in
xdp_frags_size.

> @@ -5866,6 +5932,33 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
>  			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
>  					 buf->page_offset, buf1_len, true);
> +			first_desc = false;
> +			buf->page = NULL;
> +		} else if (buf1_len) {
> +			error |= !stmmac_build_xdp_frags(priv, rx_q, buf1_len,
> +							 buf->page,
> +							 buf->page_offset,
> +							 dma_dir, &ctx.xdp);
> +			buf->page = NULL;
> +		}
> +
> +		if (buf2_len) {
> +			error |= !stmmac_build_xdp_frags(priv, rx_q, buf2_len,
> +							 buf->sec_page,
> +							 buf->page_offset,
> +							 dma_dir, &ctx.xdp);
> +			buf->sec_page = NULL;
> +		}

[Severity: High]
Two things look out of sync with the multi-buffer contract now that
fragments are attached before stmmac_xdp_run_prog() runs.

stmmac_dvr_probe() still advertises:

	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
			     NETDEV_XDP_ACT_XSK_ZEROCOPY;

so NETDEV_XDP_ACT_RX_SG is missing while the driver does deliver
scatter-gather xdp_buffs.  Should it be added?

stmmac_xdp_set_prog() also only gates on the MTU:

	if (prog && dev->mtu > ETH_DATA_LEN) {
		...
		return -EOPNOTSUPP;
	}

There is no prog->aux->xdp_has_frags check, so a program built without
multi-buffer support is now invoked on an xdp_buff with
XDP_FLAGS_HAS_FRAGS set whose data_end only covers the first descriptor.
Since dwmac4 accepts frames well above dma_buf_sz even at MTU 1500, is
that reachable with the currently allowed MTU?

[ ... ]

> @@ -5989,13 +6036,24 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		rx_packets++;
>  		rx_bytes += len;
>  		count++;
> +		continue;
> +error_free_frag:
> +		if (!first_desc) {
> +			stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1);
> +			first_desc = true;
> +		}
> +		dev_kfree_skb(skb);
> +		skb = NULL;
> +		count++;
>  	}

[Severity: Medium]
Should this label also bump rx_dropped or rx_errors?

For the newly added drop reason -- stmmac_build_xdp_frags() returning false
because the frame needs more than MAX_SKB_FRAGS fragments -- the path is:

	error |= !stmmac_build_xdp_frags(...);
	...
	if (unlikely(error))
		goto error_free_frag;

and the label only does stmmac_xdp_put_buff() + dev_kfree_skb() + count++.
The frame then disappears with only rx_pkt_n moving, whereas the other
software drops in this function (the XDP_CONSUMED verdict and the
stmmac_build_skb() failure) both increment rx_dropped.

>  
> -	if (status & rx_not_ls || skb) {
> -		rx_q->state_saved = true;
> -		rx_q->state.skb = skb;
> -		rx_q->state.error = error;
> -		rx_q->state.len = len;
> +	if (count || !first_desc) {
> +		rx_q->state_saved = !first_desc;
> +		if (!first_desc) {
> +			rx_q->state.xdp = ctx.xdp;
> +			rx_q->state.error = error;
> +			rx_q->state.len = len;
> +		}
>  	}
>  
>  	stmmac_finalize_xdp_rx(priv, xdp_status);

Thanks for looking at these.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-stmmac-rx-mb-v2-1-6e826e1ff306%40oss.qualcomm.com