Re: [net-next,v2,1/8] net: macb: move Rx buffers alloc from link up to open

[Jakub Kicinski <kuba@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: move Rx buffers alloc from link up to open

This commit moves Rx buffer allocation to macb_open() to enable early
error propagation, adding a fail_early parameter to control whether ring
initialization fails fast or continues on error.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 87414a2ddf6e..2cb65ec37d44 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1180,7 +1180,7 @@ struct macb_queue;
>  struct macb_or_gem_ops {
>  	int	(*mog_alloc_rx_buffers)(struct macb *bp);
>  	void	(*mog_free_rx_buffers)(struct macb *bp);
> -	void	(*mog_init_rings)(struct macb *bp);
> +	int	(*mog_init_rings)(struct macb *bp, bool fail_early);
>  	int	(*mog_rx)(struct macb_queue *queue, struct napi_struct *napi,
>  			  int budget);
>  };

[ ... ]

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 43cd013bb70e..8cc16f2b4024 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -2544,8 +2547,6 @@ static int macb_alloc_consistent(struct macb *bp)
>  		if (!queue->tx_skb)
>  			goto out_err;
>  	}
> -	if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
> -		goto out_err;
>
>  	/* Required for tie off descriptor for PM cases */
>  	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE)) {
> @@ -2557,6 +2558,11 @@ static int macb_alloc_consistent(struct macb *bp)
>  			goto out_err;
>  	}
>
> +	if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
> +		goto out_err;
> +	if (bp->macbgem_ops.mog_init_rings(bp, true))
> +		goto out_err;
> +
>  	return 0;
>
>  out_err:

[ ... ]

> @@ -2951,7 +2971,7 @@ static int macb_open(struct net_device *dev)
>  		goto pm_exit;
>  	}
>
> -	bp->macbgem_ops.mog_init_rings(bp);
> +	bp->macbgem_ops.mog_init_rings(bp, true);
>  	macb_init_buffers(bp);
>
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {

Is mog_init_rings() being called twice in the macb_open() path? Looking at
the code flow:

macb_open()
  macb_alloc_consistent() at line 2967
    mog_init_rings(bp, true) at line 2563
  mog_init_rings(bp, true) at line 2974

The first call inside macb_alloc_consistent() initializes the rings and
checks the return value. The second call in macb_open() appears to be
redundant: by the time macb_open() reaches line 2974, gem_init_rings() has
already refilled all Rx slots via the earlier call.

When gem_init_rings() runs the second time, CIRC_SPACE returns 0 because
all slots are filled, so gem_rx_refill() effectively does nothing. The
second call reinitializes head/tail pointers that are already set and runs
a no-op refill.

Should this second call be removed to avoid the redundancy?

Additionally, the return value from the second mog_init_rings(bp, true)
call is silently discarded. The stated purpose of this patch is to "fail
early and loudly rather than init the device with Rx mostly broken," but
this call site doesn't check for errors while the call in
macb_alloc_consistent() does.

If the second call is intentional, should its return value be checked?

This was noted in the v2 review by Théo Lebrun (Jan 8, 2026) who pointed
out that commit 99537d5c476c relocated mog_init_rings() from
macb_mac_link_up() to macb_open(), making this second call redundant. Paolo
acknowledged this with "will do" but the duplicate call remains.
-- 
pw-bot: cr