Re: [PATCH net-next v3 1/6] net: bcmgenet: convert RX path to page_pool

Re: [PATCH net-next v3 1/6] net: bcmgenet: convert RX path to page_pool

[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: convert RX path to page_pool

Replace the per-packet __netdev_alloc_skb() + dma_map_single() in the
RX path with page_pool, which provides efficient page recycling and
DMA mapping management. This is a prerequisite for XDP support (which
requires stable page-backed buffers rather than SKB linear data).

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>

> diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
> index cd7dddeb91dd..e3b9a5272406 100644
> --- a/drivers/net/ethernet/broadcom/Kconfig
> +++ b/drivers/net/ethernet/broadcom/Kconfig

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 482a31e7b72b..7410034d9bdc 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2747,6 +2758,31 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
>  	netif_napi_add_tx(priv->dev, &ring->napi, bcmgenet_tx_poll);
>  }
>
> +static int bcmgenet_rx_ring_create_pool(struct bcmgenet_priv *priv,
> +					struct bcmgenet_rx_ring *ring)
> +{
> +	struct page_pool_params pp_params = {
> +		.order = 0,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
> +		.pool_size = ring->size,
> +		.nid = NUMA_NO_NODE,
> +		.dev = &priv->pdev->dev,
> +		.dma_dir = DMA_FROM_DEVICE,
> +		.offset = GENET_XDP_HEADROOM,
> +		.max_len = RX_BUF_LENGTH,
> +	};
> +
> +	ring->page_pool = page_pool_create(&pp_params);
> +	if (IS_ERR(ring->page_pool)) {
> +		int err = PTR_ERR(ring->page_pool);
> +
> +		ring->page_pool = NULL;
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  /* Initialize a RDMA ring */
>  static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>  				 unsigned int index, unsigned int size,
> @@ -2765,10 +2801,17 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>  	ring->cb_ptr = start_ptr;
>  	ring->end_ptr = end_ptr - 1;
>
> -	ret = bcmgenet_alloc_rx_buffers(priv, ring);
> +	ret = bcmgenet_rx_ring_create_pool(priv, ring);
>  	if (ret)
>  		return ret;
>
> +	ret = bcmgenet_alloc_rx_buffers(priv, ring);
> +	if (ret) {
> +		page_pool_destroy(ring->page_pool);
> +		ring->page_pool = NULL;
> +		return ret;
> +	}
> +

Does this leak pages if bcmgenet_alloc_rx_buffers() fails partway through?

If bcmgenet_alloc_rx_buffers() successfully allocates pages for the first
k iterations but then page_pool_alloc_pages() fails on iteration k+1, the
pages stored in cb->rx_page for iterations 0 through k-1 are never
returned to the page pool.

When the error path calls page_pool_destroy(), the pool detects outstanding
pages and defers cleanup, triggering periodic 'stalled pool shutdown' kernel
warnings from page_pool_release_retry().

Then bcmgenet_init_dma's error path calls bcmgenet_free_rx_buffers(), which
skips this ring entirely because ring->page_pool is NULL:

bcmgenet_free_rx_buffers() {
    for (q = 0; q <= priv->hw_params->rx_queues; q++) {
        ring = &priv->rx_rings[q];
        if (!ring->page_pool)
            continue;
        ...
    }
}

Should the error path iterate through the ring's cbs and call
bcmgenet_free_rx_cb() for each one before destroying the page pool?

The old code didn't have this issue because bcmgenet_free_rx_buffers()
iterated all rx_cbs globally without checking for a per-ring page_pool.

[ ... ]