Re: [PATCH net-next v6 3/4] net: stmmac: drop multi-buffer frames in XDP mode

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

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

On Oct 13, Nicolai Buchwitz wrote:
> Hi Lorenzo
> 
> On 13.10.2026 10:17, Lorenzo Bianconi wrote:
> > On Oct 12, Nicolai Buchwitz wrote:
> 
> > [...]
> 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > index 426bb68c9bbc..8c08d44fdbd3 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > @@ -5893,18 +5893,24 @@ 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;
> > > > +
> > > > +		if (unlikely(!error && status == discard_frame)) {
> > > >  			error = 1;
> > > >  			if (!priv->hwts_rx_en)
> > > >  				rx_errors++;
> > > >  		}
> > > >
> > > > -		if (unlikely(error && (status & rx_not_ls)))
> > > > -			goto read_again;
> > > > +		/* We do not support SG in XDP mode */
> > > > +		if (unlikely(!error && (status & rx_not_ls) &&
> > > > +			     stmmac_xdp_is_enabled(priv))) {
> > > > +			rx_dropped++;
> > > > +			error = 1;
> > > > +		}
> > > 
> > > AFAIU GMAC4 and XGMAC always use the second buffer of a descriptor. If
> > > so, at MTU 1500 a frame of up to 3072 bytes can fit in one descriptor,
> > > and this check would not catch it?
> > > 
> > > The XDP program would then only see the first buffer. With XDP_TX the
> > > frame is cut and sent too short. With XDP_PASS the second buffer is
> > > added
> > > after the program already decided.
> > > 
> > > Patch 4 fixes this. But if this patch should work on its own, should
> > > it
> > > also drop the frame when buf2_len is not zero? Or am I missing
> > > something?
> > 
> > Hi Nicolai,
> > 
> > IIUC your comment we are missing the case where we have a single
> > descriptor
> > (rx_not_ls is not set in status) but the packet is splitted over buf1
> > and buf2,
> > right? Something like:
> > 
> > 	...
> > 	buf2_len = stmmac_rx_buf2_len(priv, p, status, len);
> > 	len += buf2_len;
> > 
> > 	if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
> > 		rx_dropped++;
> > 		dev_kfree_skb(skb);
> > 		goto next;
> > 	}
> > 	...
> > 
> > Is my understanding correct?
> 
> Yes, exactly.
> 
> I think skb is always NULL there in XDP mode, so the dev_kfree_skb()
> can go. Both pages stay in the slot and stmmac_rx_refill() reuses them,
> so nothing leaks.
> 
> Maybe extend the "We do not support SG in XDP mode" comment to cover
> this case too.

ack, thx for the review. I will fix it in v7.

Regards,
Lorenzo

> 
> > [...]
> 
> Thanks,
> Nicolai

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