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

Jakub Kicinski
2026-05-01T01:40:01Z

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 patch adds XDP_TX support to bcmgenet by dedicating ring 16
(DESC_INDEX) to XDP TX, carving 32 BDs from ring 0's allocation and
piggybacking TX completion on the RX NAPI poll. The page_pool DMA
direction is switched from DMA_FROM_DEVICE to DMA_BIDIRECTIONAL.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 1b60571446e1..3c3b0c44ea8a 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -1940,8 +1956,15 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
>
> /* Reclaim transmitted buffers */
> while (txbds_processed < txbds_ready) {
> - skb = bcmgenet_free_tx_cb(&priv->pdev->dev,
> - &priv->tx_cbs[ring->clean_ptr]);
> + tx_cb = &priv->tx_cbs[ring->clean_ptr];
> + if (tx_cb->xdpf) {
> + pkts_compl++;
> + bytes_compl += tx_cb->xdp_dma_map
> + ? tx_cb->xdpf->len
> + : tx_cb->xdpf->len -
> + sizeof(struct status_64);
> + }
> + skb = bcmgenet_free_tx_cb(&priv->pdev->dev, tx_cb);
> if (skb) {
> pkts_compl++;
> bytes_compl += GENET_CB(skb)->bytes_sent;
> @@ -1963,8 +1986,11 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
> u64_stats_add(&stats->bytes, bytes_compl);
> u64_stats_update_end(&stats->syncp);
>
> - netdev_tx_completed_queue(netdev_get_tx_queue(dev, ring->index),
> - pkts_compl, bytes_compl);
> + /* Ring DESC_INDEX (XDP TX) has no netdev TX queue; skip BQL */
> + if (ring->index != DESC_INDEX)
> + netdev_tx_completed_queue(netdev_get_tx_queue(dev,
> + ring->index),
> + pkts_compl, bytes_compl);
>
> return txbds_processed;
> }

The guard here skips netdev_get_tx_queue() for the XDP TX ring, but the
outer wrapper bcmgenet_tx_reclaim() still calls:

netdev_tx_reset_queue(netdev_get_tx_queue(dev, ring->index));
...
bcmgenet_tdma_ring_writel(priv, ring->index, ring->prod_index, ...);

unconditionally when invoked with all=true.

For the XDP TX ring, ring->index == DESC_INDEX == 16, but bcmgenet_probe()
allocates the netdev via alloc_etherdev_mqs(..., GENET_MAX_MQ_CNT + 1,
GENET_MAX_MQ_CNT + 1), which is only 5 TX queues. So netdev_get_tx_queue(dev,
16) returns &dev->_tx[16], well past the end of the allocated _tx[] array.

On CONFIG_BQL=y (the default), netdev_tx_reset_queue() then writes into that
memory through dql_reset() and clear_bit(__QUEUE_STATE_STACK_XOFF, &q->state).

Can this path corrupt memory past dev->_tx[] whenever bcmgenet_tx_reclaim_all()
runs on the XDP TX ring? See the new call added below:

[ ... ]

> @@ -2043,6 +2069,9 @@ static void bcmgenet_tx_reclaim_all(struct net_device *dev)
> do {
> bcmgenet_tx_reclaim(dev, &priv->tx_rings[i++], true);
> } while (i <= priv->hw_params->tx_queues && netif_is_multiqueue(dev));
> +
> + /* Also reclaim XDP TX ring */
> + bcmgenet_tx_reclaim(dev, &priv->xdp_tx_ring, true);
> }

bcmgenet_tx_reclaim_all() is reached from bcmgenet_netif_stop() (ifconfig
down), the WoL/suspend reconfigure path, and open/reset-failure cleanup, so
this is triggerable from userspace on every interface-down.

Should the netdev_tx_reset_queue() in bcmgenet_tx_reclaim()'s all-branch be
guarded with the same ring->index != DESC_INDEX check that was added to
__bcmgenet_tx_reclaim()?

[ ... ]

> @@ -3008,14 +3161,18 @@ static int bcmgenet_rdma_disable(struct bcmgenet_priv *priv)

[ ... ]

> static void bcmgenet_init_tx_queues(struct net_device *dev)
> {
> @@ -3028,7 +3185,8 @@ static void bcmgenet_init_tx_queues(struct net_device *dev)
>
> /* Initialize Tx priority queues */
> for (i = 0; i <= priv->hw_params->tx_queues; i++) {
> - bcmgenet_init_tx_ring(priv, i, end - start, start, end);
> + bcmgenet_init_tx_ring(priv, &priv->tx_rings[i],
> + i, end - start, start, end);
> start = end;
> end += priv->hw_params->tx_bds_per_q;
> dma_priority[DMA_PRIO_REG_INDEX(i)] |=
> @@ -3036,13 +3194,19 @@ static void bcmgenet_init_tx_queues(struct net_device *dev)
> << DMA_PRIO_REG_SHIFT(i);
> }
>
> + /* Initialize ring 16 (descriptor ring) for XDP TX */
> + bcmgenet_init_tx_ring(priv, &priv->xdp_tx_ring,
> + DESC_INDEX, GENET_Q16_TX_BD_CNT,
> + TOTAL_DESC - GENET_Q16_TX_BD_CNT, TOTAL_DESC);
> +
> /* Set Tx queue priorities */
> bcmgenet_tdma_writel(priv, dma_priority[0], DMA_PRIORITY_0);
> bcmgenet_tdma_writel(priv, dma_priority[1], DMA_PRIORITY_1);
> bcmgenet_tdma_writel(priv, dma_priority[2], DMA_PRIORITY_2);

What priority does ring 16 end up with under strict-priority arbitration?

dma_priority[] is declared as:

u32 dma_priority[3] = {0, 0, 0};

and only rings 0..tx_queues populate it. Ring 16's priority field lives in
DMA_PRIORITY_2, bits [20..24] (DMA_PRIO_REG_INDEX(16) == 2,
DMA_PRIO_REG_SHIFT(16) == 20), and is left as zero.

In this driver priority 0 is the highest:

#define GENET_Q1_PRIORITY 0 /* Default highest priority queue */
#define GENET_Q0_PRIORITY 1

A few lines above, arbitration is set to strict priority:

bcmgenet_tdma_writel(priv, DMA_ARBITER_SP, DMA_ARB_CTRL);

So ring 16 ends up at priority 0, outranking Q0 (priority 1) and equal to
the user-configured high-priority queues Q1..Q4. Should ring 16 be given
an explicit priority (for example the same as Q0, or lower) so XDP_TX does
not preempt normal SKB TX under strict-priority arbitration?