Re: [PATCH net-next v9 5/7] net: bcmgenet: add XDP_REDIRECT and ndo_xdp_xmit support

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

Hi Jacub

On 10.5.2026 20:55, 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_REDIRECT and ndo_xdp_xmit support
> 
> This commit adds XDP_REDIRECT handling in the RX path and implements
> ndo_xdp_xmit so other devices can redirect frames into bcmgenet via
> ring 16. It also advertises NETDEV_XDP_ACT_REDIRECT and
> NETDEV_XDP_ACT_NDO_XMIT in xdp_features.
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index f1e515526787..4e4fe785f0bf 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> 
> [ ... ]
> 
>> @@ -4027,6 +4048,36 @@ static int bcmgenet_xdp(struct net_device *dev, 
>> struct netdev_bpf *xdp)
>>  	}
>>  }
>> 
>> +static int bcmgenet_xdp_xmit(struct net_device *dev, int num_frames,
>> +			     struct xdp_frame **frames, u32 flags)
>> +{
>> +	struct bcmgenet_priv *priv = netdev_priv(dev);
>> +	struct bcmgenet_tx_ring *ring = &priv->xdp_tx_ring;
>> +	int sent = 0;
>> +	int i;
>> +
>> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>> +		return -EINVAL;
>> +
>> +	if (unlikely(!netif_running(dev)))
>> +		return -ENETDOWN;
>> +
>> +	spin_lock(&ring->lock);
>> +
>> +	for (i = 0; i < num_frames; i++) {
>> +		if (!bcmgenet_xdp_xmit_frame(priv, ring, frames[i], true))
>> +			break;
>> +		sent++;
>> +	}
>> +
>> +	if (sent)
>> +		bcmgenet_xdp_ring_doorbell(priv, ring);
>> +
>> +	spin_unlock(&ring->lock);
>> +
>> +	return sent;
>> +}
> 
> Can this path make forward progress when bcmgenet is used purely as an
> XDP_REDIRECT destination (for example, redirecting from a USB NIC or
> Wi-Fi adapter into the on-SoC bcmgenet)?
> 
> The xdp_tx_ring (ring DESC_INDEX) has no dedicated TX interrupt and no
> TX NAPI.  bcmgenet_init_tx_ring() explicitly skips the TX NAPI for this
> ring:
> 
>     /* Initialize Tx NAPI for priority queues only; ring DESC_INDEX
>      * (XDP TX) has its completions handled inline in RX NAPI.
>      */
>     if (index != DESC_INDEX)
>         netif_napi_add_tx(priv->dev, &ring->napi, bcmgenet_tx_poll);
> 
> The only place that reclaims completed descriptors on this ring is
> bcmgenet_rx_poll():
> 
>     static int bcmgenet_rx_poll(struct napi_struct *napi, int budget)
>     {
>         ...
>         /* 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);
>         ...
>     }
> 
> Before this patch, the only producer on this ring was XDP_TX from
> bcmgenet's own RX path, so reclaim and production were always paired
> with RX NAPI.
> 
> With ndo_xdp_xmit, a remote device becomes a producer on the same
> ring.  If bcmgenet itself has no (or very sparse) RX traffic, its RX
> NAPI is never scheduled, bcmgenet_tx_reclaim() on xdp_tx_ring is never
> called, free_bds drains to 0, and:
> 
>> +	for (i = 0; i < num_frames; i++) {
>> +		if (!bcmgenet_xdp_xmit_frame(priv, ring, frames[i], true))
>> +			break;
>> +		sent++;
>> +	}
> 
> bcmgenet_xdp_xmit_frame() returns false on the free_bds < 1 check and
> bcmgenet_xdp_xmit() returns 0, so bq_xmit_all() in the redirect caller
> counts every frame as a drop.
> 
> There does not appear to be a TX completion IRQ, a watchdog, or a
> napi_schedule() kick from bcmgenet_xdp_xmit() to trigger reclaim
> independently of RX traffic.  Is there something else that drains the
> ring in this configuration, or does ndo_xdp_xmit need to schedule the
> RX NAPI (or an independent reclaim path) when it enqueues frames?
> 
>> @@ -4341,7 +4393,8 @@ static int bcmgenet_probe(struct platform_device 
>> *pdev)
>>  			 NETIF_F_RXCSUM;
>>  	dev->hw_features |= dev->features;
>>  	dev->vlan_features |= dev->features;
>> -	dev->xdp_features = NETDEV_XDP_ACT_BASIC;
>> +	dev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
>> +			    NETDEV_XDP_ACT_NDO_XMIT;
> 
> Relatedly, advertising NETDEV_XDP_ACT_NDO_XMIT tells user space and
> redirect maps that this device is a usable xmit target.  Should this
> bit only be advertised once ring 16 has an independent completion
> source, or is the expectation that users of this feature will always
> have concurrent RX traffic on bcmgenet to drive the reclaim?

You're right, ring 16 has no independent completion source. The
piggyback on RX NAPI works for XDP_TX since RX and TX are always
paired there, but ndo_xdp_xmit has no such guarantee.

I see two paths forward:

1. Drop ndo_xdp_xmit and NETDEV_XDP_ACT_NDO_XMIT from this series.
    XDP_TX, XDP_PASS, XDP_DROP, and XDP_REDIRECT (originating on
    bcmgenet itself) still work. ndo_xdp_xmit support comes back in a
    follow-up with a dedicated completion mechanism (workqueue-based
    reclaim, or scheduling RX NAPI from the xmit path).

2. Keep ndo_xdp_xmit and schedule the RX NAPI from ndo_xdp_xmit so
    reclaim runs regardless of RX traffic. Other XDP drivers either
    have dedicated TX IRQs (bnxt) or share normal TX queues with IRQs
    (mvneta) so there's no direct precedent for this hack.

I'd lean toward (1) for v10 to get the rest of the series merged, and
address the completion infrastructure properly in a follow-up. Does
that work for you?

@Justin / Florian: If you have any other ideas for this, happy to hear
your thoughts.

Thanks
Nicolai