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

[netdev-bot+sashiko]

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 ENET_THLD_PAGE_LEN comment added by this patch says 'A
  descriptor is one page, which also holds skb_shared_info behind the…

--

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

This patch replaces the fixed 2048 byte RX buffer length with one
computed from the MTU through a new RBUF packet ready threshold, plus the
64 byte status block. The patch also programs RBUF_PKT_RDY_THLD and sets
TBUF_PKT_RDY_THLD to its maximum. A new priv->rx_buf_len replaces
RX_BUF_LENGTH in the refill, sync, length check and ring setup paths.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 5cb3d25482a0d..bf889558f6ade 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> -/* RX buffer plus the skb_shared_info napi_build_skb() places behind it */
> -#define GENET_RX_BUF_SIZE	SKB_HEAD_ALIGN(RX_BUF_LENGTH)
> +/* A descriptor is one page, which also holds skb_shared_info behind the frame,
> + * so on 4K pages the page bounds the threshold before the register does.
> + */

[Severity: Low]
This isn't a bug, but is "A descriptor is one page" accurate on every page
size?

bcmgenet_rx_refill() asks page_pool_dev_alloc() for
SKB_HEAD_ALIGN(priv->rx_buf_len) bytes. page_pool returns a whole page at
offset 0 only when twice the request is larger than the page. Otherwise it
returns a fragment.

rx_buf_len is 2112 with the default MTU, so on 4K pages each descriptor
gets a whole page. On 16K or 64K pages, several descriptors share one page.

This comment also seems to contradict the existing one in
bcmgenet_rx_ring_create_pool() in the same file:

	/* Buffers share a page. bcmgenet_rx_refill() syncs each one for the
	 * device, PP_FLAG_DMA_SYNC_DEV would sync the whole page.
	 */

That one is wrong for 4K pages, and the new one is wrong for larger pages.
Neither comment changes later in the series.

The ENET_THLD_PAGE_LEN bound itself looks fine, since a buffer never
exceeds one page. Could both comments be reworded so they agree and say
that buffers may share a page depending on PAGE_SIZE?

> +#define ENET_SHINFO_LEN		SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
> +#define ENET_THLD_PAGE_LEN	round_down(PAGE_SIZE - ENET_SHINFO_LEN - \
> +					   sizeof(struct status_64), \
> +					   ENET_THLD_BURST)
> +#define ENET_THLD_MAX_LEN	min_t(unsigned int, \
> +				      ENET_THLD_MAX * ENET_THLD_UNIT, \
> +				      ENET_THLD_PAGE_LEN)

[ ... ]

> @@ -2252,7 +2269,7 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
>  			      struct enet_cb *cb)
>  {
>  	struct bcmgenet_priv *priv = ring->priv;
> -	unsigned int size = GENET_RX_BUF_SIZE;
> +	unsigned int size = SKB_HEAD_ALIGN(priv->rx_buf_len);
>  	unsigned int offset;
>  	dma_addr_t mapping;
>  	struct page *page;

[ ... ]

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