Re: [PATCH net v2 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] macb_probe() initializes bp->hresp_err_bh_work and
  bp->tx_lpi_work (lines 6056-6057) only after register_netdev() (line…

Pre-existing issues:
- [High] The suspend path still has the same tx_error_task vs. NAPI
  deadlock that this patch fixes for close.
- [High] macb_set_ringparam()
  (drivers/net/ethernet/cadence/macb_main.c:3801-3812) calls…
- [High] macb_set_ringparam() (line 3803) calls macb_close() directly,
  holding only RTNL, so the core's…

--

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

This patch adds an irq_quiesced flag. macb_interrupt() checks it and
disarms itself while the interface is closed. It also adds
macb_quiesce_start() and macb_quiesce_end(). macb_quiesce_start() drains
the IRQ handlers, hresp_err_bh_work, tx_error_task, NAPI and tx_lpi_work,
in that order. macb_close() and the macb_open() error path now use these
helpers.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 18a1b5f7ad916..1d6361c0d8bc9 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2009,6 +2009,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: Low]
macb_probe() initializes these works only after register_netdev() has
returned:

drivers/net/ethernet/cadence/macb_main.c:macb_probe() {
    ...
	err = register_netdev(netdev);
	...
	INIT_WORK(&bp->hresp_err_bh_work, macb_hresp_error_task);
	INIT_DELAYED_WORK(&bp->tx_lpi_work, macb_tx_lpi_work_fn);
    ...
}

Userspace can open the interface inside that window, for example in
response to the registration uevent. Suppose the open then fails in
phy_set_mode_ext(), phy_power_on() or macb_phylink_connect(), or the
interface is closed right away. Can cancel_work_sync() here then trigger
the WARN_ON(!work->func) in __flush_work() on a work that is still
zeroed?

The close path already cancelled tx_lpi_work before this patch. The new
parts are the hresp_err_bh_work cancel and the cancels on the macb_open()
error path.

Would moving INIT_WORK() and INIT_DELAYED_WORK() before register_netdev(),
or into macb_init_dflt(), avoid this?

> +
> +	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);
> +	}

[Severity: High]
This is a pre-existing issue and was not introduced by this patch.
macb_suspend() still seems to have the same tx_error_task vs. NAPI
deadlock that this patch fixes for close. It disables NAPI without
setting irq_quiesced and without cancelling tx_error_task or
hresp_err_bh_work:

drivers/net/ethernet/cadence/macb_main.c:macb_suspend() {
    ...
	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);
	}
    ...
}

In the non-WoL branch the IRQs are still unmasked at this point. On a TX
error, macb_interrupt()->macb_interrupt_misc() can still call
schedule_work(&queue->tx_error_task). On HRESP it can queue
hresp_err_bh_work.

In the WoL branch, a tx_error_task queued just before the IDR write can
also run after the napi_disable() calls.

macb_tx_error_task() then calls this on a NAPI that is already disabled:

	napi_disable(&queue->napi_tx);

Would that spin forever in napi_disable_locked() while holding
netdev_lock? macb_resume() then calls napi_enable(), which takes the same
netdev_lock, so it looks like resume would never complete.

The commit message says the flag will be reused for suspend/WOL, and the
patch is Cc: stable for this class of deadlock. Could macb_suspend() and
macb_resume() use macb_quiesce_start() and macb_quiesce_end() too?

[ ... ]

> @@ -3234,11 +3276,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);

[Severity: High]
This isn't a bug introduced by this patch, but macb_set_ringparam() calls
macb_close() and macb_open() directly and ignores macb_open()'s return
value:

drivers/net/ethernet/cadence/macb_main.c:macb_set_ringparam() {
    ...
	if (netif_running(bp->netdev)) {
		reset = 1;
		macb_close(bp->netdev);
	}
    ...
	if (reset)
		macb_open(bp->netdev);

	return 0;
}

Every macb_open() failure path leaves NAPI disabled. The macb_alloc() and
mog_init_rings() failures jump past macb_quiesce_end(). The later failures
call macb_quiesce_start() here.

These paths also free the rings and drop the runtime PM reference, but
__LINK_STATE_START stays set. A large ring size can make macb_alloc() fail
under memory pressure, and ethtool -G still reports success.

A later ip link set down, or another ethtool -G, calls
macb_close()->macb_quiesce_start() again. Does napi_disable() then wait
forever in napi_disable_locked() with RTNL held? If that step were somehow
passed, would macb_free() and pm_runtime_put() run a second time?

This also doesn't match the new comments. They describe irq_quiesced as
"True when the interface is closed" and say it is "cleared by macb_open()
and re-armed by macb_close()".

> +
>  	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:
> @@ -3249,19 +3290,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);

[Severity: High]
This is a pre-existing issue, but it is about draining contexts before
teardown. macb_set_ringparam() calls macb_close() directly under RTNL, so
the core's dev_deactivate_many()/synchronize_net() never runs to drain an
ndo_start_xmit that is already in flight.

netif_tx_stop_all_queues() only sets the XOFF bits. It doesn't take the
xmit lock, and it doesn't wait for a macb_start_xmit() that has already
passed the queue-stopped check.

macb_start_xmit() writes queue->tx_skb and the TX descriptors under
queue->tx_ptr_lock. Neither macb_close() nor macb_free() takes that lock
before this:

drivers/net/ethernet/cadence/macb_main.c:macb_free() {
    ...
			kfree(queue->tx_skb);
			queue->tx_skb = NULL;
    ...
}

The same applies to the dma_free_coherent() of the TX ring.

Can an xmit that is still running during ethtool -G then write into the
freed tx_skb array or the freed TX ring? This seems more likely on
PREEMPT_RT, where the xmit path and tx_ptr_lock can be preempted.

Would netif_tx_disable() or an explicit synchronize_net() here close this
window?

> +	for (q = 0; q < bp->num_queues; ++q)
>  		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, q));

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-macb-close-v2-0-360efa565914%40bootlin.com