Re: [PATCH net-next v6 4/7] net: bcmgenet: add XDP_TX support

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

On 6.4.2026 10:35, Nicolai Buchwitz wrote:
> Implement XDP_TX using ring 16 (DESC_INDEX), the hardware default
> descriptor ring, dedicated to XDP TX for isolation from SKB TX queues.

> [...]

> +
> +	if (dma_map) {
> +		void *tsb_start;
> +
> +		/* The GENET MAC has TBUF_64B_EN set globally, so hardware
> +		 * expects a 64-byte TSB prefix on every TX buffer.  For
> +		 * redirected frames (ndo_xdp_xmit) we prepend a zeroed TSB
> +		 * using the frame's headroom.
> +		 */
> +		if (unlikely(xdpf->headroom < sizeof(struct status_64))) {
> +			bcmgenet_put_txcb(priv, ring);
> +			spin_unlock(&ring->lock);
> +			return false;
> +		}
> +
> +		tsb_start = xdpf->data - sizeof(struct status_64);
> +		memset(tsb_start, 0, sizeof(struct status_64));

Sashiko:
Does this memset() overwrite the struct xdp_frame? The xdp_frame is 
typically
stored in the headroom immediately before xdpf->data (or before the 
metadata).
If the 64-byte status_64 block is zeroed out here, could it destroy the
xdp_frame memory structure and cause a panic when xdp_return_frame() is 
later
called during TX completion?

Reponse:
No. The xdp_frame struct ends at data_hard_start + sizeof(xdp_frame).
The headroom (xdpf->headroom) is the gap between that and xdpf->data.
The TSB is written at xdpf->data - 64, which falls inside the headroom, 
after
the xdp_frame. The check xdpf->headroom < sizeof(struct status_64) 
prevents overlap.

> +
> +		dma_len = xdpf->len + sizeof(struct status_64);
> +		mapping = dma_map_single(kdev, tsb_start, dma_len,
> +					 DMA_TO_DEVICE);
> +		if (dma_mapping_error(kdev, mapping)) {
> +			tx_cb_ptr->skb = NULL;
> +			tx_cb_ptr->xdpf = NULL;
> +			bcmgenet_put_txcb(priv, ring);
> +			spin_unlock(&ring->lock);
> +			return false;
> +		}
> +	} 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;

Sashiko:
Is this DMA address calculation correct? It evaluates to the offset of
xdp->data_meta relative to xdp->data_hard_start, which completely drops 
the
GENET_XDP_HEADROOM (256 bytes) offset applied to the page pool base 
address.
Also, if metadata is present, this will point to the metadata instead of
xdpf->data. Could this cause the hardware to read the wrong memory 
location
for the Transmit Status Block and truncate the payload?

Reponse:
Correct. page_pool_get_dma_addr() returns the DMA address of the page 
start,
not offset by the pool's .offset. sizeof(*xdpf) + xdpf->headroom equals
xdpf->data - data_hard_start, pointing to the TSB. No metadata issue:
for local XDP_TX, data already includes the prepended TSB.

> +		dma_sync_single_for_device(kdev, mapping, dma_len,

> [...]

>  	}
> @@ -2542,9 +2684,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_tx_ring.free_bds < priv->xdp_tx_ring.size)
> +		bcmgenet_tx_reclaim(priv->dev,
> +				    &priv->xdp_tx_ring, false);

Sashiko:
Since the XDP TX ring capacity is 32 (GENET_Q16_TX_BD_CNT) and the NAPI
budget can be up to 64, could a burst of XDP_TX packets exhaust the TX 
ring?
Because the reclaim only happens once at the beginning of the poll, 
would
subsequent XDP_TX packets within the same poll be dropped when the ring
fills up?

Reponse:
By design. If the ring fills, frames are dropped, which is kinda 
standard XDP backpressure.
Same approach as bnxt, mvneta, and other piggybacked-reclaim drivers.
Adding mid-poll reclaim would add complexity for probably minimal 
benefit at 1Gbps.

> [...]