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 Tue Mar 17, 2026 at 2:25 AM CET, Kevin Hao wrote:
> 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?

Nah, after all no one is going to be bothered by those locks that
almost never happen. They have been here for a while after all. The
macb_resume() ones as well are probably useless because IRQs are
disabled.

Anyway, for this patch:

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks,

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