Re: [PATCH net-next v6 1/7] 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.
---
net: macb: move Rx buffers alloc from link up to open

This commit moves the Rx buffer allocation in the macb driver from link up
to open. It allows the initialization to fail early and loudly if the
Rx rings cannot be refilled, rather than initializing the device in a
broken state.

[ ... ]

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 1cb49252abf5..38302cba05ad 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1504,6 +1505,7 @@ static void gem_rx_refill(struct macb_queue *queue)
>  			if (unlikely(!skb)) {
>  				netdev_err(bp->dev,
>  					   "Unable to allocate sk_buff\n");
> +				err = -ENOMEM;
>  				break;
>  			}

If dma_map_single() fails shortly after this block, the code frees the
allocated skb and breaks out of the loop:

	paddr = dma_map_single(&bp->pdev->dev, skb->data,
			       bp->rx_buffer_size,
			       DMA_FROM_DEVICE);
	if (dma_mapping_error(&bp->pdev->dev, paddr)) {
		dev_kfree_skb(skb);
		break;
	}

Does this cause the function to return 0 (success) despite the DMA mapping
failure, bypassing the new fail-early design?

[ ... ]

> @@ -6108,12 +6127,20 @@ static int __maybe_unused macb_resume(struct device *dev)
>  	for (q = 0, queue = bp->queues; q < bp->num_queues;
>  	     ++q, ++queue) {
>  		if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
> -			if (macb_is_gem(bp))
> -				gem_init_rx_ring(queue);
> -			else
> +			if (macb_is_gem(bp)) {
> +				err = gem_init_rx_ring(queue);
> +				if (err) {
> +					dev_err(dev, "Unable to init RX ring: %d\n", err);
> +					return err;
> +				}
> +			} else {
>  				macb_init_rx_ring(queue);
> +			}
>  		}
> +	}
>  
> +	for (q = 0, queue = bp->queues; q < bp->num_queues;
> +	     ++q, ++queue) {
>  		napi_enable(&queue->napi_rx);
>  		napi_enable(&queue->napi_tx);
>  	}

If gem_init_rx_ring() fails and we return err early here, the subsequent
loop that calls napi_enable() is skipped.

Since NAPI was disabled during suspend, the NAPI context remains in a
disabled state. If the interface is later administratively brought down,
.ndo_stop (macb_close) will unconditionally call napi_disable().

Can calling napi_disable() on an already disabled NAPI context cause an
infinite loop and a hard kernel deadlock in this scenario?
-- 
pw-bot: cr