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

Nicolai Buchwitz
2026-04-06T18:52:41Z

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.

> …