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

[Jakub Kicinski]

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?