Re: [PATCH net-next v6 3/7] net: macb: Add page pool support handle multi-descriptor frame rx

[Simon Horman <horms@kernel.org>]

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: macb: Add page pool support handle multi-descriptor frame rx

This commit adds page pool support for the Cadence MACB/GEM Ethernet
driver and enables multi-descriptor frame reception. It replaces
netdev_alloc_skb() with page pool allocation and removes the limitation
that frames must fit in a single descriptor.

> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
> index 5b2a461dfd28..ae500f717433 100644
> --- a/drivers/net/ethernet/cadence/Kconfig
> +++ b/drivers/net/ethernet/cadence/Kconfig

[ ... ]

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index fca7b9c6b670..84989ff0c3a9 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -2600,12 +2700,22 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	return ret;
>  }
>
> -static void macb_init_rx_buffer_size(struct macb *bp, size_t size)
> +static void macb_init_rx_buffer_size(struct macb *bp, unsigned int mtu)
>  {
> +	unsigned int overhead;
> +	size_t size;
> +
>  	if (!macb_is_gem(bp)) {
>  		bp->rx_buffer_size = MACB_RX_BUFFER_SIZE;
>  	} else {
> -		bp->rx_buffer_size = MIN(size, RX_BUFFER_MAX);
> +		size = mtu + ETH_HLEN + ETH_FCS_LEN;
> +		bp->rx_buffer_size = SKB_DATA_ALIGN(size + bp->rx_ip_align);
> +		if (gem_total_rx_buffer_size(bp) > PAGE_SIZE) {
> +			overhead = bp->rx_headroom +
> +				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +			bp->rx_buffer_size = rounddown(PAGE_SIZE - overhead,
> +						       RX_BUFFER_MULTIPLE);
> +		}
>
>  		if (bp->rx_buffer_size % RX_BUFFER_MULTIPLE) {
>  			netdev_dbg(bp->dev,

Can this overflow the hardware register field? The old code clamped size
against RX_BUFFER_MAX (16320), but the new code only guards against
PAGE_SIZE. Looking at the RXBS field in the hardware (GEM_RXBS_SIZE=8 bits
at macb.h:356), it can only represent values 0-255, encoding buffer sizes
0-16320 (value * RX_BUFFER_MULTIPLE where RX_BUFFER_MULTIPLE=64).

On systems with large page sizes (e.g., ARM64 with CONFIG_ARM64_64K_PAGES),
the PAGE_SIZE guard doesn't trigger for jumbo MTU values. If rx_buffer_size
reaches 16384 (SKB_DATA_ALIGN(16320+2)), then buffer_size = 16384/64 = 256,
which overflows the 8-bit field to 0 when written via GEM_BF(RXBS,
buffer_size) in macb_configure_dma():

drivers/net/ethernet/cadence/macb_main.c:macb_configure_dma() {
	buffer_size = bp->rx_buffer_size / RX_BUFFER_MULTIPLE;
	...
	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
		if (q)
			queue_writel(queue, RBQS, buffer_size);
		else
			dmacfg |= GEM_BF(RXBS, buffer_size);
	}
}

This would cause the hardware to use an incorrect (zero) buffer size.

Should the code clamp rx_buffer_size to ensure buffer_size stays within
0-255 after the division?