Re: [PATCH net-next v2 4/7] net: bcmgenet: derive the receive buffer length from the MTU

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch also changes what bcmgenet_init_tx_ring() writes into
  each transmit ring's DMA_RING_BUF_SIZE register, and the commit…

--

net: bcmgenet: derive the receive buffer length from the MTU

This patch replaces the fixed 2048 byte RX_BUF_LENGTH with
priv->rx_buf_len. That value comes from an RBUF packet ready threshold
computed from the MTU, plus the 64 byte status block. It also programs
the RBUF and TBUF packet ready threshold registers. The TBUF threshold
is set to its maximum.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 641d918577d4d..75d1006a35c55 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2623,6 +2641,44 @@ static void bcmgenet_link_intr_enable(struct bcmgenet_priv *priv)

[ ... ]

> +/* A buffer has to hold everything the threshold lets the hardware deliver */
> +static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
> +{
> +	return sizeof(struct status_64) +
> +	       bcmgenet_pkt_rdy_thld(mtu) * ENET_THLD_UNIT;
> +}

[ ... ]

> @@ -2755,7 +2811,7 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
>  				  TDMA_FLOW_PERIOD);
>  	bcmgenet_tdma_ring_writel(priv, index,
>  				  ((size << DMA_RING_SIZE_SHIFT) |
> -				   RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
> +				   priv->rx_buf_len), DMA_RING_BUF_SIZE);

[Severity: Low]
Is this change to the transmit ring intended? The commit message only
talks about the receive buffer and the RBUF/TBUF thresholds. It doesn't
mention that bcmgenet_init_tx_ring() now writes a different value into
each TDMA ring's DMA_RING_BUF_SIZE register.

Before the patch, the TX rings got the constant 2048. With this patch
they get priv->rx_buf_len, which bcmgenet_rx_buf_len() computes as:

    sizeof(struct status_64) + bcmgenet_pkt_rdy_thld(mtu) * 16

That is 64 + 2048 = 2112 at the default MTU. The new value includes the
64 byte RX status block, which only the receive side writes.

This sentence in the commit message reads as if it describes the RX
page pool buffer only:

    "the buffer only grows by the status block the hardware already
    wrote."

The TX ring already took its value from an RX-named constant, so this
coupling is not new. What is new is that it now follows the MTU.

With the later "net: bcmgenet: allow the MTU to be changed" patch in
this series, every RX buffer resize would also reprogram the TX rings.
That reaches 3648 on 4K pages and 3904 on 64K pages.

Each TX descriptor carries its own length in len_stat, so this may have
no functional effect. Should the TX ring keep its own value here? If
the change is intended, could the commit message say so?

>  
>  	/* Set start and end address, read and write pointers */
>  	bcmgenet_tdma_ring_writel(priv, index, start_ptr * words_per_bd,

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 501dd12566936..6444bac168c38 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h

[ ... ]

> @@ -612,6 +615,7 @@ struct bcmgenet_priv {
>  	void __iomem *rx_bds;
>  	struct enet_cb *rx_cbs;
>  	unsigned int num_rx_bds;
> +	unsigned int rx_buf_len;

This field is declared with the receive variables, but
bcmgenet_init_tx_ring() above also uses it to configure the transmit
rings.

>  	struct bcmgenet_rxnfc_rule rxnfc_rules[MAX_NUM_OF_FS_RULES];
>  	struct list_head rxnfc_list;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-nb-genet-mtu-nn-v2-v2-0-c4be21151912%40tipi-net.de