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

[Ingo Molnar]

* Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> On Thu, Mar 27 2025 at 20:54, Fernando Fernandez Mancera wrote:
> > On 3/27/25 6:15 PM, Thomas Gleixner wrote:
> > 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.
> 
> I personally don't care whether there is consensus simply because it's a
> matter of fact, that at the point where pit_timer_init() is invoked there
> can't be concurrency on the lock by any means. Therefore it _is_ a false
> positive.
> 
> Ingo is right that pit_timer_init() should disable interrupts before
> invoking clockevent_i8253_disable() and not inflicting the irqsave() on
> the callback function.
> 
> But it should do so for the sake of consistency and correctness and not
> to "fix" a impossible deadlock or an magically assumed invalid assumption.
> 
> The assumption,
> 
>     - assumed that the author of the offending commit made
>       any assumptions at all (pun intended) -
> 
> that invoking clockevent_i8253_disable() with interrupts enabled at this
> point in the boot process is harmless, is completely correct.
> 
> Therefore I really prefer to have this described as:
> 
>   x86/i8253: Invoke clockevent_i8253_disable() with interrupts disabled
> 
> with a proper explanation that the current code makes lockdep
> (rightfully) complain, but that it has no actual deadlock potential in
> the current state of the code.
> 
> That means the code change serves two purposes:
> 
>    1) Prevent lockdep from detecting a false positive
> 
>    2) Future proving the code
> 
> #1 is a matter of fact with the current code
>  
> #2 is valuable despite the fact that PIT is a legacy, which won't
>    suddenly roar its ugly head in unexpected ways.
> 
> I know that's word smithing, but I'm observing a increasing tendency of
> "fixing" problems based on tooling output without any further analysis.
> 
> I'm absolutely not blaming you for that and your patch is fine, except
> for the technical details I pointed out and the change log related
> issues.
> 
> Though I really want people to sit down and think about the factual
> impact of a tool based problem observation. Tools are good in detecting
> problems, but they are patently bad in properly analysing them. And no,
> AI is not going to fix that anytime soon, quite the contrary.
> 
> Taking the tools output at face value leads exactly to what triggered my
> response:
> 
>   "fix possible deadlock when turning off the PIT"
> 
> which is misleading at best as I explained before.
> 
> Wording matters, but maybe that's just me...

I fully agree with all of your suggestions.

I suggested a save/restore cycle primarily because I wasn't 100% 
certain that IRQs were always enabled in that call chain, and a 
superfluous save is better than an unintended IRQ-enable. So it was a 
chicken-bit. :-/

Your title suggestion is also much better, it makes it clear that this 
is not a potential deadlock.

Wording matters.

Thanks,

	Ingo