Message-ID: <742283a6-221f-4d87-ac18-90133c6df735@riseup.net>
Date: Thu, 27 Mar 2025 22:36:05 +0100
From: "Fernando F. Mancera" <ffmancera@...eup.net>
To: Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org,
 linux-kernel@...r.kernel.org
Cc: dwmw@...zon.co.uk, mhkelley@...look.com, mingo@...nel.org
Subject: Re: [PATCH v2] x86/i8253: fix possible deadlock when turning off the
 PIT



On 27/03/2025 22:17, Thomas Gleixner 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.
> 

Thanks for taking the time to write this extensive reply. I agree on the 
arguments provided here.

> 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.
> 

I agree on this statement. I just wanted to remark I didn't know that 
during the pit_timer_init() call can't be concurrency on the lock at 
all. I do know now, tho. Thanks for explaining.

I will provide a new patch with your suggestion to use scoped_guardian() 
and wording the commit message and description properly.

Thanks,
ffmancera.

> 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...
> 
> Thanks,
> 
>          tglx

Powered by blists - more mailing lists