Re: [PATCH net-next v6 3/7] net: bcmgenet: add basic XDP support (PASS/DROP)

Jakub Kicinski
2026-04-12T19:23:01Z

On Mon, 6 Apr 2026 10:35:27 +0200 Nicolai Buchwitz wrote:
> Add XDP program attachment via ndo_bpf and execute XDP programs in the
> RX path. XDP_PASS builds an SKB from the xdp_buff (handling
> xdp_adjust_head/tail), XDP_DROP returns the page to page_pool without
> SKB allocation.
>
> XDP_TX and XDP_REDIRECT are not yet supported and return XDP_ABORTED.
>
> Advertise NETDEV_XDP_ACT_BASIC in xdp_features.

> - skb_mark_for_recycle(skb);
> -
> - /* Reserve the RSB + pad, then set the data length */
> - skb_reserve(skb, GENET_RSB_PAD);
> - __skb_put(skb, len - GENET_RSB_PAD);
> + {

floating code blocks are considered poor coding style in the kernel
Why not push the variables up into the outer scope or make this
a helper?

> + struct xdp_buff xdp;
> + unsigned int xdp_act;
> + int pkt_len;
> +
> + pkt_len = len - GENET_RSB_PAD;
> + if (priv->crc_fwd_en)
> + pkt_len -= ETH_FCS_LEN;
> +
> + /* Save rx_csum before XDP runs - an XDP program
> + * could overwrite the RSB via bpf_xdp_adjust_head.
> + */
> + if (dev->features & NETIF_F_RXCSUM)
> + rx_csum = (__force __be16)(status->rx_csum
> + & 0xffff);

FWIW this could be before the block

> + xdp_init_buff(&xdp, PAGE_SIZE, &ring->xdp_rxq);
> + xdp_prepare_buff(&xdp, page_address(rx_page),
> + GENET_RX_HEADROOM, pkt_len, true);
> +
> + if (xdp_prog) {
> + xdp_act = bcmgenet_run_xdp(ring, xdp_prog,
> + &xdp, rx_page);

Since you pass the xdp_prog in you can save yourself the indentation by
making bcmgenet_run_xdp() return PASS when no program is set.
bcmgenet_run_xdp() has one caller, it's going to get inlined.

> + if (xdp_act != XDP_PASS)
> + goto next;
> + }
>
> - if (priv->crc_fwd_en) {
> - skb_trim(skb, skb->len - ETH_FCS_LEN);
> + skb = bcmgenet_xdp_build_skb(ring, &xdp);
> + if (unlikely(!skb)) {
> + BCMGENET_STATS64_INC(stats, dropped);
> + page_pool_put_full_page(ring->page_pool,
> + rx_page, true);
> + goto next;
> + }
> }
>
> /* Set up checksum offload */
> if (dev->features & NETIF_F_RXCSUM) {
> - rx_csum = (__force __be16)(status->rx_csum & 0xffff);
> if (rx_csum) {
> skb->csum = (__force __wsum)ntohs(rx_csum);
> skb->ip_summed = CHECKSUM_COMPLETE;
> @@ -3744,6 +3810,37 @@ static int bcmgenet_change_carrier(struct net_device *dev, bool new_carrier)
> return 0;
> }
>
> +static int bcmgenet_xdp_setup(struct net_device *dev,
> + struct netdev_bpf *xdp)
> +{
> + struct bcmgenet_priv *priv = netdev_priv(dev);
> + struct bpf_prog *old_prog;
> + struct bpf_prog *prog = xdp->prog;
> +
> + if (prog && dev->mtu > PAGE_SIZE - GENET_RX_HEADROOM -
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))) {

I'm confused by this check, it appears that the max page size this
driver can Rx in the first place is 2kB (RX_BUF_LENGTH). And max_mtu
is 1.5kB.

If GENET_RX_HEADROOM + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
is larger than 2kB the Rx path will break completely whether XDP was
attached or not.

This check seems to be cargo culting what other drivers do?