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

[Jakub Kicinski]

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?