Re: [PATCH v2] x86/i8253: fix possible deadlock when turning off the PIT

[Fernando Fernandez Mancera]

On 3/27/25 6:15 PM, Thomas Gleixner wrote:
> On Thu, Mar 27 2025 at 16:22, Fernando Fernandez Mancera wrote:
> > As the PIT could be disabled during the init, it can possibly cause a
> > deadlock. hpet_time_init()->pit_timer_init() is called without IRQ off.
> > It assumes that clockevent_i8253_disable() is IRQ-safe, which it isn't.
>
> It assumes nothing and all the missing interrupt disable is causing is a
> lockdep false positive.
>
> Lockdep complains correctly due to the observed contexts, but in reality
> there is no possible deadlock at all. Definitely not the one your
> subject line is claiming to be possible.
>
> At the point where pit_timer_init() is invoked there is no other usage
> of 8253_lock possible because the system is still in the very early boot
> stage.

Thanks for taking the time to review the patch.

I was not aware of this. I took a look to other functions that used 
i8253_lock like pcspkr_event() and thought it could be possible.

> So disabling interrupt here just prevents lockdep triggering a false
> positive and not more.
>
> Please analyze problems properly instead of assuming that the lockdep
> splat is the ultimate truth.

I tried, but it seems I failed. I just found out this while working on 
something else not related in a VM.

> >   bool __init pit_timer_init(void)
> >   {
> > +	unsigned long flags;
> > +
> >   	if (!use_pit()) {
> >   		/*
> >   		 * Don't just ignore the PIT. Ensure it's stopped, because
> >   		 * VMMs otherwise steal CPU time just to pointlessly waggle
> >   		 * the (masked) IRQ.
> >   		 */
> > +		local_irq_save(flags);
>
> Why save()? You just established that interrupts are enabled here, so
> this really wants to be:

I followed Ingo's suggestions on V1 [1]. It made sense to me, if the 
problem was the one described on the commit message. So, is there 
consensus about this being a false positive? If so, I will send a new 
patch just suppressing the warning as suggested below.

[1] https://lore.kernel.org/linux-kernel/Z-B6ob0zLZr81e8i@xxxxxxxxx/

>                  scoped_guard(irq)()
> 	              	clockevent_i8253_disable();
>    		return false;
>
> Thanks,
>
>          tglx