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

[netdev-bot+sashiko@kernel.org]

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.

With no hardware-owned descriptor, can the controller ever complete a
receive and raise RCOMP?  bp->rx_intr_mask is only MACB_RX_INT_FLAGS
(RCOMP | ISR_ROVR) unless the part has MACB_CAPS_NEEDS_RSTONUBR:

	bp->rx_intr_mask = MACB_RX_INT_FLAGS;
	if (bp->caps & MACB_CAPS_NEEDS_RSTONUBR)
		bp->rx_intr_mask |= MACB_BIT(RXUBR);

and even on those parts macb_interrupt_misc() only toggles NCR.RE without
refilling.  The single remaining refill trigger, gem_rx(), is only reached
through napi_schedule on RCOMP/ROVR.

The callers that discard the new return value re-arm the receiver anyway:

macb_resume():

	if (macb_is_gem(bp))
		gem_init_rx_ring(queue);
	...
	macb_init_hw(bp);

macb_hresp_error_task():

	bp->macbgem_ops.mog_init_rings(bp);
	...
	ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
	macb_writel(bp, NCR, ctrl);

gem_rx():

	gem_rx_refill(queue);

	return count;

So can a queue still end up permanently RX-dead after resume, after HRESP
recovery, or at runtime when a poll drains every armed descriptor and the
following refill fails on its first allocation, with recovery only through
an administrative down/up?  Would it make sense for those paths to retry
(NAPI reschedule or delayed work), or to keep the receiver disabled for
the affected queue, rather than only propagating the error at open()?

>  
>  /* 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:

	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
		queue_writel(queue, IDR, bp->rx_intr_mask |
					 MACB_TX_INT_FLAGS |
					 MACB_BIT(HRESP));
	}
	...
	bp->macbgem_ops.mog_init_rings(bp);

There is no napi_disable() or napi_synchronize(), and neither side takes
bp->lock around queue->rx_tail, queue->rx_prepared_head, queue->rx_skbuff[]
or the descriptors.  Can this sequence happen?

CPU0, gem_rx() poll at entry E (hardware has completed that descriptor)

CPU1, HRESP worker, gem_rx_refill() sees rx_skbuff[E] still non-NULL:
	} else {
		desc->ctrl = 0;
		dma_wmb();
		desc->addr &= ~MACB_BIT(RX_USED);
	}

CPU0 then continues in gem_rx():
	queue->rx_skbuff[entry] = NULL;
	...
	dma_unmap_single(&bp->pdev->dev, addr,
			 bp->rx_buffer_size, DMA_FROM_DEVICE);

and passes the skb up.  Once macb_hresp_error_task() rewrites NCR with
RE|TE, does the DMA engine then write the next frame into a buffer that is
already unmapped and owned by the stack?  The unsynchronised resets of
rx_tail and rx_prepared_head to 0 against gem_rx()/gem_rx_refill()'s own
updates of the same counters also look like they could arm a descriptor
twice or leak an skb.

[ ... ]

> @@ -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:

	if (netif_running(bp->netdev)) {
		reset = 1;
		macb_close(bp->netdev);
	}

	bp->rx_ring_size = new_rx_size;
	bp->tx_ring_size = new_tx_size;

	if (reset)
		macb_open(bp->netdev);

	return 0;

Because the ops are called directly rather than through __dev_open(), the
netdev stays marked running while the rings are freed and both NAPI
instances are left disabled.  The next close then calls napi_disable() on
instances that were never re-enabled, and napi_disable_locked() spins:

net/core/dev.c:napi_disable_locked() {
	...
	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}
	...
}

Nothing clears those bits without an intervening napi_enable(), so can an
"ethtool -G" resize that fails allocation under memory pressure leave the
next "ip link set dev down" looping forever with rtnl_lock held?

>  	macb_init_buffers(bp);
>  
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> @@ -3200,6 +3215,7 @@ static int macb_open(struct net_device *netdev)
>  		napi_disable(&queue->napi_rx);
>  		napi_disable(&queue->napi_tx);
>  	}
> +free_rings:
>  	macb_free(bp);
>  pm_exit:
>  	pm_runtime_put_sync(&bp->pdev->dev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-macb-close-v1-0-05e32ce98813%40bootlin.com