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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

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