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

[Daniel Machon]

> 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.
>

They should, and the PCIe framing is the one to keep. The strip was asked for by an
earlier round of this review, in v3, because the PCIe hook honours the post-program
window - lan966x_xdp_pci_run() writes back xdp.data_end - xdp.data and XDP_TX uses
it - so an FCS left in there would be delivered as payload whenever a program
touched the tail. The platform hook does not use them for XDP_PASS or XDP_TX at
all - it passes the pre-program length instead.

> > 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?

Same as above - the correct handling is removing the FCS, so the platform hook
is the one to move. There is no uAPI signal to describe the difference with
either, so converging is the only option.

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