Re: [PATCH net-next v6 12/14] net: lan966x: add PCIe FDMA XDP support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Inconsistent XDP frame presentation between the two RX hooks
  of the same driver: after this patch lan966x has two in-tree XDP RX…

--

net: lan966x: add PCIe FDMA XDP support

This adds XDP support to the lan966x PCIe FDMA path, using contiguous
ATU-mapped buffers and a memcpy-based XDP_TX instead of the page_pool
scheme used by the platform path. The IFH and FCS are stripped before
the BPF program runs, and lan966x_xdp_setup() now skips the page_pool
reload for the PCIe case. Only XDP_ACT_BASIC is advertised for PCIe.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> index 6cabbb8b47f2..02c021617a3e 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c

[ ... ]

> @@ -126,7 +127,123 @@ static bool lan966x_fdma_pci_rx_size_fits(struct fdma *fdma, u32 blockl)

[ ... ]

> +static int lan966x_xdp_pci_run(struct lan966x_port *port, void *data,
> +			       u32 data_len, void **xdp_data, u32 *xdp_len)
> +{

[ ... ]

> +	xdp_prepare_buff(&xdp,
> +			 data - XDP_PACKET_HEADROOM,
> +			 XDP_PACKET_HEADROOM + IFH_LEN_BYTES,
> +			 data_len,
> +			 false);
> +
> +	act = bpf_prog_run_xdp(xdp_prog, &xdp);

[ ... ]

> @@ -158,38 +275,33 @@ static int lan966x_fdma_pci_rx_check_frame(struct lan966x_rx *rx, u64 *src_port)
>  	if (!lan966x_fdma_pci_rx_size_fits(fdma, blockl))
>  		return FDMA_ERROR;
>  
> -	return FDMA_PASS;
> +	/* Present the Ethernet frame (no IFH, no FCS). HW re-inserts the
> +	 * FCS on TX; see lan966x_fdma_pci_xmit_xdpf(). May be overridden
> +	 * by XDP. The FCS strip is unconditional because NETIF_F_RXFCS
> +	 * is not advertised in hw_features.
> +	 */
> +	*data = virt_addr + IFH_LEN_BYTES;
> +	*data_len = blockl - IFH_LEN_BYTES - ETH_FCS_LEN;
> +
> +	return lan966x_xdp_pci_run(port, virt_addr, *data_len, data, data_len);
>  }

[Severity: Medium]
Should the two lan966x XDP receive hooks agree on whether the FCS is part
of the window handed to the BPF program?

On the PCIe path, lan966x_fdma_pci_rx_check_frame() subtracts the FCS
before calling into the program:

	*data_len = blockl - IFH_LEN_BYTES - ETH_FCS_LEN;

	return lan966x_xdp_pci_run(port, virt_addr, *data_len, data, data_len);

On the platform path, lan966x_fdma_rx_check_frame() passes the raw BLOCKL
value and lan966x_xdp_run() only removes the IFH:

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_rx_check_frame() {
	...
	return lan966x_xdp_run(port, page, FDMA_DCB_STATUS_BLOCKL(db->status));
}

drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c:lan966x_xdp_run() {
	...
	xdp_prepare_buff(&xdp, page_address(page),
			 IFH_LEN_BYTES + XDP_PACKET_HEADROOM,
			 data_len - IFH_LEN_BYTES, false);
}

That BLOCKL value still contains the FCS, as shown by the trim done later
on the same path:

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_rx_get_frame() {
	...
	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
	...
	skb_pull(skb, IFH_LEN_BYTES);

	if (likely(!(skb->dev->features & NETIF_F_RXFCS)))
		skb_trim(skb, skb->len - ETH_FCS_LEN);
}

So xdp.data_end points four bytes past the end of the Ethernet payload on
the platform path, and exactly at the end of the payload on the new PCIe
path. Both windows stay inside the DMA buffer, so this looks like a
behavioural difference rather than an out-of-bounds access.

A program that parses backwards from data_end, compares data_end - data
against an L3 length field, or trims via bpf_xdp_adjust_tail() would get
different results depending on whether the switch is platform- or
PCIe-attached. The PCIe framing matches the usual XDP convention of no
FCS, which would make the platform hook the one that differs from
convention.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index e089c08b9c37..9ada497767c6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -874,11 +874,13 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
>  
>  	port->phylink = phylink;
>  
> -	/* XDP is not supported on the PCIe FDMA path. */
> -	if (lan966x->fdma && !lan966x_is_pci(lan966x))
> -		dev->xdp_features = NETDEV_XDP_ACT_BASIC |
> -				    NETDEV_XDP_ACT_REDIRECT |
> -				    NETDEV_XDP_ACT_NDO_XMIT;
> +	if (lan966x->fdma) {
> +		dev->xdp_features = NETDEV_XDP_ACT_BASIC;
> +
> +		if (!lan966x_is_pci(lan966x))
> +			dev->xdp_features |= NETDEV_XDP_ACT_REDIRECT |
> +					     NETDEV_XDP_ACT_NDO_XMIT;
> +	}

[Severity: Medium]
Both paths now report the same NETDEV_XDP_ACT_BASIC bit from
lan966x_probe_port(), so is there anything in the uAPI that lets a
program distinguish the FCS-included window of lan966x_xdp_run() from
the FCS-stripped window of lan966x_xdp_pci_run()?

Would it make sense to make one of the two hooks match the other so that
the frame presentation is the same for identically-advertised netdevs?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671%40microchip.com