Re: [PATCH net-next v7 5/9] 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] In bcmgenet_init_tx_ring()
  (drivers/net/ethernet/broadcom/genet/bcmgenet.c), the buffer-size…

--

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

This patch replaces the fixed 2048 byte receive buffer length with a value
taken from the MTU. The RBUF packet ready threshold is computed from the
MTU and programmed into RBUF, and the buffer length is that threshold
plus the status block. UMAC_MAX_FRAME_LEN and the TX flow period are
also derived from the MTU, with a floor at ETH_DATA_LEN.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index b0b0d9b423482..1bb5fed4a8b4d 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> +/* 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;
> +}

[ ... ]

> @@ -2792,7 +2845,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 TX ring intended?

In bcmgenet_init_tx_ring(), the buffer size field of the TDMA
DMA_RING_BUF_SIZE register now gets priv->rx_buf_len. That value comes
from bcmgenet_rx_buf_len(), which only uses RX quantities: the RX status
block plus the RBUF packet ready threshold.

At the default MTU this is 64 + 0x80 * 16 = 2112, so the TDMA register
value goes from 0x800 to 0x840 on every interface init.

The commit message only talks about the receive side:

  "The MTU is still fixed at ETH_DATA_LEN, so the threshold comes out at
  the reset default and the buffer only grows by the status block the
  hardware already wrote."

It doesn't mention that a TX DMA register changes as well.

The TX ring already used the RX constant RX_BUF_LENGTH before this patch.
What is new here is the changed value, and the fact that the TX
programming now follows the MTU through the RX threshold and its
page-size clamp.

This line is unchanged in the later patches of the series. So once the MTU
can grow, the TDMA field keeps following bcmgenet_rx_buf_len(mtu) and not
any value picked for TX.

No functional effect has been shown. The per-descriptor TX length is in
length_status. The last patch in the series, "net: bcmgenet: reassemble
jumbo frames from status block fragments", reports 16361 byte frames
sent byte-exact over UMAC loopback while this field stays at rx_buf_len
(about 3.9K at most). That suggests the field does not limit TX.

Would it be clearer to keep a TX-specific value here? If the change is
intentional, could the commit message say so?

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