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

[Nicolai Buchwitz <nb@tipi-net.de>]

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.

> [...]

Thanks,
Nicolai