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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch changes the discard test from `status == discard_frame`
  to `status & discard_frame` in both stmmac_rx() and stmmac_rx_zc().
- [Low] After this patch, stmmac_rx_zc() counts the same event (a frame
  split across RX descriptors on an AF_XDP zero-copy queue) differently…
- [Low] In stmmac_rx_zc() on dwmac4/XGMAC, a split frame can now be
  counted in both rx_dropped and rx_errors.

Pre-existing issues:
- [High] At this commit, stmmac_rx() (non-ZC, XDP attached, GMAC4/XGMAC
  whose non-last descriptors report bare rx_not_ls) still drops a non-…
- [Medium] stmmac_rx_zc() and stmmac_rx() add their local
  rx_dropped/rx_errors into the device-wide `priv->xstats.rx_dropped`…

--

net: stmmac: drop split frames on AF_XDP zero-copy RX

In stmmac_rx_zc(), when the 1:1 XSK mapping check drops a non-last
descriptor, this patch now marks the frame as errored. The rest of the
split frame is then drained, and its tail is no longer handled as a new
frame. The patch also makes the legacy enhanced and normal descriptor
callbacks report rx_not_ls together with discard_frame, and changes the
discard test in stmmac_rx_zc() and stmmac_rx() from == to &.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> index 0512536012251..297e2428a8f9b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> @@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
>  
>  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
>  		x->rx_length++;
> -		return discard_frame;
> +		return discard_frame | rx_not_ls;
>  	}

[Severity: Low]

With this change and the matching one in ndesc_get_rx_status(), does
stmmac_rx_zc() now count the same split frame differently depending on
the descriptor format?

On dwmac4 and xgmac, dwmac4_wrback_get_rx_status() and
dwxgmac2_get_rx_status() return a bare rx_not_ls for a non-last
descriptor:

	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
		return rx_not_ls;

So the frame reaches the new 1:1 mapping drop and rx_dropped is
incremented once.

On dwmac100/dwmac1000, non-last descriptors now carry discard_frame. The
status & discard_frame block in stmmac_rx_zc() runs first and increments
rx_errors for each non-last descriptor (when hwts_rx_en is off). The new
rx_dropped++ in the 1:1 path is never reached on these cores.

An N-descriptor jumbo frame would then show up as 1 rx_dropped on
GMAC4/XGMAC, but as N-1 rx_errors on dwmac1000. Both counters are
exported through stmmac_get_stats64(). Is this asymmetry intended?

The legacy cores already counted rx_errors per descriptor before this
patch; what is new is the different rx_dropped accounting. Later in the
series, "net: stmmac: rework stmmac_rx to support XDP rx multi-buff"
changes stmmac_rx() to test !error && (status & discard_frame), so it
counts once per frame. stmmac_rx_zc() still counts per descriptor at the
end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8fd2acb25fb60..efa88cd359dac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5715,8 +5718,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (likely(status & rx_not_ls)) {
>  			xsk_buff_free(buf->xdp);
>  			buf->xdp = NULL;
> +			rx_dropped++;
> +			error = 1;
>  			dirty++;
> -			count++;
>  			goto read_again;
>  		}
>  

[Severity: Low]

Can one split frame on dwmac4/XGMAC now be counted in both rx_dropped and
rx_errors?

The first non-last descriptor takes this path, which increments
rx_dropped and sets error = 1. Suppose the last descriptor of the same
frame then reports discard_frame (RDES3_ERROR_SUMMARY, for example a CRC
or giant error on an oversized frame). The earlier block in
stmmac_rx_zc() still runs:

		if (unlikely(status & discard_frame)) {
			error = 1;
			if (!priv->hwts_rx_en)
				rx_errors++;
		}

This happens even though error is already set for this frame.
stmmac_get_stats64() exports both totals as rx_dropped and rx_errors.

Later in the series, "net: stmmac: rework stmmac_rx to support XDP rx
multi-buff" avoids this in stmmac_rx() by testing !error && (status &
discard_frame). stmmac_rx_zc() is not changed, so the double count is
still there at the end of the series.

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch. The
new rx_dropped++ feeds this accumulation at the end of stmmac_rx_zc():

	priv->xstats.rx_dropped += rx_dropped;
	priv->xstats.rx_errors += rx_errors;

These are plain unsigned long fields in struct stmmac_extra_stats,
shared by the whole device. Each RX channel has its own NAPI instance,
so stmmac_rx_zc() and stmmac_rx() can run at the same time for different
queues on different CPUs.

Can these non-atomic read-modify-writes lose updates?
stmmac_get_stats64() also reads the fields without any synchronization.

stmmac_rx() does the same accumulation, and it is unchanged at the end
of the series.

> @@ -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)) {

[Severity: Low]

Changing == to & here and in stmmac_rx_zc() also changes the normal RX
path on dwmac4/GMAC4/GMAC5. Should the commit message mention this?

dwmac4_wrback_get_rx_status() sets ret = discard_frame when
RDES3_ERROR_SUMMARY is set. It then ORs in csum_none for IP header or
payload errors:

	if (rdes1 & RDES1_IP_HDR_ERROR) {
		x->ip_hdr_err++;
		ret |= csum_none;
	}

So a frame with both an L2 error and an IP checksum error reports
discard_frame | csum_none.

Before this patch, such a frame failed the == test. It was run through
XDP and passed to napi_gro_receive() with CHECKSUM_NONE. Now it is
dropped and counted in rx_errors.

The new behaviour looks correct. It stops errored frames from reaching
the stack, which has been possible since commit 644b8437ccef9e ("net:
stmmac: dwmac4: report Rx checksum errors in status").

However, the commit message presents the operator change only as a
result of the enh_desc/norm_desc normalization, and limits the scope to
AF_XDP zero-copy. Could it describe this GMAC4 change and reference
644b8437ccef9e?

>  			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
>  			buf->page = NULL;
>  			error = 1;

[Severity: High]

This isn't a bug introduced by this patch. The commit message says the
change lets stmmac_rx() drain split frames, which holds for
dwmac100/dwmac1000.

GMAC4/XGMAC report non-last descriptors as a bare rx_not_ls. On those
cores, with an XDP program attached, are there still paths in
stmmac_rx() that drop a non-last descriptor without setting error?

When the XDP verdict is STMMAC_XDP_CONSUMED:

				if (xdp_res & STMMAC_XDP_CONSUMED) {
					...
					skb = NULL;

					if (unlikely((status & rx_not_ls)))
						goto read_again;

error is still 0 here, so the next fragment goes through XDP as a new
frame. With a 1-3 byte tail, wouldn't buf1_len -= ETH_FCS_LEN underflow?

For XDP_TX/XDP_REDIRECT:

				} else if (xdp_res & (STMMAC_XDP_TX |
						      STMMAC_XDP_REDIRECT)) {
					xdp_status |= xdp_res;
					buf->page = NULL;
					goto next;
				}

goto next clears in_progress even when rx_not_ls is set. The loop head
then resets error and len, and the tail is reparsed with len == 0.

When napi_build_skb() fails:

				rx_dropped++;
				count++;
				goto drain_data;

error is not set here either, so a later descriptor can build an skb from
only the end of the frame.

This is the same class of bug the patch fixes in stmmac_rx_zc(). An
on-link peer can trigger it, since GMAC_CONFIG_JE is always set on
GMAC4.

It looks like later patches in the same series fix it: "net: stmmac:
drop multi-buffer frames in XDP mode" and "net: stmmac: rework stmmac_rx
to support XDP rx multi-buff". After those, XDP and stmmac_build_skb()
only run on the last descriptor, and failures go to error_free_frag.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com