[PATCH net] net: stmmac: fix rx Scatter-Gather support

[PATCH net] net: stmmac: fix rx Scatter-Gather support

[Lorenzo Bianconi]

When a received frame is larger than dma_buf_sz, the DMA scatters it
across multiple RX descriptors (rx Scatter-Gather). The secondary RX
buffer (sec_page) was only allocated and programmed when split-header
(SPH) was active, so for regular frames buffer2 was neither allocated
nor backed by a valid mapping. As soon as an incoming frame overflowed
buffer1, the DMA wrote the overflow into the unmapped secondary-buffer
address, triggering an SMMU translation fault on IOMMU-based platforms:

arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x00000000, fsynr=0x7f0011, cbfrsynra=0x1c90, cb=11
arm-smmu 15000000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x1c90
arm-smmu 15000000.iommu: FSYNR0 = 007f0011 [S1CBNDX=127 WNR PLVL=1]

Enable scatter-gather for non-SPH frames: always allocate the secondary
RX buffer and always mark buffer2 as valid in stmmac_init_rx_buffers()
and stmmac_rx_refill(), and account for it in the buffer length
computation. stmmac_rx_buf1_len() now returns min(dma_buf_sz, plen - len)
on the last descriptor, while stmmac_rx_buf2_len() returns the remaining
bytes on the last descriptor and dma_buf_sz on the intermediate ones. The
GMAC4 + split-header path keeps using the accumulated payload length
semantics, since there an intermediate descriptor's buffer2 can be only
partially filled.

Fixes: 88ebe2cf7f3f ("net: stmmac: Rework stmmac_rx()")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++---------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1fb5f804ea23..be7cb0cafeb5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1659,17 +1659,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
 		buf->page_offset = stmmac_rx_offset(priv);
 	}
 
-	if (priv->sph_active && !buf->sec_page) {
+	if (!buf->sec_page) {
 		buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
 		if (!buf->sec_page)
 			return -ENOMEM;
 
 		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
-		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
-	} else {
-		buf->sec_page = NULL;
-		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
 	}
+	stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
 
 	buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
 
@@ -5097,7 +5094,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 				break;
 		}
 
-		if (priv->sph_active && !buf->sec_page) {
+		if (!buf->sec_page) {
 			buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
 			if (!buf->sec_page)
 				break;
@@ -5108,10 +5105,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 		buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
 
 		stmmac_set_desc_addr(priv, p, buf->addr);
-		if (priv->sph_active)
-			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
-		else
-			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
+		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
 		stmmac_refill_desc3(priv, rx_q, p);
 
 		rx_q->rx_count_frames++;
@@ -5160,7 +5154,7 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
 	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);
+	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
 }
 
 static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
@@ -5170,10 +5164,6 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
 	int coe = priv->hw->rx_csum;
 	unsigned int plen = 0;
 
-	/* Not split header, buffer is not available */
-	if (!priv->sph_active)
-		return 0;
-
 	/* For GMAC4, when split header is enabled, in some rare cases, the
 	 * hardware does not fill buf2 of the first descriptor with payload.
 	 * Thus we cannot assume buf2 is always fully filled if it is not
@@ -5188,8 +5178,9 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
 	 * Thus 'plen - len' always gives the correct length of buf2.
 	 */
 
-	/* Not GMAC4 and not last descriptor */
-	if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
+	/* Not GMAC4, or non-SPH and not last descriptor */
+	if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
+	    (status & rx_not_ls))
 		return priv->dma_conf.dma_buf_sz;
 
 	/* GMAC4 or last descriptor */

---
base-commit: ceac0de741bfb47ca255eee075257b3bb31f0651
change-id: 20260916-stmmac-rx-sg-fix-a8d2d8a3ba01

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

Re: [PATCH net] net: stmmac: fix rx Scatter-Gather support

[Maxime Chevallier]

Hi Lorenzo

On 9/16/26 17:25, Lorenzo Bianconi wrote:
> When a received frame is larger than dma_buf_sz, the DMA scatters it
> across multiple RX descriptors (rx Scatter-Gather). The secondary RX
> buffer (sec_page) was only allocated and programmed when split-header
> (SPH) was active, so for regular frames buffer2 was neither allocated
> nor backed by a valid mapping. As soon as an incoming frame overflowed
> buffer1, the DMA wrote the overflow into the unmapped secondary-buffer
> address, triggering an SMMU translation fault on IOMMU-based platforms:
> 
> arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x00000000, fsynr=0x7f0011, cbfrsynra=0x1c90, cb=11
> arm-smmu 15000000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x1c90
> arm-smmu 15000000.iommu: FSYNR0 = 007f0011 [S1CBNDX=127 WNR PLVL=1]
> 
> Enable scatter-gather for non-SPH frames: always allocate the secondary
> RX buffer and always mark buffer2 as valid in stmmac_init_rx_buffers()
> and stmmac_rx_refill(), and account for it in the buffer length
> computation. stmmac_rx_buf1_len() now returns min(dma_buf_sz, plen - len)
> on the last descriptor, while stmmac_rx_buf2_len() returns the remaining
> bytes on the last descriptor and dma_buf_sz on the intermediate ones. The
> GMAC4 + split-header path keeps using the accumulated payload length
> semantics, since there an intermediate descriptor's buffer2 can be only
> partially filled.
> 
> Fixes: 88ebe2cf7f3f ("net: stmmac: Rework stmmac_rx()")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Ah this is also nice ! Ran into that with the Jumbo Frame selftest, it
spills into the next desc and it doesn't end well (at least on dwmac1000).

I don't know how you've tested that, can you test it in conjunction with
this patch too ?

https://lore.kernel.org/netdev/20260911212028.1497613-6-maxime.chevallier@bootlin.com/

It changes the way we select the dma_buf_sz based on the MTU.

I'll run your series on the boards I have and report if anything weird happens,
but if you have some testing commands to share that would be awesome :)

Maxime

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++---------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea23..be7cb0cafeb5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1659,17 +1659,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
>  		buf->page_offset = stmmac_rx_offset(priv);
>  	}
>  
> -	if (priv->sph_active && !buf->sec_page) {
> +	if (!buf->sec_page) {
>  		buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
>  		if (!buf->sec_page)
>  			return -ENOMEM;
>  
>  		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
> -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> -	} else {
> -		buf->sec_page = NULL;
> -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
>  	}
> +	stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
>  
>  	buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
>  
> @@ -5097,7 +5094,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
>  				break;
>  		}
>  
> -		if (priv->sph_active && !buf->sec_page) {
> +		if (!buf->sec_page) {
>  			buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
>  			if (!buf->sec_page)
>  				break;
> @@ -5108,10 +5105,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
>  		buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
>  
>  		stmmac_set_desc_addr(priv, p, buf->addr);
> -		if (priv->sph_active)
> -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> -		else
> -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> +		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
>  		stmmac_refill_desc3(priv, rx_q, p);
>  
>  		rx_q->rx_count_frames++;
> @@ -5160,7 +5154,7 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
>  	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);
> +	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
>  }
>  
>  static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> @@ -5170,10 +5164,6 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
>  	int coe = priv->hw->rx_csum;
>  	unsigned int plen = 0;
>  
> -	/* Not split header, buffer is not available */
> -	if (!priv->sph_active)
> -		return 0;
> -
>  	/* For GMAC4, when split header is enabled, in some rare cases, the
>  	 * hardware does not fill buf2 of the first descriptor with payload.
>  	 * Thus we cannot assume buf2 is always fully filled if it is not
> @@ -5188,8 +5178,9 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
>  	 * Thus 'plen - len' always gives the correct length of buf2.
>  	 */
>  
> -	/* Not GMAC4 and not last descriptor */
> -	if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
> +	/* Not GMAC4, or non-SPH and not last descriptor */
> +	if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
> +	    (status & rx_not_ls))
>  		return priv->dma_conf.dma_buf_sz;
>  
>  	/* GMAC4 or last descriptor */
> 
> ---
> base-commit: ceac0de741bfb47ca255eee075257b3bb31f0651
> change-id: 20260916-stmmac-rx-sg-fix-a8d2d8a3ba01
> 
> Best regards,

Re: [PATCH net] net: stmmac: fix rx Scatter-Gather support

[Lorenzo Bianconi]

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

> Hi Lorenzo

Hi Maxime,

> 
> On 9/16/26 17:25, Lorenzo Bianconi wrote:
> > When a received frame is larger than dma_buf_sz, the DMA scatters it
> > across multiple RX descriptors (rx Scatter-Gather). The secondary RX
> > buffer (sec_page) was only allocated and programmed when split-header
> > (SPH) was active, so for regular frames buffer2 was neither allocated
> > nor backed by a valid mapping. As soon as an incoming frame overflowed
> > buffer1, the DMA wrote the overflow into the unmapped secondary-buffer
> > address, triggering an SMMU translation fault on IOMMU-based platforms:
> > 
> > arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x00000000, fsynr=0x7f0011, cbfrsynra=0x1c90, cb=11
> > arm-smmu 15000000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x1c90
> > arm-smmu 15000000.iommu: FSYNR0 = 007f0011 [S1CBNDX=127 WNR PLVL=1]
> > 
> > Enable scatter-gather for non-SPH frames: always allocate the secondary
> > RX buffer and always mark buffer2 as valid in stmmac_init_rx_buffers()
> > and stmmac_rx_refill(), and account for it in the buffer length
> > computation. stmmac_rx_buf1_len() now returns min(dma_buf_sz, plen - len)
> > on the last descriptor, while stmmac_rx_buf2_len() returns the remaining
> > bytes on the last descriptor and dma_buf_sz on the intermediate ones. The
> > GMAC4 + split-header path keeps using the accumulated payload length
> > semantics, since there an intermediate descriptor's buffer2 can be only
> > partially filled.
> > 
> > Fixes: 88ebe2cf7f3f ("net: stmmac: Rework stmmac_rx()")
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> Ah this is also nice ! Ran into that with the Jumbo Frame selftest, it
> spills into the next desc and it doesn't end well (at least on dwmac1000).
> 
> I don't know how you've tested that, can you test it in conjunction with
> this patch too ?
> 
> https://lore.kernel.org/netdev/20260911212028.1497613-6-maxime.chevallier@bootlin.com/
> 
> It changes the way we select the dma_buf_sz based on the MTU.

Ack. My main goal here is to keep the default MTU/rx length (1536B) on
the stmmac device and to split the received packet (e.g. with length 8KB)
over multiple DMA descriptors.

> 
> I'll run your series on the boards I have and report if anything weird happens,
> but if you have some testing commands to share that would be awesome :)

If you want to test with a simple ping you just need to increase the MTU on
the sender (e.g. setting MTU to 7KB). In case you prefer to test with TCP, you
would need to change even the route MTU used by the stmmac device:

- sender:
  $ip link set <nic> mtu 7000
- receiver (stmmac):
  $ip route change <net> dev <nic> mtu 7000

Regards,
Lorenzo

> 
> Maxime
> 
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++---------------
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 1fb5f804ea23..be7cb0cafeb5 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1659,17 +1659,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
> >  		buf->page_offset = stmmac_rx_offset(priv);
> >  	}
> >  
> > -	if (priv->sph_active && !buf->sec_page) {
> > +	if (!buf->sec_page) {
> >  		buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> >  		if (!buf->sec_page)
> >  			return -ENOMEM;
> >  
> >  		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
> > -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> > -	} else {
> > -		buf->sec_page = NULL;
> > -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> >  	}
> > +	stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> >  
> >  	buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
> >  
> > @@ -5097,7 +5094,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> >  				break;
> >  		}
> >  
> > -		if (priv->sph_active && !buf->sec_page) {
> > +		if (!buf->sec_page) {
> >  			buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> >  			if (!buf->sec_page)
> >  				break;
> > @@ -5108,10 +5105,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> >  		buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
> >  
> >  		stmmac_set_desc_addr(priv, p, buf->addr);
> > -		if (priv->sph_active)
> > -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> > -		else
> > -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> > +		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> >  		stmmac_refill_desc3(priv, rx_q, p);
> >  
> >  		rx_q->rx_count_frames++;
> > @@ -5160,7 +5154,7 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
> >  	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);
> > +	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
> >  }
> >  
> >  static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> > @@ -5170,10 +5164,6 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> >  	int coe = priv->hw->rx_csum;
> >  	unsigned int plen = 0;
> >  
> > -	/* Not split header, buffer is not available */
> > -	if (!priv->sph_active)
> > -		return 0;
> > -
> >  	/* For GMAC4, when split header is enabled, in some rare cases, the
> >  	 * hardware does not fill buf2 of the first descriptor with payload.
> >  	 * Thus we cannot assume buf2 is always fully filled if it is not
> > @@ -5188,8 +5178,9 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> >  	 * Thus 'plen - len' always gives the correct length of buf2.
> >  	 */
> >  
> > -	/* Not GMAC4 and not last descriptor */
> > -	if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
> > +	/* Not GMAC4, or non-SPH and not last descriptor */
> > +	if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
> > +	    (status & rx_not_ls))
> >  		return priv->dma_conf.dma_buf_sz;
> >  
> >  	/* GMAC4 or last descriptor */
> > 
> > ---
> > base-commit: ceac0de741bfb47ca255eee075257b3bb31f0651
> > change-id: 20260916-stmmac-rx-sg-fix-a8d2d8a3ba01
> > 
> > Best regards,
> 

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

Re: [PATCH net] net: stmmac: fix rx Scatter-Gather support

[netdev-bot+sashiko]

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

Critical: 1 · High: 1 (1) · Medium: 6 · 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).

- [Critical] stmmac_rx() strips the FCS with an unchecked unsigned
  subtraction (`buf2_len -= ETH_FCS_LEN` at stmmac_main.c:5803,…
- [High] The patch unconditionally allocates a secondary RX page and
  unconditionally reports a non-zero buffer2 length for every MAC core,…
- [Medium] The RX page pool is created with `pp_params.offset =
  stmmac_rx_offset(priv)` and `max_len = dma_conf->dma_buf_sz`, and only…
- [Medium] For GMAC4 without SPH, stmmac_rx_buf2_len() now assumes every
  intermediate descriptor's buffer2 is completely filled (returns…
- [Medium] The patch makes the secondary page-pool page mandatory for
  every RX descriptor on every stmmac device and configuration…
- [Medium] With a native XDP program attached, stmmac_xdp_set_prog()
  explicitly clears priv->sph_active, and this patch now arms buffer2…
- [Medium] The AF_XDP zero-copy refill path is left untouched:
  stmmac_rx_refill_zc() still calls `stmmac_set_desc_sec_addr(priv,…
- [Medium] The new length arithmetic depends on `priv->hw->rx_csum`,
  which stmmac_set_features() (stmmac_main.c:6210-6214) mutates from…
- [Low] The descriptor-position comments in the changed RX length helpers
  no longer describe the code.

Pre-existing issues:
- [High] In stmmac_rx(), when napi_build_skb() fails the code calls
  page_pool_recycle_direct(rx_q->page_pool, buf->page) and jumps to…

--

net: stmmac: fix rx Scatter-Gather support

The patch always allocates and programs the secondary RX buffer (sec_page)
instead of doing so only when split-header is active, so that a frame
overflowing buffer1 does not make the DMA write to an unmapped address.
stmmac_rx_buf1_len() and stmmac_rx_buf2_len() are adjusted to account for
buffer2 in the non-SPH case.

A few questions about the buffer2 accounting and the cores/paths that are
now affected.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..be7cb0cafeb50 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1659,17 +1659,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
>  		buf->page_offset = stmmac_rx_offset(priv);
>  	}
>  
> -	if (priv->sph_active && !buf->sec_page) {
> +	if (!buf->sec_page) {
>  		buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
>  		if (!buf->sec_page)
>  			return -ENOMEM;
>  
>  		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
> -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> -	} else {
> -		buf->sec_page = NULL;
> -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
>  	}
> +	stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);

[Severity: High]
Does the "always mark buffer2 as valid" part hold on the legacy cores?
Only dwmac4_descs.c and dwxgmac2_descs.c provide a set_sec_addr callback;
enh_desc_ops (dwmac1000) and ndesc_ops (dwmac100) do not, and the
dispatch macro silently skips an absent callback:

drivers/net/ethernet/stmicro/stmmac/hwif.h:stmmac_do_void_callback() {
	int __result = -EINVAL; \
	if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) { \
		(__priv)->hw->__module->__cname((__arg0), ##__args); \
	...
}

So on those cores buf->sec_addr is never handed to the hardware, and in
ring mode with 16 KiB buffers the hardware buffer2 is inside the primary
page anyway:

drivers/net/ethernet/stmicro/stmmac/ring_mode.c:refill_desc3() {
	if (priv->dma_conf.dma_buf_sz == BUF_SIZE_16KiB)
		p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
}

With the "!priv->sph_active" early return gone from stmmac_rx_buf2_len(),
buf2_len becomes non-zero on those cores too, and stmmac_rx() then does:

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

Can this publish the contents of a page the DMA never wrote to the network
stack?  On dwmac1000/dwmac100 the intermediate descriptors of a scattered
frame are discarded by enh_desc_get_rx_status()/ndesc_get_rx_status(), so
the last descriptor is processed with len = 0 and its FL field is the total
frame length; a 3000-byte frame with dma_buf_sz = 2048 would give
buf1_len = 2048 and buf2_len = 952 out of the untouched sec_page.  For
larger jumbo frames buf2_len (plen - dma_buf_sz) can also exceed the
order-0 sec_page.  Should buffer2 be gated on the cores that can actually
program an independently mapped buffer2?

[Severity: Medium]
The secondary buffer is handed to the DMA at page offset 0, but the page
pool is only configured with offset 0 when SPH is active:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__alloc_dma_rx_desc_resources() {
	pp_params.offset = stmmac_rx_offset(priv);
	pp_params.max_len = dma_conf->dma_buf_sz;

	if (priv->sph_active) {
		pp_params.offset = 0;
		pp_params.max_len += stmmac_rx_offset(priv);
	}
}

net/core/page_pool.c:__page_pool_dma_sync_for_device() {
	dma_sync_size = min(dma_sync_size, pool->p.max_len);
	__dma_sync_single_for_device(pool->p.dev, dma_addr + pool->p.offset,
				     dma_sync_size, pool->p.dma_dir);
}

With !sph_active the pool syncs from dma_addr + NET_SKB_PAD + NET_IP_ALIGN,
so the first bytes of every secondary page are given to the device without
a sync-for-device, both on first map and on recycle.  On a non-coherent
platform, can a dirty cache line in that prefix be written back over data
the device has already placed there?  The SPH path sets offset = 0 for
exactly this reason.

> @@ -5097,7 +5094,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
>  				break;
>  		}
>  
> -		if (priv->sph_active && !buf->sec_page) {
> +		if (!buf->sec_page) {
>  			buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
>  			if (!buf->sec_page)
>  				break;

[Severity: Medium]
This makes a second page-pool page mandatory for every RX descriptor on
every stmmac device, doubling the RX buffer footprint (order-2/order-3
pages for the 8/16 KiB bfsize cases), while pp_params.pool_size stays at
dma_conf->dma_rx_size, i.e. the recycle ring is now sized for half the
in-flight pages.

For MTU-conformant traffic dma_buf_sz is always above the MTU:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_bfsize() {
	if (mtu >= BUF_SIZE_8KiB)
		ret = BUF_SIZE_16KiB;
	...
	else
		ret = DEFAULT_BUFSIZE;
}

so buffer1 alone holds the frame and the second page is unused.  Could the
commit message state in which configuration an in-spec frame exceeds
dma_buf_sz, and mention the extra page per descriptor plus the pool_size
mismatch?

> @@ -5108,10 +5105,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
>  		buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
>  
>  		stmmac_set_desc_addr(priv, p, buf->addr);
> -		if (priv->sph_active)
> -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> -		else
> -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> +		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
>  		stmmac_refill_desc3(priv, rx_q, p);

[Severity: Medium]
Native XDP always runs in the non-SPH mode that this patch now arms
buffer2 for:

drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
	priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
}

In stmmac_rx() the program only ever sees buffer1:

	xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
			 buf->page_offset, buf1_len, true);

and on a TX/redirect verdict the loop leaves before the buf2_len block:

	} else if (xdp_res & (STMMAC_XDP_TX |
			      STMMAC_XDP_REDIRECT)) {
		xdp_status |= xdp_res;
		buf->page = NULL;
		skb = NULL;
		count++;
		continue;
	}

XDP caps the configured MTU at ETH_DATA_LEN, but GMAC_CORE_INIT in
dwmac4.h contains GMAC_CONFIG_JE and XGMAC_CORE_INIT_RX programs
GPSL = XGMAC_JUMBO_LEN, so an oversized frame is still accepted and split.
Does this silently forward a truncated frame, and should the multi-buffer
case be rejected instead?

[Severity: Medium]
The AF_XDP zero-copy refill path is left as it was:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_refill_zc() {
	stmmac_set_desc_addr(priv, rx_desc, dma_addr);
	stmmac_set_desc_sec_addr(priv, rx_desc, 0, false);
}

but on XGMAC the validity argument is ignored and there is no BUF2V bit in
the RX read descriptor:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:dwxgmac2_set_sec_addr() {
	p->des2 = cpu_to_le32(lower_32_bits(addr));
	p->des3 = cpu_to_le32(upper_32_bits(addr));
}

so buffer2 stays armed at DMA address 0 while RBSZ for a ZC queue is
xsk_pool_get_rx_frame_size() and GPSL is the jumbo limit.  Is the SMMU
translation fault quoted in the commit message still reachable on this
path?  The multi-descriptor rejection in stmmac_rx_zc() happens after the
DMA write.

> @@ -5160,7 +5154,7 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
>  	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);
> +	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
>  }

[Severity: Low]
This isn't a bug, but the descriptor-position comments here no longer
match the code.  The non-first-descriptor early return at the top of
stmmac_rx_buf1_len() is gated on priv->sph_active, so with non-SPH
scatter-gather a continuation descriptor (len != 0) falls through to the
branches commented "First descriptor, not last descriptor and not split
header" and "First descriptor and last descriptor and not split header",
while plen - len only makes sense for a continuation descriptor.

> @@ -5170,10 +5164,6 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
>  	int coe = priv->hw->rx_csum;
>  	unsigned int plen = 0;
>  
> -	/* Not split header, buffer is not available */
> -	if (!priv->sph_active)
> -		return 0;
> -

[Severity: Critical]
With this return gone, the last-descriptor path returns the exact
remainder plen - len, which can be 1, 2 or 3.  stmmac_rx() then strips the
FCS with an unchecked unsigned subtraction:

	/* ACS is disabled; strip manually. */
	if (likely(!(status & rx_not_ls))) {
		if (buf2_len) {
			buf2_len -= ETH_FCS_LEN;
			len -= ETH_FCS_LEN;
		} else if (buf1_len) {
			buf1_len -= ETH_FCS_LEN;
			len -= ETH_FCS_LEN;
		}
	}

and passes the result straight on:

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

Can a frame that overflows a buffer boundary by 1..3 bytes wrap this?
GMAC_CORE_INIT in dwmac4.h sets GMAC_CONFIG_JE unconditionally, so with
MTU 1500 and dma_buf_sz = DEFAULT_BUFSIZE = 1536 a 1537-byte frame is
accepted and split as 1536 + 1.  On that first-and-last descriptor
stmmac_rx_buf1_len() returns min(1536, 1537 - 0) = 1536, then
stmmac_rx_buf2_len() returns 1537 - 1536 = 1, and 1 - 4 gives 0xfffffffd
being handed to dma_sync_single_for_cpu() and skb_add_rx_frag().  The
buf1_len branch is reachable the same way with frames of
2 * n * dma_buf_sz + 1..3 bytes, and stmmac_rx_zc() also subtracts
ETH_FCS_LEN from stmmac_rx_buf1_len() unconditionally.  Before this patch
the non-SPH path returned 0 here, and in SPH mode buffer1 ends at the
split header so the whole FCS always landed in buffer2.

>  	/* For GMAC4, when split header is enabled, in some rare cases, the
>  	 * hardware does not fill buf2 of the first descriptor with payload.
>  	 * Thus we cannot assume buf2 is always fully filled if it is not
> @@ -5188,8 +5178,9 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
>  	 * Thus 'plen - len' always gives the correct length of buf2.
>  	 */
>  
> -	/* Not GMAC4 and not last descriptor */
> -	if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
> +	/* Not GMAC4, or non-SPH and not last descriptor */
> +	if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
> +	    (status & rx_not_ls))
>  		return priv->dma_conf.dma_buf_sz;
>  
>  	/* GMAC4 or last descriptor */

[Severity: Medium]
For GMAC4 without SPH this now assumes every intermediate descriptor's
buffer2 is completely filled, discarding the accumulated-PL value that the
comment just above describes as always giving the correct length of buf2.

The fall-through has no len <= plen guard:

	plen = stmmac_get_rx_frame_len(priv, p, coe);

	return plen - len;

and stmmac_rx_buf1_len() clamps the same wrapped value with
min_t(unsigned int, dma_buf_sz, plen - len) rather than detecting the
inconsistency.  One way the assumption can break is the RBSZ field itself:
DMA_RBSZ_MASK is GENMASK(14, 1) in dwmac4_dma.h and cannot represent
BUF_SIZE_16KiB, so dwmac4_set_dma_bfsize() writes a truncated RBSZ while
the driver keeps assuming dma_buf_sz per buffer.  If an intermediate
buffer2 is short by even one byte, does the accumulated len exceed plen and
turn buf2_len into a multi-gigabyte fragment length?  Would a
plen > len ? plen - len : 0 form be safer here?

The commit message says the GMAC4 plus split-header path keeps the
accumulated payload semantics "since there an intermediate descriptor's
buffer2 can be only partially filled" - is there a databook statement that
this cannot happen for GMAC4 outside SPH mode?

[Severity: Medium]
Both helpers read priv->hw->rx_csum independently for the same descriptor
(int coe = priv->hw->rx_csum;) and feed it to stmmac_get_rx_frame_len(),
whose COE_TYPE1 path subtracts 2 from the descriptor frame length.  The
writer runs from process context with no synchronization against the RX
NAPI poll:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_features() {
	if (features & NETIF_F_RXCSUM)
		priv->hw->rx_csum = priv->plat->rx_coe;
	else
		priv->hw->rx_csum = 0;
	...
	stmmac_rx_ipc(priv, priv->hw);
}

If ethtool -K ethX rx off/on lands between the stmmac_rx_buf1_len() and
stmmac_rx_buf2_len() reads on type-1-COE hardware, plen in the second call
is 2 bytes smaller than the one used to accumulate len, so plen - len
wraps.  Before this patch the non-SPH path returned 0 here and buf1_len did
no subtraction, so the worst case was a 2-byte length inaccuracy.  Could
coe/plen be read once per descriptor?

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the RX
buffer lifecycle the patch reworks: in stmmac_rx() the napi_build_skb()
failure path releases the page without clearing buf->page:

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

Every other consumption site in the function sets buf->page = NULL after
handing the page away.  With the stale pointer, stmmac_rx_refill() skips
the replacement allocation and programs the already-recycled page back
into the descriptor:

	if (!buf->page) {
		buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);

and at teardown stmmac_free_rx_buffer() puts it a second time:

	page_pool_put_full_page(rx_q->page_pool, buf->page, false);
	buf->page = NULL;

Can this alias one page into two descriptors and underflow the page-pool
refcount?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f%40oss.qualcomm.com

Re: [PATCH net] net: stmmac: fix rx Scatter-Gather support

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 9 potential
> issue(s) to consider.
> 
> Critical: 1 · High: 1 (1) · Medium: 6 · 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).
> 
> - [Critical] stmmac_rx() strips the FCS with an unchecked unsigned
>   subtraction (`buf2_len -= ETH_FCS_LEN` at stmmac_main.c:5803,…
> - [High] The patch unconditionally allocates a secondary RX page and
>   unconditionally reports a non-zero buffer2 length for every MAC core,…
> - [Medium] The RX page pool is created with `pp_params.offset =
>   stmmac_rx_offset(priv)` and `max_len = dma_conf->dma_buf_sz`, and only…
> - [Medium] For GMAC4 without SPH, stmmac_rx_buf2_len() now assumes every
>   intermediate descriptor's buffer2 is completely filled (returns…
> - [Medium] The patch makes the secondary page-pool page mandatory for
>   every RX descriptor on every stmmac device and configuration…
> - [Medium] With a native XDP program attached, stmmac_xdp_set_prog()
>   explicitly clears priv->sph_active, and this patch now arms buffer2…
> - [Medium] The AF_XDP zero-copy refill path is left untouched:
>   stmmac_rx_refill_zc() still calls `stmmac_set_desc_sec_addr(priv,…
> - [Medium] The new length arithmetic depends on `priv->hw->rx_csum`,
>   which stmmac_set_features() (stmmac_main.c:6210-6214) mutates from…
> - [Low] The descriptor-position comments in the changed RX length helpers
>   no longer describe the code.

I will address sashiko's comments in v2.

Regards,
Lorenzo

> 
> Pre-existing issues:
> - [High] In stmmac_rx(), when napi_build_skb() fails the code calls
>   page_pool_recycle_direct(rx_q->page_pool, buf->page) and jumps to…
> 
> --
> 
> net: stmmac: fix rx Scatter-Gather support
> 
> The patch always allocates and programs the secondary RX buffer (sec_page)
> instead of doing so only when split-header is active, so that a frame
> overflowing buffer1 does not make the DMA write to an unmapped address.
> stmmac_rx_buf1_len() and stmmac_rx_buf2_len() are adjusted to account for
> buffer2 in the non-SPH case.
> 
> A few questions about the buffer2 accounting and the cores/paths that are
> now affected.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 1fb5f804ea234..be7cb0cafeb50 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1659,17 +1659,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
> >  		buf->page_offset = stmmac_rx_offset(priv);
> >  	}
> >  
> > -	if (priv->sph_active && !buf->sec_page) {
> > +	if (!buf->sec_page) {
> >  		buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> >  		if (!buf->sec_page)
> >  			return -ENOMEM;
> >  
> >  		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
> > -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> > -	} else {
> > -		buf->sec_page = NULL;
> > -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> >  	}
> > +	stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> 
> [Severity: High]
> Does the "always mark buffer2 as valid" part hold on the legacy cores?
> Only dwmac4_descs.c and dwxgmac2_descs.c provide a set_sec_addr callback;
> enh_desc_ops (dwmac1000) and ndesc_ops (dwmac100) do not, and the
> dispatch macro silently skips an absent callback:
> 
> drivers/net/ethernet/stmicro/stmmac/hwif.h:stmmac_do_void_callback() {
> 	int __result = -EINVAL; \
> 	if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) { \
> 		(__priv)->hw->__module->__cname((__arg0), ##__args); \
> 	...
> }
> 
> So on those cores buf->sec_addr is never handed to the hardware, and in
> ring mode with 16 KiB buffers the hardware buffer2 is inside the primary
> page anyway:
> 
> drivers/net/ethernet/stmicro/stmmac/ring_mode.c:refill_desc3() {
> 	if (priv->dma_conf.dma_buf_sz == BUF_SIZE_16KiB)
> 		p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
> }
> 
> With the "!priv->sph_active" early return gone from stmmac_rx_buf2_len(),
> buf2_len becomes non-zero on those cores too, and stmmac_rx() then does:
> 
> 	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);
> 
> Can this publish the contents of a page the DMA never wrote to the network
> stack?  On dwmac1000/dwmac100 the intermediate descriptors of a scattered
> frame are discarded by enh_desc_get_rx_status()/ndesc_get_rx_status(), so
> the last descriptor is processed with len = 0 and its FL field is the total
> frame length; a 3000-byte frame with dma_buf_sz = 2048 would give
> buf1_len = 2048 and buf2_len = 952 out of the untouched sec_page.  For
> larger jumbo frames buf2_len (plen - dma_buf_sz) can also exceed the
> order-0 sec_page.  Should buffer2 be gated on the cores that can actually
> program an independently mapped buffer2?
> 
> [Severity: Medium]
> The secondary buffer is handed to the DMA at page offset 0, but the page
> pool is only configured with offset 0 when SPH is active:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__alloc_dma_rx_desc_resources() {
> 	pp_params.offset = stmmac_rx_offset(priv);
> 	pp_params.max_len = dma_conf->dma_buf_sz;
> 
> 	if (priv->sph_active) {
> 		pp_params.offset = 0;
> 		pp_params.max_len += stmmac_rx_offset(priv);
> 	}
> }
> 
> net/core/page_pool.c:__page_pool_dma_sync_for_device() {
> 	dma_sync_size = min(dma_sync_size, pool->p.max_len);
> 	__dma_sync_single_for_device(pool->p.dev, dma_addr + pool->p.offset,
> 				     dma_sync_size, pool->p.dma_dir);
> }
> 
> With !sph_active the pool syncs from dma_addr + NET_SKB_PAD + NET_IP_ALIGN,
> so the first bytes of every secondary page are given to the device without
> a sync-for-device, both on first map and on recycle.  On a non-coherent
> platform, can a dirty cache line in that prefix be written back over data
> the device has already placed there?  The SPH path sets offset = 0 for
> exactly this reason.
> 
> > @@ -5097,7 +5094,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> >  				break;
> >  		}
> >  
> > -		if (priv->sph_active && !buf->sec_page) {
> > +		if (!buf->sec_page) {
> >  			buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> >  			if (!buf->sec_page)
> >  				break;
> 
> [Severity: Medium]
> This makes a second page-pool page mandatory for every RX descriptor on
> every stmmac device, doubling the RX buffer footprint (order-2/order-3
> pages for the 8/16 KiB bfsize cases), while pp_params.pool_size stays at
> dma_conf->dma_rx_size, i.e. the recycle ring is now sized for half the
> in-flight pages.
> 
> For MTU-conformant traffic dma_buf_sz is always above the MTU:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_bfsize() {
> 	if (mtu >= BUF_SIZE_8KiB)
> 		ret = BUF_SIZE_16KiB;
> 	...
> 	else
> 		ret = DEFAULT_BUFSIZE;
> }
> 
> so buffer1 alone holds the frame and the second page is unused.  Could the
> commit message state in which configuration an in-spec frame exceeds
> dma_buf_sz, and mention the extra page per descriptor plus the pool_size
> mismatch?
> 
> > @@ -5108,10 +5105,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> >  		buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
> >  
> >  		stmmac_set_desc_addr(priv, p, buf->addr);
> > -		if (priv->sph_active)
> > -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> > -		else
> > -			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> > +		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> >  		stmmac_refill_desc3(priv, rx_q, p);
> 
> [Severity: Medium]
> Native XDP always runs in the non-SPH mode that this patch now arms
> buffer2 for:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
> 	priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
> }
> 
> In stmmac_rx() the program only ever sees buffer1:
> 
> 	xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
> 			 buf->page_offset, buf1_len, true);
> 
> and on a TX/redirect verdict the loop leaves before the buf2_len block:
> 
> 	} else if (xdp_res & (STMMAC_XDP_TX |
> 			      STMMAC_XDP_REDIRECT)) {
> 		xdp_status |= xdp_res;
> 		buf->page = NULL;
> 		skb = NULL;
> 		count++;
> 		continue;
> 	}
> 
> XDP caps the configured MTU at ETH_DATA_LEN, but GMAC_CORE_INIT in
> dwmac4.h contains GMAC_CONFIG_JE and XGMAC_CORE_INIT_RX programs
> GPSL = XGMAC_JUMBO_LEN, so an oversized frame is still accepted and split.
> Does this silently forward a truncated frame, and should the multi-buffer
> case be rejected instead?
> 
> [Severity: Medium]
> The AF_XDP zero-copy refill path is left as it was:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_refill_zc() {
> 	stmmac_set_desc_addr(priv, rx_desc, dma_addr);
> 	stmmac_set_desc_sec_addr(priv, rx_desc, 0, false);
> }
> 
> but on XGMAC the validity argument is ignored and there is no BUF2V bit in
> the RX read descriptor:
> 
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:dwxgmac2_set_sec_addr() {
> 	p->des2 = cpu_to_le32(lower_32_bits(addr));
> 	p->des3 = cpu_to_le32(upper_32_bits(addr));
> }
> 
> so buffer2 stays armed at DMA address 0 while RBSZ for a ZC queue is
> xsk_pool_get_rx_frame_size() and GPSL is the jumbo limit.  Is the SMMU
> translation fault quoted in the commit message still reachable on this
> path?  The multi-descriptor rejection in stmmac_rx_zc() happens after the
> DMA write.
> 
> > @@ -5160,7 +5154,7 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
> >  	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);
> > +	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
> >  }
> 
> [Severity: Low]
> This isn't a bug, but the descriptor-position comments here no longer
> match the code.  The non-first-descriptor early return at the top of
> stmmac_rx_buf1_len() is gated on priv->sph_active, so with non-SPH
> scatter-gather a continuation descriptor (len != 0) falls through to the
> branches commented "First descriptor, not last descriptor and not split
> header" and "First descriptor and last descriptor and not split header",
> while plen - len only makes sense for a continuation descriptor.
> 
> > @@ -5170,10 +5164,6 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> >  	int coe = priv->hw->rx_csum;
> >  	unsigned int plen = 0;
> >  
> > -	/* Not split header, buffer is not available */
> > -	if (!priv->sph_active)
> > -		return 0;
> > -
> 
> [Severity: Critical]
> With this return gone, the last-descriptor path returns the exact
> remainder plen - len, which can be 1, 2 or 3.  stmmac_rx() then strips the
> FCS with an unchecked unsigned subtraction:
> 
> 	/* ACS is disabled; strip manually. */
> 	if (likely(!(status & rx_not_ls))) {
> 		if (buf2_len) {
> 			buf2_len -= ETH_FCS_LEN;
> 			len -= ETH_FCS_LEN;
> 		} else if (buf1_len) {
> 			buf1_len -= ETH_FCS_LEN;
> 			len -= ETH_FCS_LEN;
> 		}
> 	}
> 
> and passes the result straight on:
> 
> 	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);
> 
> Can a frame that overflows a buffer boundary by 1..3 bytes wrap this?
> GMAC_CORE_INIT in dwmac4.h sets GMAC_CONFIG_JE unconditionally, so with
> MTU 1500 and dma_buf_sz = DEFAULT_BUFSIZE = 1536 a 1537-byte frame is
> accepted and split as 1536 + 1.  On that first-and-last descriptor
> stmmac_rx_buf1_len() returns min(1536, 1537 - 0) = 1536, then
> stmmac_rx_buf2_len() returns 1537 - 1536 = 1, and 1 - 4 gives 0xfffffffd
> being handed to dma_sync_single_for_cpu() and skb_add_rx_frag().  The
> buf1_len branch is reachable the same way with frames of
> 2 * n * dma_buf_sz + 1..3 bytes, and stmmac_rx_zc() also subtracts
> ETH_FCS_LEN from stmmac_rx_buf1_len() unconditionally.  Before this patch
> the non-SPH path returned 0 here, and in SPH mode buffer1 ends at the
> split header so the whole FCS always landed in buffer2.
> 
> >  	/* For GMAC4, when split header is enabled, in some rare cases, the
> >  	 * hardware does not fill buf2 of the first descriptor with payload.
> >  	 * Thus we cannot assume buf2 is always fully filled if it is not
> > @@ -5188,8 +5178,9 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> >  	 * Thus 'plen - len' always gives the correct length of buf2.
> >  	 */
> >  
> > -	/* Not GMAC4 and not last descriptor */
> > -	if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
> > +	/* Not GMAC4, or non-SPH and not last descriptor */
> > +	if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
> > +	    (status & rx_not_ls))
> >  		return priv->dma_conf.dma_buf_sz;
> >  
> >  	/* GMAC4 or last descriptor */
> 
> [Severity: Medium]
> For GMAC4 without SPH this now assumes every intermediate descriptor's
> buffer2 is completely filled, discarding the accumulated-PL value that the
> comment just above describes as always giving the correct length of buf2.
> 
> The fall-through has no len <= plen guard:
> 
> 	plen = stmmac_get_rx_frame_len(priv, p, coe);
> 
> 	return plen - len;
> 
> and stmmac_rx_buf1_len() clamps the same wrapped value with
> min_t(unsigned int, dma_buf_sz, plen - len) rather than detecting the
> inconsistency.  One way the assumption can break is the RBSZ field itself:
> DMA_RBSZ_MASK is GENMASK(14, 1) in dwmac4_dma.h and cannot represent
> BUF_SIZE_16KiB, so dwmac4_set_dma_bfsize() writes a truncated RBSZ while
> the driver keeps assuming dma_buf_sz per buffer.  If an intermediate
> buffer2 is short by even one byte, does the accumulated len exceed plen and
> turn buf2_len into a multi-gigabyte fragment length?  Would a
> plen > len ? plen - len : 0 form be safer here?
> 
> The commit message says the GMAC4 plus split-header path keeps the
> accumulated payload semantics "since there an intermediate descriptor's
> buffer2 can be only partially filled" - is there a databook statement that
> this cannot happen for GMAC4 outside SPH mode?
> 
> [Severity: Medium]
> Both helpers read priv->hw->rx_csum independently for the same descriptor
> (int coe = priv->hw->rx_csum;) and feed it to stmmac_get_rx_frame_len(),
> whose COE_TYPE1 path subtracts 2 from the descriptor frame length.  The
> writer runs from process context with no synchronization against the RX
> NAPI poll:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_features() {
> 	if (features & NETIF_F_RXCSUM)
> 		priv->hw->rx_csum = priv->plat->rx_coe;
> 	else
> 		priv->hw->rx_csum = 0;
> 	...
> 	stmmac_rx_ipc(priv, priv->hw);
> }
> 
> If ethtool -K ethX rx off/on lands between the stmmac_rx_buf1_len() and
> stmmac_rx_buf2_len() reads on type-1-COE hardware, plen in the second call
> is 2 bytes smaller than the one used to accumulate len, so plen - len
> wraps.  Before this patch the non-SPH path returned 0 here and buf1_len did
> no subtraction, so the worst case was a 2-byte length inaccuracy.  Could
> coe/plen be read once per descriptor?
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while looking at the RX
> buffer lifecycle the patch reworks: in stmmac_rx() the napi_build_skb()
> failure path releases the page without clearing buf->page:
> 
> 	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;
> 	}
> 
> Every other consumption site in the function sets buf->page = NULL after
> handing the page away.  With the stale pointer, stmmac_rx_refill() skips
> the replacement allocation and programs the already-recycled page back
> into the descriptor:
> 
> 	if (!buf->page) {
> 		buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> 
> and at teardown stmmac_free_rx_buffer() puts it a second time:
> 
> 	page_pool_put_full_page(rx_q->page_pool, buf->page, false);
> 	buf->page = NULL;
> 
> Can this alias one page into two descriptors and underflow the page-pool
> refcount?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f%40oss.qualcomm.com

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