Re: [PATCH net-next v5 4/6] net: bcmgenet: add XDP_TX support

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

On 2.4.2026 05:27, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: bcmgenet: add XDP_TX support
> 
> This commit implements XDP_TX using ring 16, which is dedicated to XDP 
> TX
> for isolation from SKB TX queues. It also changes the page_pool DMA
> direction to bidirectional to allow TX reuse of the existing DMA 
> mapping.
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index b45ba2c2857e..687c3b12d44f 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> [ ... ]
>> @@ -2299,11 +2328,96 @@ static struct sk_buff 
>> *bcmgenet_xdp_build_skb(struct bcmgenet_rx_ring *ring,
>>  	return skb;
>>  }
>> 
>> +static bool bcmgenet_xdp_xmit_frame(struct bcmgenet_priv *priv,
>> +				     struct xdp_frame *xdpf, bool dma_map)
>> +{
> [ ... ]
>> +	} else {
>> +		struct page *page = virt_to_page(xdpf->data);
>> +
>> +		/* For local XDP_TX the caller already prepended the TSB
>> +		 * into xdpf->data/len, so dma_len == xdpf->len.
>> +		 */
>> +		dma_len = xdpf->len;
>> +		mapping = page_pool_get_dma_addr(page) +
>> +			  sizeof(*xdpf) + xdpf->headroom;
> 
> When the RX page pool is created in bcmgenet_rx_ring_create_pool(), it 
> is
> initialized with .offset = GENET_XDP_HEADROOM. Does the DMA address
> calculation here need to include this GENET_XDP_HEADROOM offset? 
> Without it,
> it appears the hardware might be given a DMA address that is 256 bytes
> before the actual packet data, transmitting uninitialized memory.

The DMA address calculation is correct. page_pool_get_dma_addr()
returns the base DMA address of the page, not offset by the pool's
.offset parameter. sizeof(*xdpf) + xdpf->headroom correctly offsets
to xdpf->data where the TSB starts.

> 
> [ ... ]
>> @@ -2311,14 +2425,42 @@ static unsigned int bcmgenet_run_xdp(struct 
>> bcmgenet_rx_ring *ring,
>>  	switch (act) {
>>  	case XDP_PASS:
>>  		return XDP_PASS;
>> +	case XDP_TX:
>> +		/* Prepend a zeroed TSB (Transmit Status Block).  The GENET
>> +		 * MAC has TBUF_64B_EN set globally, so hardware expects every
>> +		 * TX buffer to begin with a 64-byte struct status_64.  Back
>> +		 * up xdp->data into the RSB area (which is no longer needed
>> +		 * after the BPF program ran) and zero it.
>> +		 */
>> +		if (xdp->data - xdp->data_hard_start <
>> +		    sizeof(struct status_64)) {
>> +			page_pool_put_full_page(ring->page_pool, rx_page,
>> +						true);
>> +			return XDP_DROP;
>> +		}
>> +		xdp->data -= sizeof(struct status_64);
>> +		xdp->data_meta -= sizeof(struct status_64);
>> +		memset(xdp->data, 0, sizeof(struct status_64));
>> +
>> +		xdpf = xdp_convert_buff_to_frame(xdp);
> 
> Does this headroom check account for the additional sizeof(struct 
> xdp_frame)
> required by xdp_convert_buff_to_frame()?

The TSB headroom check doesn't account for sizeof(struct xdp_frame), but 
xdp_update_frame_from_buff() has its own check that returns -ENOSPC if 
headroom - metasize < sizeof(*xdp_frame). We handle the NULL return by 
dropping the frame. I'll tighten the check to include sizeof(struct 
xdp_frame) to fail early.

> 
> If metadata is present, xdp->data_meta could potentially underflow
> xdp->data_hard_start. If the remaining headroom minus metasize becomes
> negative in xdp_convert_buff_to_frame(), could it undergo integer 
> promotion
> to a large unsigned value and bypass the internal safety check? This 
> might
> cause the struct xdp_frame to overwrite the newly zeroed status_64 
> block.

xdp_update_frame_from_buff() handles this - it clamps metasize to >= 0 
and checks (headroom - metasize) < sizeof(*xdp_frame), returning NULL on 
insufficient space.

> 
> [ ... ]
>> @@ -2556,9 +2698,15 @@ static int bcmgenet_rx_poll(struct napi_struct 
>> *napi, int budget)
>>  {
>>  	struct bcmgenet_rx_ring *ring = container_of(napi,
>>  			struct bcmgenet_rx_ring, napi);
>> +	struct bcmgenet_priv *priv = ring->priv;
>>  	struct dim_sample dim_sample = {};
>>  	unsigned int work_done;
>> 
>> +	/* Reclaim completed XDP TX frames (ring 16 has no interrupt) */
>> +	if (priv->xdp_prog)
>> +		bcmgenet_tx_reclaim(priv->dev,
>> +				    &priv->xdp_tx_ring, false);
>> +
> 
> What happens to in-flight frames if the XDP program is detached at 
> runtime
> and priv->xdp_prog becomes NULL? Will this permanently lose track of 
> the
> page pool buffers and DMA mappings since the NAPI poll stops reclaiming
> them?

Good catch. The reclaim in rx_poll is gated on priv->xdp_prog, so
in-flight frames on the TX ring would never be reclaimed after detach.
I'll change the condition to check for pending frames on the ring
instead.

> 
> Additionally, if the interface is the target of an XDP_REDIRECT from 
> another
> device, a local XDP program is not required and priv->xdp_prog could be
> NULL. Does this mean the transmitted frames for redirected traffic will
> never be reclaimed, eventually filling the TX ring?

The redirect-without-local-prog case can't happen since
xdp_features_set_redirect_target is tied to program attachment.
But the detach race is real - will fix in v6.

Thanks
Nicolai