Re: [PATCH net 3/3] net: macb: quiesce IRQs and drain BH on interface close

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 (1) · 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).

- [Medium] macb_close() now calls macb_quiesce_start() (synchronize_irq()
  on every queue IRQ, cancel_work_sync(&bp->hresp_err_bh_work),…

Pre-existing issues:
- [High] macb_suspend()/macb_resume() still open-code the per-queue
  napi_disable()/napi_enable() loops without setting/clearing…
- [High] macb_set_ringparam() calls macb_close() and then macb_open()
  while discarding macb_open()'s return value and always returning 0.
- [Medium] bp->hresp_err_bh_work and bp->tx_lpi_work are initialized by
  INIT_WORK()/INIT_DELAYED_WORK() only *after* register_netdev() in…

--

net: macb: quiesce IRQs and drain BH on interface close

This adds macb_quiesce_start()/macb_quiesce_end() helpers plus a new
bp->irq_quiesced flag so that macb_interrupt() self-disarms while the
interface is down, and reorders macb_close() to drain IRQs, the two work
items, NAPI and the LPI delayed work in a defined order. macb_open() uses
the same helpers on its success and error paths.

A few questions below.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index cfaa0ca49f1a..1cb2778fe49e 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1382,6 +1382,11 @@ struct macb {
>  	struct delayed_work	tx_lpi_work;
>  	u32			tx_lpi_timer;
>  
> +	/* ISR must not drive NAPI & BH mechanisms. True when the interface
> +	 * is closed. Protected by bp->lock.
> +	 */
> +	bool			irq_quiesced;
> +
>  	int	rx_bd_rd_prefetch;
>  	int	tx_bd_rd_prefetch;
>  
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c418f859cc34..75bbde81d62c 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1995,6 +1995,53 @@ static int macb_tx_poll(struct napi_struct *napi, int budget)
>  	return work_done;
>  }
>  
> +static void macb_quiesce_start(struct macb *bp)
> +{
> +	struct macb_queue *queue;
> +	unsigned long flags;
> +	unsigned int q;
> +
> +	spin_lock_irqsave(&bp->lock, flags);
> +	bp->irq_quiesced = true;
> +	spin_unlock_irqrestore(&bp->lock, flags);
> +
> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +		synchronize_irq(queue->irq);
> +
> +	cancel_work_sync(&bp->hresp_err_bh_work);

[Severity: Medium]
This isn't a bug introduced by this patch, but can this cancel_work_sync()
run on a work_struct that has not been initialized yet?

In macb_probe(), register_netdev() happens before the two work items are
set up:

	err = register_netdev(netdev);
	if (err) {
		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
		goto err_out_free_tieoff;
	}

	INIT_WORK(&bp->hresp_err_bh_work, macb_hresp_error_task);
	INIT_DELAYED_WORK(&bp->tx_lpi_work, macb_tx_lpi_work_fn);

The queue IRQ handlers were already installed earlier by
macb_init_dflt()'s devm_request_irq(..., macb_interrupt, ...), and
register_netdev() publishes the device, so an 'ip link set up' racing with
the tail of probe can reach macb_open() -> macb_quiesce_end(), which clears
bp->irq_quiesced. From there an HRESP interrupt would let
macb_interrupt_misc() do queue_work(system_bh_wq, &bp->hresp_err_bh_work)
on a zeroed work_struct whose ->func is NULL, and macb_close() or the
macb_open() error path would call cancel_work_sync() /
cancel_delayed_work_sync() on the same uninitialized objects.

The pre-existing macb_close() already had the cancel_delayed_work_sync() on
bp->tx_lpi_work, so this only widens an existing window. Would moving both
INIT_WORK()/INIT_DELAYED_WORK() calls ahead of macb_init()/register_netdev()
be reasonable?

> +
> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> +		/* Must be done before NAPI is disabled: the task ends with a
> +		 * napi_enable() call.
> +		 */
> +		cancel_work_sync(&queue->tx_error_task);
> +
> +		napi_disable(&queue->napi_rx);
> +		napi_disable(&queue->napi_tx);
> +	}
> +
> +	/* Must be done after napi_tx is disabled: its completion re-arms
> +	 * the LPI timer.
> +	 */
> +	cancel_delayed_work_sync(&bp->tx_lpi_work);
> +}
> +
> +static void macb_quiesce_end(struct macb *bp)
> +{
> +	struct macb_queue *queue;
> +	unsigned long flags;
> +	unsigned int q;
> +
> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> +		napi_enable(&queue->napi_rx);
> +		napi_enable(&queue->napi_tx);
> +	}
> +
> +	spin_lock_irqsave(&bp->lock, flags);
> +	bp->irq_quiesced = false;
> +	spin_unlock_irqrestore(&bp->lock, flags);
> +}
> +
>  static void macb_hresp_error_task(struct work_struct *work)
>  {
>  	struct macb *bp = from_work(bp, work, hresp_err_bh_work);
> @@ -2137,8 +2184,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>  	spin_lock(&bp->lock);
>  
>  	while (status) {
> -		/* close possible race with dev_close */
> -		if (unlikely(!netif_running(netdev))) {
> +		/* self-disarm while the netdev is closed */
> +		if (unlikely(bp->irq_quiesced)) {
>  			queue_writel(queue, IDR, -1);
>  			macb_queue_isr_clear(bp, queue, -1);
>  			break;

[Severity: High]
This is a pre-existing issue, but does the new gating condition need a
companion change in macb_suspend()/macb_resume()?

macb_suspend() still open-codes the NAPI loop, with no bp->irq_quiesced
update, no synchronize_irq() and no draining of queue->tx_error_task or
bp->hresp_err_bh_work:

	netif_device_detach(netdev);
	for (q = 0, queue = bp->queues; q < bp->num_queues;
	     ++q, ++queue) {
		napi_disable(&queue->napi_rx);
		napi_disable(&queue->napi_tx);
	}

	if (!(bp->wol & MACB_WOL_ENABLED)) {
		rtnl_lock();
		phylink_stop(bp->phylink);
		rtnl_unlock();
		...
		macb_reset_hw(bp);

In the non-WOL path the interrupt enables left by macb_mac_link_up()
(bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP)) are still unmasked
until that later macb_reset_hw(), and netif_device_detach() does not affect
netif_running(), so the ISR stays armed with bp->irq_quiesced == false.

A TX error interrupt in that window reaches macb_interrupt_misc():

	if (unlikely(status & (MACB_TX_ERR_FLAGS))) {
		queue_writel(queue, IDR, MACB_TX_INT_FLAGS);
		schedule_work(&queue->tx_error_task);

and macb_tx_error_task() begins with napi_disable(&queue->napi_tx) on a NAPI
that macb_suspend() already disabled. napi_disable_locked() then never
returns:

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

The worker wedges holding the netdev instance lock, so macb_resume()'s
napi_enable() blocks as well. The same window lets macb_hresp_error_task()
re-init every ring and rewrite IER/NCR/carrier behind suspend's back.

The new comment says "ISR must not drive NAPI & BH mechanisms. True when
the interface is closed.", and the commit message lists suspend/WOL as
future work ("we'll reuse that flag in suspend/WOL, set_ringparam and
change_mtu"). Given that the old netif_running() check is now gone, should
macb_suspend()/macb_resume() switch to macb_quiesce_start()/
macb_quiesce_end() in this patch rather than later?

> @@ -3155,8 +3202,6 @@ static int macb_open(struct net_device *netdev)
>  {
>  	size_t bufsz = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + NET_IP_ALIGN;
>  	struct macb *bp = netdev_priv(netdev);
> -	struct macb_queue *queue;
> -	unsigned int q;
>  	int err;
>  
>  	netdev_dbg(bp->netdev, "open\n");
> @@ -3180,10 +3225,7 @@ static int macb_open(struct net_device *netdev)
>  		goto free_rings;
>  	macb_init_buffers(bp);
>  
> -	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> -		napi_enable(&queue->napi_rx);
> -		napi_enable(&queue->napi_tx);
> -	}
> +	macb_quiesce_end(bp);
>  
>  	macb_init_hw(bp);
>  
> @@ -3210,11 +3252,10 @@ static int macb_open(struct net_device *netdev)
>  	phy_power_off(bp->phy);
>  
>  reset_hw:
> +	/* The netdev stays down: quiesce and drain, as macb_close() does. */
> +	macb_quiesce_start(bp);
> +
>  	macb_reset_hw(bp);
> -	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> -		napi_disable(&queue->napi_rx);
> -		napi_disable(&queue->napi_tx);
> -	}
>  free_rings:
>  	macb_free(bp);
>  pm_exit:

[Severity: High]
This is a pre-existing issue, but does the new reset_hw path make the
macb_set_ringparam() failure case worse?

macb_set_ringparam() drops the macb_open() return value entirely:

	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;

Failures before macb_quiesce_end() (pm_runtime_resume_and_get(),
macb_alloc()'s dma_alloc_coherent(), mog_init_rings() RX skb allocation)
return with the NAPI instances still disabled from the preceding
macb_close(). Failures after it now take the reset_hw label, which calls
macb_quiesce_start() and disables them a second time.

Since this path bypasses dev_open()/dev_close(), netif_running() stays
true, so the next ndo_stop ('ip link set dev ethX down', another
ethtool -G, or macb_suspend()) re-enters macb_close() ->
macb_quiesce_start() -> napi_disable() on an already disabled NAPI, and
napi_disable_locked() spins forever with RTNL and the netdev instance lock
held.

Should macb_set_ringparam() propagate macb_open()'s error, and should
macb_quiesce_start()/macb_quiesce_end() be made robust against being
called twice in a row?

> @@ -3225,19 +3266,17 @@ static int macb_open(struct net_device *netdev)
>  static int macb_close(struct net_device *netdev)
>  {
>  	struct macb *bp = netdev_priv(netdev);
> -	struct macb_queue *queue;
>  	unsigned long flags;
>  	unsigned int q;
>  
> -	netif_tx_stop_all_queues(netdev);
> +	macb_quiesce_start(bp);
>  
> -	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> -		napi_disable(&queue->napi_rx);
> -		napi_disable(&queue->napi_tx);
> +	/* Drain the BH contexts before stopping the queues: NAPI completion
> +	 * and tx_error_task wake them up.
> +	 */
> +	netif_tx_stop_all_queues(netdev);
> +	for (q = 0; q < bp->num_queues; ++q)
>  		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, q));
> -	}
> -
> -	cancel_delayed_work_sync(&bp->tx_lpi_work);

[Severity: Medium]
Moving netif_tx_stop_all_queues() after the whole quiesce sequence looks
fine for the ndo_stop path, where the core already ran
dev_deactivate_many() so no xmit can happen. Is it also safe for the direct
caller?

macb_set_ringparam() calls macb_close() with the netdev up and the qdisc
still live:

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

In that path macb_start_xmit() remains callable for the whole
macb_quiesce_start() window (synchronize_irq() per queue,
cancel_work_sync() on hresp_err_bh_work and each tx_error_task,
two napi_disable() per queue, cancel_delayed_work_sync() on tx_lpi_work),
since ethtool only holds RTNL.

Two consequences I am unsure about:

Frames mapped into the TX ring and kicked with NCR.TSTART after napi_tx is
disabled and after macb_interrupt() started answering with
queue_writel(queue, IDR, -1) will never be reaped; they are dropped by
macb_free()'s unmap loop and only counted in queue->stats.tx_dropped.

An xmit landing after netdev_tx_reset_queue()'s dql_reset() calls
netdev_tx_sent_queue() for completions that are never accounted, since
macb_free() does no BQL accounting and macb_open() does not reset BQL
again, so dql->num_queued > num_completed persists across the resize. Can
that leave the queue XOFF-ed?

The commit message explains the new ordering as "Drain the BH contexts
before stopping the queues: NAPI completion and tx_error_task wake them
up", and the recap table only lists 'swap' as a future context. Would
keeping netif_tx_stop_all_queues() first, or converting
macb_set_ringparam() to dev_close()/dev_open() semantics, address this?

>  	phylink_stop(bp->phylink);
>  	phylink_disconnect_phy(bp->phylink);

[ ... ]

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