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

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

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