Re: [PATCH net 1/2] net: macb: Move devm_{free,request}_irq() out of spin lock area

["Théo Lebrun" <theo.lebrun@bootlin.com>]

On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
> @@ -5962,6 +5962,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  			/* write IP address into register */
>  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
>  		}
> +		spin_unlock_irqrestore(&bp->lock, flags);
>  
>  		/* Change interrupt handler and
>  		 * Enable WoL IRQ on queue 0
> @@ -5974,11 +5975,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  				dev_err(dev,
>  					"Unable to request IRQ %d (error %d)\n",
>  					bp->queues[0].irq, err);
> -				spin_unlock_irqrestore(&bp->lock, flags);
>  				return err;
>  			}
> +			spin_lock_irqsave(&bp->lock, flags);
>  			queue_writel(bp->queues, IER, GEM_BIT(WOL));
>  			gem_writel(bp, WOL, tmp);
> +			spin_unlock_irqrestore(&bp->lock, flags);
>  		} else {
>  			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
>  					       IRQF_SHARED, netdev->name, bp->queues);
> @@ -5986,13 +5988,13 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  				dev_err(dev,
>  					"Unable to request IRQ %d (error %d)\n",
>  					bp->queues[0].irq, err);
> -				spin_unlock_irqrestore(&bp->lock, flags);
>  				return err;
>  			}
> +			spin_lock_irqsave(&bp->lock, flags);
>  			queue_writel(bp->queues, IER, MACB_BIT(WOL));
>  			macb_writel(bp, WOL, tmp);
> +			spin_unlock_irqrestore(&bp->lock, flags);
>  		}
> -		spin_unlock_irqrestore(&bp->lock, flags);
>  
>  		enable_irq_wake(bp->queues[0].irq);
>  	}

So it used to be that approximatively the whole macb_suspend() function
was ran under the bp->lock spinlock. Now you split it in two to avoid
calling IRQ functions in atomic context:
 - (1) the disable queues & silence IRQs part and,
 - (2) the enable WOL part (IER and WOL reg writes).

Why do you need to grab bp->lock for the 2nd part? All queues are
disabled anyway and IRQs masked. BH features like our work queues are
disabled during the dev_pm_ops.suspend() calls anyway. Maybe I am
forgetting? Or this was just out of caution?

Thanks,

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