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

["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 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?

This race is out of this world, but does technically exist.
Let's fix it anyway.

>> +
>> +	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()?

Pre-existing. Similarly to HRESP task, the infra will be put in place
and reusable to fix the suspend path which needs to shutdown all BH
features, even in WOL case.

[...]

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

We ignore macb_set_ringparam() close/open when netif_running() because
we know what the right solution is (context swapping) and it's coming.
One step at a time.

[...]

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

Again, context swapping is the solution. Sashiko complains the
close/open dance in set_ringparam is unsafe, but anyway before this
series macb_close() is super race sensitive and we fix it here. One
step at a time.

[...]

Thanks,

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