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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello netdev-bot+sashiko,

On Tue Sep 22, 2026 at 10:37 PM CEST,  wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 (1) · Medium: 0 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] Incomplete fix: the new errno from
>   gem_rx_refill()/gem_init_rx_ring()/mog_init_rings() is discarded by…
> - [High] macb_set_ringparam()
>   (drivers/net/ethernet/cadence/macb_main.c:3737-3749) closes and…
>
> Pre-existing issues:
> - [High] macb_hresp_error_task() (macb_main.c:1998-2035, BH-workqueue
>   context) calls bp->macbgem_ops.mog_init_rings(bp) -> gem_init_rings()…
>
> --
>
> net: macb: propagate RX ring refill errors
>
> This changes gem_rx_refill(), gem_init_rx_ring(), gem_init_rings(),
> macb_init_rings() and the macbgem_ops.mog_init_rings function pointer to
> return errno-style ints, so that macb_open() fails when RX skb allocation
> or DMA mapping fails. The other refill callers (resume, HRESP error task,
> NAPI) keep their best-effort behaviour.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index d6931c41f39d..cfaa0ca49f1a 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -1197,7 +1197,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);
>>  	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 751fa9e68099..c418f859cc34 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1486,13 +1486,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>>  	return packets;
>>  }
>>  
>> -static void gem_rx_refill(struct macb_queue *queue)
>> +static int gem_rx_refill(struct macb_queue *queue)
>>  {
>>  	struct macb *bp = queue->bp;
>>  	struct macb_dma_desc *desc;
>>  	struct sk_buff *skb;
>>  	unsigned int entry;
>>  	dma_addr_t paddr;
>> +	int err = 0;
>>  
>>  	while (CIRC_SPACE(queue->rx_prepared_head, queue->rx_tail,
>>  			bp->rx_ring_size) > 0) {
>> @@ -1509,6 +1510,7 @@ static void gem_rx_refill(struct macb_queue *queue)
>>  			if (unlikely(!skb)) {
>>  				netdev_err(bp->netdev,
>>  					   "Unable to allocate sk_buff\n");
>> +				err = -ENOMEM;
>>  				break;
>>  			}
>>  
>> @@ -1518,6 +1520,7 @@ static void gem_rx_refill(struct macb_queue *queue)
>>  					       DMA_FROM_DEVICE);
>>  			if (dma_mapping_error(&bp->pdev->dev, paddr)) {
>>  				dev_kfree_skb(skb);
>> +				err = -ENOMEM;
>>  				break;
>>  			}
>>  
>> @@ -1558,6 +1561,8 @@ static void gem_rx_refill(struct macb_queue *queue)
>>  
>>  	netdev_vdbg(bp->netdev, "rx ring: queue: %p, prepared head %d, tail %d\n",
>>  		    queue, queue->rx_prepared_head, queue->rx_tail);
>> +
>> +	return err;
>>  }
>
> [Severity: High]
> The commit message says:
>
>     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.
>
> Is the "hoping it will improve" part true for those callers, given the
> same analysis used to justify the open() fix?
>
> gem_init_rx_ring() first marks every descriptor as software owned and
> resets both cursors:
>
> 	queue->rx_tail = 0;
> 	queue->rx_prepared_head = 0;
>
> 	for (i = 0; i < queue->bp->rx_ring_size; i++)
> 		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
>
> 	return gem_rx_refill(queue);
>
> gem_rx_refill() then breaks out on the first netdev_alloc_skb() or
> dma_mapping_error() failure, so it can return with zero descriptors handed
> to the hardware.

Discussed with Nicolai [0]. The idea I'm proposing is that it errors out
if zero slots exist. That way we don't reach that forever-dead state
that only can occur if we hand zero descriptors to the hardware.

[0]: https://lore.kernel.org/netdev/0eeb123ad32558ac42762827eb509110@tipi-net.de/

[...]

>>  
>>  /* Mark DMA descriptors from begin up to and not including end as unused */
>
> [ ... ]
>
>> @@ -2793,14 +2798,16 @@ static int macb_alloc(struct macb *bp)
>>  	for (i = 0; i < queue->bp->rx_ring_size; i++)
>>  		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
>>  
>> -	gem_rx_refill(queue);
>> +	return gem_rx_refill(queue);
>>  }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but it sits on the call chain
> being changed here, so it may be worth a look: is the HRESP path allowed to
> re-run gem_init_rx_ring()/gem_rx_refill() while an RX poll is in flight?
>
> macb_hresp_error_task() runs in BH workqueue context and only masks the
> device interrupt sources:

Yes. We introduce in [PATCH net 3/3] a shutdown sequence that the HRESP
task should reuse to make it safe to run at any point in time.

We will not use it yet, this will come later. Let's first fix the big
hole that everyone takes which is close. Once the infra is in place
we'll reuse it for HRESP task.

[...]

>
>> @@ -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;
>
> [Severity: High]
> This adds a new, fairly easy to hit macb_open() failure path that returns
> before the napi_enable() loop.  Does macb_set_ringparam() need updating for
> it?  It closes and reopens the interface and discards the result:

Again, infra is put in place. Be patient. It will be reused to implement
context swapping. One step at a time.

[...]

Thanks,

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