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

[Nicolai Buchwitz <nb@tipi-net.de>]

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