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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: bcmgenet: add XDP_TX support

Implement XDP_TX using ring 16 (DESC_INDEX), the hardware default
descriptor ring, dedicated to XDP TX for isolation from SKB TX queues.
Ring 16 gets 32 BDs carved from ring 0's allocation. TX completion is
piggybacked on RX NAPI poll since ring 16's INTRL2_1 bit collides with
RX ring 0, similar to how bnxt, ice, and other XDP drivers handle TX
completion within the RX poll path.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index e8f90d6b16a2..54df1694f1cd 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -48,8 +48,10 @@
>
>  #define GENET_Q0_RX_BD_CNT	\
>  	(TOTAL_DESC - priv->hw_params->rx_queues * priv->hw_params->rx_bds_per_q)
> +#define GENET_Q16_TX_BD_CNT	64
> +#define GENET_Q0_TX_BD_CNT	\
> -	(TOTAL_DESC - priv->hw_params->tx_queues * priv->hw_params->tx_bds_per_q)
> +	(TOTAL_DESC - priv->hw_params->tx_queues * priv->hw_params->tx_bds_per_q \
> +	 - GENET_Q16_TX_BD_CNT)
>
>  #define RX_BUF_LENGTH		2048
>  #define SKB_ALIGNMENT		32

[ ... ]

> @@ -3022,14 +3158,18 @@ static int bcmgenet_rdma_disable(struct bcmgenet_priv *priv)
>   * with queue 1 being the highest priority queue.
>   *
>   * Queue 0 is the default Tx queue with
> - * GENET_Q0_TX_BD_CNT = 256 - 4 * 32 = 128 descriptors.
> + * GENET_Q0_TX_BD_CNT = 256 - 4 * 32 - 32 = 96 descriptors.
> + *
> + * Ring 16 (DESC_INDEX) is used for XDP TX with
> + * GENET_Q16_TX_BD_CNT = 32 descriptors.
> + *
>   * The transmit control block pool is then partitioned as follows:
> - * - Tx queue 0 uses tx_cbs[0..127]
> - * - Tx queue 1 uses tx_cbs[128..159]
> - * - Tx queue 2 uses tx_cbs[160..191]
> - * - Tx queue 3 uses tx_cbs[192..223]
> - * - Tx queue 4 uses tx_cbs[224..255]
> + * - Tx queue 0 uses tx_cbs[0..95]
> + * - Tx queue 1 uses tx_cbs[96..127]
> + * - Tx queue 2 uses tx_cbs[128..159]
> + * - Tx queue 3 uses tx_cbs[160..191]
> + * - Tx queue 4 uses tx_cbs[192..223]
> + * - Tx ring 16 uses tx_cbs[224..255]
>   */
>  static void bcmgenet_init_tx_queues(struct net_device *dev)
>  {

Does the GENET_Q16_TX_BD_CNT define match the comment?

The comment and the commit message both say "Ring 16 gets 32 BDs," but
the define at the top of the file says:

#define GENET_Q16_TX_BD_CNT	64

The comment math also says "256 - 4 * 32 - 32 = 96" which expects ring
16 to use 32 descriptors, and shows queue 0 getting 96 descriptors
(tx_cbs[0..95]).  But with the define set to 64, queue 0 would actually
get only 64 descriptors (tx_cbs[0..63]) instead of 96:

GENET_Q0_TX_BD_CNT = TOTAL_DESC (256)
                     - priv->hw_params->tx_queues (4) * 32
                     - GENET_Q16_TX_BD_CNT (64)
                   = 256 - 128 - 64
                   = 64

and ring 16 would use tx_cbs[192..255] (64 entries) instead of
tx_cbs[224..255] (32 entries).

All of the intermediate queue ranges in the comment would also be
shifted by 32 entries from what's documented.

Which is correct, the define or the documentation?