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

[Kevin Hao <haokexin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]

On Mon, Mar 16, 2026 at 07:11:34PM +0100, Théo Lebrun wrote:
> 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?

You are right. I agree that the lock may not be necessary in this scenario.

> Or this was just out of caution?

This is just out of cautious consideration. Do you think I should delete these
spinlocks?

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]