Re: [PATCH 2/3] net: macb: propagate RX ring refill errors

[Théo Lebrun]

Hello Nicolai,

On Mon Sep 21, 2026 at 9:05 AM CEST, Nicolai Buchwitz wrote:
> On 18.9.2026 22:32, Théo Lebrun wrote:
>> gem_rx_refill() is responsible for Rx SKB allocation, including at 
>> open,
>> but its prototype indicates a void return value.
>> 
>> Therefore we change the code to propagate allocation and DMA mapping
>> errors back up the stack, making sure the open fails if it occurs.
>> Change all those to return errno-style ints:
>>  - gem_rx_refill()
>>  - its parent gem_init_rx_ring()
>>  - its grand-parent gem_init_rings()
>>  - the macbgem_ops.mog_init_rings function pointer
>>  - its grand-uncle macb_init_rings()
>> 
>> Theoretical bugfix, never encountered in practice. To reproduce,
>> introduce memory pressure (less than 512 SKBs of free memory) and open
>> the interface. I expect the last queue to be unuseable because it has
>> zero usable rx buffers. Nothing will ever trigger a refill on that
>> queue which only happens once a frame has been received.
>> 
>> Note that other callers of refill (resume, HRESP error task, NAPI)
>> cannot do anything useful with that error and keep their best-effort
>> refill, hoping it will improve.
>> 
>> Fixes: 4df95131ea80 ("net/macb: change RX path for GEM")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>> ---
>
>> [...]
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 751fa9e68099..c418f859cc34 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
>> [...]
>
>>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> @@ -2813,11 +2820,15 @@ static void gem_init_rings(struct macb *bp)
>>  		queue->tx_head = 0;
>>  		queue->tx_tail = 0;
>> 
>> -		gem_init_rx_ring(queue);
>> +		err = gem_init_rx_ring(queue);
>> +		if (err)
>> +			last_err = err;
>
> A partly filled queue refills itself on the next gem_rx().  AFAIU only a
> queue with zero buffers can get stuck. Fail the open just for that case?

Ah so gem_init_rx_ring()/gem_rx_refill() would check the total allocated
slots count (not just count of slots allocated on this call). Then we
fail only if that is zero. I agree it sounds better.

>
>> [...]
>
>>  static void macb_reset_hw(struct macb *bp)
>> @@ -3162,7 +3175,9 @@ static int macb_open(struct net_device *netdev)
>>  		goto pm_exit;
>>  	}
>> 
>> -	bp->macbgem_ops.mog_init_rings(bp);
>> +	err = bp->macbgem_ops.mog_init_rings(bp);
>> +	if (err)
>> +		goto free_rings;
>
> macb_set_ringparam() ignores macb_open()'s error, so NAPI stays disabled
> with netif_running() == true and the next close will hang in 
> napi_disable().
>
> Should macb_set_ringparam() return the error?

Yes. I ignored that codepath because I know context
swapping will land soon. :-)

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com