[PATCH net-next v8 2/5] net: stmmac: drop split frames on AF_XDP zero-copy RX

[Nicolai Buchwitz]

On 19.10.2026 14:17, Lorenzo Bianconi wrote:
>> Hi Lorenzo
>> 
>> On 18.10.2026 19:07, Lorenzo Bianconi wrote:
>> > stmmac_rx_zc() assumes a 1:1 mapping between a received frame and an
>> > XSK pool buffer and drops any frame that spans more than one RX
>> > descriptor:
>> >
>> >         /* XSK pool expects RX frame 1:1 mapped to XSK buffer */
>> >         if (likely(status & rx_not_ls)) {
>> >                 xsk_buff_free(buf->xdp);
>> >                 buf->xdp = NULL;
>> >                 dirty++;
>> >                 count++;
>> >                 goto read_again;
>> >         }
>> >
>> > The non-last descriptors are freed and dropped, but the frame is not
>> > marked as errored. When the poll reaches the last descriptor of the
>> > same frame, error is still 0, so the fragment is handled as a complete
>> > frame: it is passed to the XDP program, copied into an skb on XDP_PASS,
>> > or converted for XDP_TX. The tail starts in the middle of the original
>> > payload at a predictable offset and, parsed as a fresh frame (len == 0),
>> > stmmac_rx_buf1_len() reports the full dma_buf_sz length for it even
>> > though the DMA engine only wrote the tail remainder into that buffer, so
>> > the reads extend past the received bytes and can reach beyond the end of
>> > the XSK pool buffer. GMAC4 always enables jumbo frames (GMAC_CONFIG_JE
>> > in GMAC_CORE_INIT), so an on-link peer can trigger such a split even
>> > when the interface MTU is left at 1500.
>> >
>> > Set error when the 1:1 mapping check drops a descriptor, so the
>> > remaining descriptors of the frame - including the last one - are
>> > drained and released with xsk_buff_free() instead of being processed as
>> > a new frame. Account every freed buffer with dirty and count the
>> > dropped frame once in the poll budget / rx_pkt_n. The direct 1:1
>> > mapping drop also increments rx_dropped.
>> >
>> > Normalize the legacy normal and enhanced descriptor callbacks to report
>> > rx_not_ls together with discard_frame for non-last descriptors. This
>> > allows both stmmac_rx_zc() and stmmac_rx() to drain split frames on
>> > dwmac100 and dwmac1000 instead of reparsing the final fragment as a new
>> > packet.
>> >
>> > Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
>> > Reviewed-by: Nicolai Buchwitz <nb at tipi-net.de>
>> 
>> This grew quite a bit since v6 (legacy desc changes, stmmac_rx() etc),
>> so my R-b should have been dropped. I had another look and two small
>> notes below.
> 
> Hi Nicolai,
> 
> ack, right.
> 
>> 
>> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at oss.qualcomm.com>
>> > ---
>> 
>> > [...]
>> 
>> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> > @@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(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)) {
>> > -			xsk_buff_free(buf->xdp);
>> > -			buf->xdp = NULL;
>> > -			dirty++;
>> > +
>> > +		if (unlikely(status & discard_frame)) {
>> >  			error = 1;
>> >  			if (!priv->hwts_rx_en)
>> >  				rx_errors++;
>> >  		}
>> 
>> There's no !error check here, so on the legacy descriptors a split
>> frame now counts once per descriptor in rx_errors. stmmac_rx() in 3/5
>> counts it once. Could you add the !error check here too?
> 
> ack, I agree we should align stmmac_rx() and stmmac_rx_zc(). In order 
> to avoid
> reposting the full series for this change, do you agree to address it 
> as a
> follow-up patch?

Would be ok for me if nothing else is flagged in this version. In the 
end it will
be Maxime's / the net dev maintainers decision :)

> 
> Regards,
> Lorenzo
> 
>> 
>> > [...]
>> 
>> > @@ -5883,7 +5887,8 @@ 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)) {
>> > +
>> > +		if (unlikely(status & discard_frame)) {
>> 
>> AFAIU this also changes GMAC4: a frame with an error plus an IP header
>> error returns discard_frame | csum_none, which the old "==" let
>> through. Dropping it is right, but maybe worth a line in the commit
>> message?
>> 
>> > [...]
>> 
>> With the !error check added:
>> 
>> Reviewed-by: Nicolai Buchwitz <nb at tipi-net.de>
>> 
>> Thanks,
>> Nicolai