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

Nicolai Buchwitz
2026-04-02T08:19:52Z

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