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

[Paolo Valerio <pvalerio@redhat.com>]

On 23 Feb 2026 at 04:08:32 PM, Jakub Kicinski <kuba@kernel.org> wrote:

> 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.

yes, this definitely is not intentional. Must have been left/reintroduced
by mistake while rebasing v2, sorry.

> -- 
> pw-bot: cr