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

[Nicolai Buchwitz]

On 2.4.2026 05:27, Jakub Kicinski wrote:
> On Sun, 29 Mar 2026 00:05:06 +0100 Nicolai Buchwitz wrote:
> > @@ -2403,26 +2456,52 @@ static unsigned int bcmgenet_desc_rx(struct 
> > bcmgenet_rx_ring *ring,
> >  			goto next;
> >  		} /* error packet */
> >
> > -		/* Build SKB from the page - data starts at hard_start,
> > -		 * frame begins after RSB(64) + pad(2) = 66 bytes.
> > -		 */
> > -		skb = napi_build_skb(hard_start, PAGE_SIZE - GENET_XDP_HEADROOM);
> > -		if (unlikely(!skb)) {
> > -			BCMGENET_STATS64_INC(stats, dropped);
> > -			page_pool_put_full_page(ring->page_pool, rx_page,
> > -						true);
> > -			goto next;
> > -		}
> > -
> > -		skb_mark_for_recycle(skb);
> > +		/* XDP: frame data starts after RSB + pad */
> > +		if (xdp_prog) {
> > +			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;
> > +
> > +			xdp_init_buff(&xdp, PAGE_SIZE, &ring->xdp_rxq);
> > +			xdp_prepare_buff(&xdp, page_address(rx_page),
> > +					 GENET_RX_HEADROOM, pkt_len, true);
> > +
> > +			xdp_act = bcmgenet_run_xdp(ring, xdp_prog, &xdp,
> > +						   rx_page);
> > +			if (xdp_act != XDP_PASS)
> > +				goto next;
> > +
> > +			/* XDP_PASS: build SKB from (possibly modified) xdp */
> > +			skb = bcmgenet_xdp_build_skb(ring, &xdp, rx_page);
> > +			if (unlikely(!skb)) {
> > +				BCMGENET_STATS64_INC(stats, dropped);
> > +				page_pool_put_full_page(ring->page_pool,
> > +							rx_page, true);
> > +				goto next;
> > +			}
> > +		} else {
> > +			/* Build SKB from the page - data starts at
> > +			 * hard_start, frame begins after RSB(64) + pad(2).
> > +			 */
> > +			skb = napi_build_skb(hard_start,
> > +					     PAGE_SIZE - GENET_XDP_HEADROOM);
> > +			if (unlikely(!skb)) {
> > +				BCMGENET_STATS64_INC(stats, dropped);
> > +				page_pool_put_full_page(ring->page_pool,
> > +							rx_page, true);
> > +				goto next;
> > +			}
>
> The large branches here are quite unusual, normally drivers fully
> prepare the xdp_buff and if there's no xdp prog attached act as
> if there was one and it returned XDP_PASS. Saves LoC and therefore 
> bugs.

Agreed, will refactor to always prepare xdp_buff and use
bcmgenet_xdp_build_skb for both paths in v6.

> > -		/* Reserve the RSB + pad, then set the data length */
> > -		skb_reserve(skb, GENET_RSB_PAD);
> > -		__skb_put(skb, len - GENET_RSB_PAD);
> > +			skb_mark_for_recycle(skb);
> > +			skb_reserve(skb, GENET_RSB_PAD);
> > +			__skb_put(skb, len - GENET_RSB_PAD);
> >
> > -		if (priv->crc_fwd_en) {
> > -			skb_trim(skb, skb->len - ETH_FCS_LEN);
> > -			len -= ETH_FCS_LEN;
> > +			if (priv->crc_fwd_en)
> > +				skb_trim(skb, skb->len - ETH_FCS_LEN);
> >  		}
> >
> >  		/* Set up checksum offload */
>
> AI points out that :
>
>   The status_64 structure is located at the start of the page
>   before the frame data. Since this resides inside the XDP headroom, if 
> an XDP
>   program expands the header backwards (e.g., via bpf_xdp_adjust_head() 
> or
>   metadata via bpf_xdp_adjust_meta()), could it physically overwrite
>   status->rx_csum?
>
>   Since the driver reads status->rx_csum after executing the XDP 
> program, it
>   could risk reading garbage data if the headroom was used. Should the 
> driver
>   skip setting CHECKSUM_COMPLETE when an XDP program is loaded?

Makes sense, status->rx_csum is read after XDP runs and an
adjust_head could overwrite the RSB. I'll save rx_csum before
running the XDP program rather than skipping CHECKSUM_COMPLETE
entirely, so XDP_PASS packets that weren't modified still benefit
from HW checksum offload.

> > @@ -3745,6 +3824,39 @@ 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))) {
> > +		NL_SET_ERR_MSG_MOD(xdp->extack,
> > +				   "MTU too large for single-page XDP buffer");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	old_prog = xchg(&priv->xdp_prog, prog);
> > +	if (old_prog) {
> > +		synchronize_net();
>
> Why? BPF prog gets freed after a RCU grace period I think?

You're right, bpf_prog_put handles this via call_rcu. Will remove
the synchronize_net().

> > +		bpf_prog_put(old_prog);

Thanks
Nicolai