[PATCH v3] x86/i8253: Invoke clockevent_i8253_disable() with interrupts disabled

[PATCH v3] x86/i8253: Invoke clockevent_i8253_disable() with interrupts disabled

[Fernando Fernandez Mancera]

Lockdep complains correctly due to the context, but in reality there is
no possible deadlock. At the point where pit_timer_init() is called
there is no other possible usage of i8253_lock because the system is
still in the very early boot stage.

pit_timer_init() should disable interrupts before calling
clockevent_i8253_disable() and do not inflict the
raw_spin_lock_irqsave() on the callback function. This prevents lockdep
from detecting a false positive and future proves the code.

[   45.408952] =====================================================
[   45.408970] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
[   45.408974] 6.14.0-rc7+ #6 Not tainted
[   45.408978] -----------------------------------------------------
[   45.408980] systemd-sleep/3324 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
[   45.408986] ffffffffb2c23398 (i8253_lock){+.+.}-{2:2}, at: pcspkr_event+0x3f/0xe0 [pcspkr]
[   45.409004]
               and this task is already holding:
[   45.409006] ffff9c334d7c2230 (&dev->event_lock){-.-.}-{3:3}, at: input_dev_resume+0x21/0x50
[   45.409023] which would create a new lock dependency:
[   45.409025]  (&dev->event_lock){-.-.}-{3:3} -> (i8253_lock){+.+.}-{2:2}
[   45.409043]
               but this new dependency connects a HARDIRQ-irq-safe lock:
[   45.409045]  (&dev->event_lock){-.-.}-{3:3}
[   45.409052]
               ... which became HARDIRQ-irq-safe at:
[   45.409055]   lock_acquire+0xd0/0x2f0
[   45.409062]   _raw_spin_lock_irqsave+0x48/0x70
[   45.409067]   input_event+0x3c/0x80
[   45.409071]   atkbd_receive_byte+0x9b/0x6e0
[   45.409077]   ps2_interrupt+0xb2/0x1d0
[   45.409082]   serio_interrupt+0x4a/0x90
[   45.409087]   i8042_handle_data+0xf8/0x280
[   45.409091]   i8042_interrupt+0x11/0x40
[   45.409095]   __handle_irq_event_percpu+0x87/0x260
[   45.409100]   handle_irq_event+0x38/0x90
[   45.409105]   handle_edge_irq+0x8b/0x230
[   45.409109]   __common_interrupt+0x5c/0x120
[   45.409114]   common_interrupt+0x80/0xa0
[   45.409120]   asm_common_interrupt+0x26/0x40
[   45.409125]   pv_native_safe_halt+0xf/0x20
[   45.409130]   default_idle+0x9/0x20
[   45.409135]   default_idle_call+0x7a/0x1d0
[   45.409140]   do_idle+0x215/0x260
[   45.409144]   cpu_startup_entry+0x29/0x30
[   45.409149]   start_secondary+0x132/0x170
[   45.409153]   common_startup_64+0x13e/0x141
[   45.409158]
               to a HARDIRQ-irq-unsafe lock:
[   45.409161]  (i8253_lock){+.+.}-{2:2}
[   45.409167]
               ... which became HARDIRQ-irq-unsafe at:
[   45.409170] ...
[   45.409172]   lock_acquire+0xd0/0x2f0
[   45.409177]   _raw_spin_lock+0x30/0x40
[   45.409181]   clockevent_i8253_disable+0x1c/0x60
[   45.409186]   pit_timer_init+0x25/0x50
[   45.409191]   hpet_time_init+0x46/0x50
[   45.409196]   x86_late_time_init+0x1b/0x40
[   45.409201]   start_kernel+0x962/0xa00
[   45.409206]   x86_64_start_reservations+0x24/0x30
[   45.409211]   x86_64_start_kernel+0xed/0xf0
[   45.409215]   common_startup_64+0x13e/0x141
[   45.409220]
               other info that might help us debug this:

[   45.409222]  Possible interrupt unsafe locking scenario:

[   45.409224]        CPU0                    CPU1
[   45.409226]        ----                    ----
[   45.409228]   lock(i8253_lock);
[   45.409234]                                local_irq_disable();
[   45.409237]                                lock(&dev->event_lock);
[   45.409243]                                lock(i8253_lock);
[   45.409249]   <Interrupt>
[   45.409251]     lock(&dev->event_lock);
[   45.409257]
                *** DEADLOCK ***

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
v1: initial patch
v2: use local_irq_save()/restore() around clockevent_i8253_disable()
v3: improve wording of commit message, dropped "Fixes" tag and used
scoped_guard(irq) instead.
---
 arch/x86/kernel/i8253.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/i8253.c b/arch/x86/kernel/i8253.c
index 80e262bb627f..cb9852ad6098 100644
--- a/arch/x86/kernel/i8253.c
+++ b/arch/x86/kernel/i8253.c
@@ -46,7 +46,8 @@ bool __init pit_timer_init(void)
 		 * VMMs otherwise steal CPU time just to pointlessly waggle
 		 * the (masked) IRQ.
 		 */
-		clockevent_i8253_disable();
+		scoped_guard(irq)
+			clockevent_i8253_disable();
 		return false;
 	}
 	clockevent_i8253_init(true);
-- 
2.49.0

[PATCH -v4] x86/i8253: Call clockevent_i8253_disable() with interrupts disabled

[Ingo Molnar]

I've cleaned up and simplified the changelog, see the patch below.

Thomas, does this look good to you too?

Thanks,

	Ingo

=========================>
From: Fernando Fernandez Mancera <ffmancera@riseup.net>
Date: Fri, 28 Mar 2025 00:43:57 +0100
Subject: [PATCH] x86/i8253: Call clockevent_i8253_disable() with interrupts disabled

There's a lockdep false positive warning related to i8253_lock:

  WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
  ...
  systemd-sleep/3324 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
  ffffffffb2c23398 (i8253_lock){+.+.}-{2:2}, at: pcspkr_event+0x3f/0xe0 [pcspkr]

  ...
  ... which became HARDIRQ-irq-unsafe at:
  ...
    lock_acquire+0xd0/0x2f0
    _raw_spin_lock+0x30/0x40
    clockevent_i8253_disable+0x1c/0x60
    pit_timer_init+0x25/0x50
    hpet_time_init+0x46/0x50
    x86_late_time_init+0x1b/0x40
    start_kernel+0x962/0xa00
    x86_64_start_reservations+0x24/0x30
    x86_64_start_kernel+0xed/0xf0
    common_startup_64+0x13e/0x141
  ...

Lockdep complains due pit_timer_init() using the lock in an IRQ-unsafe
fashion, but it's a false positive, because there is no deadlock
possible at that point due to init ordering: at the point where
pit_timer_init() is called there is no other possible usage of
i8253_lock because the system is still in the very early boot stage
with no interrupts.

But in any case, pit_timer_init() should disable interrupts before
calling clockevent_i8253_disable() out of general principle, and to
keep lockdep working even in this scenario.

Use scoped_guard() for that, as suggested by Thomas Gleixner.

[ mingo: Cleaned up the changelog. ]

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20250327234357.3383-1-ffmancera@riseup.net
---
 arch/x86/kernel/i8253.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/i8253.c b/arch/x86/kernel/i8253.c
index 80e262bb627f..cb9852ad6098 100644
--- a/arch/x86/kernel/i8253.c
+++ b/arch/x86/kernel/i8253.c
@@ -46,7 +46,8 @@ bool __init pit_timer_init(void)
 		 * VMMs otherwise steal CPU time just to pointlessly waggle
 		 * the (masked) IRQ.
 		 */
-		clockevent_i8253_disable();
+		scoped_guard(irq)
+			clockevent_i8253_disable();
 		return false;
 	}
 	clockevent_i8253_init(true);

Re: [PATCH -v4] x86/i8253: Call clockevent_i8253_disable() with interrupts disabled

[Fernando F. Mancera]

On 01/04/2025 11:23, Ingo Molnar wrote:
> 
> I've cleaned up and simplified the changelog, see the patch below.
> 

Thank you. To me it looks good. Should I repost it?

Thanks,
Fernando.

> Thomas, does this look good to you too?
> 
> Thanks,
> 
> 	Ingo
> 
> =========================>
> From: Fernando Fernandez Mancera <ffmancera@riseup.net>
> Date: Fri, 28 Mar 2025 00:43:57 +0100
> Subject: [PATCH] x86/i8253: Call clockevent_i8253_disable() with interrupts disabled
> 
> There's a lockdep false positive warning related to i8253_lock:
> 
>    WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
>    ...
>    systemd-sleep/3324 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
>    ffffffffb2c23398 (i8253_lock){+.+.}-{2:2}, at: pcspkr_event+0x3f/0xe0 [pcspkr]
> 
>    ...
>    ... which became HARDIRQ-irq-unsafe at:
>    ...
>      lock_acquire+0xd0/0x2f0
>      _raw_spin_lock+0x30/0x40
>      clockevent_i8253_disable+0x1c/0x60
>      pit_timer_init+0x25/0x50
>      hpet_time_init+0x46/0x50
>      x86_late_time_init+0x1b/0x40
>      start_kernel+0x962/0xa00
>      x86_64_start_reservations+0x24/0x30
>      x86_64_start_kernel+0xed/0xf0
>      common_startup_64+0x13e/0x141
>    ...
> 
> Lockdep complains due pit_timer_init() using the lock in an IRQ-unsafe
> fashion, but it's a false positive, because there is no deadlock
> possible at that point due to init ordering: at the point where
> pit_timer_init() is called there is no other possible usage of
> i8253_lock because the system is still in the very early boot stage
> with no interrupts.
> 
> But in any case, pit_timer_init() should disable interrupts before
> calling clockevent_i8253_disable() out of general principle, and to
> keep lockdep working even in this scenario.
> 
> Use scoped_guard() for that, as suggested by Thomas Gleixner.
> 
> [ mingo: Cleaned up the changelog. ]
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/r/20250327234357.3383-1-ffmancera@riseup.net
> ---
>   arch/x86/kernel/i8253.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/i8253.c b/arch/x86/kernel/i8253.c
> index 80e262bb627f..cb9852ad6098 100644
> --- a/arch/x86/kernel/i8253.c
> +++ b/arch/x86/kernel/i8253.c
> @@ -46,7 +46,8 @@ bool __init pit_timer_init(void)
>   		 * VMMs otherwise steal CPU time just to pointlessly waggle
>   		 * the (masked) IRQ.
>   		 */
> -		clockevent_i8253_disable();
> +		scoped_guard(irq)
> +			clockevent_i8253_disable();
>   		return false;
>   	}
>   	clockevent_i8253_init(true);

Re: [PATCH -v4] x86/i8253: Call clockevent_i8253_disable() with interrupts disabled

[Thomas Gleixner]

On Tue, Apr 01 2025 at 11:23, Ingo Molnar wrote:

> I've cleaned up and simplified the changelog, see the patch below.
>
> Thomas, does this look good to you too?

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[tip: timers/urgent] x86/i8253: Call clockevent_i8253_disable() with interrupts disabled

[tip-bot2 for Fernando Fernandez Mancera]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     3940f5349b476197fb079c5aa19c9a988de64efb
Gitweb:        https://git.kernel.org/tip/3940f5349b476197fb079c5aa19c9a988de64efb
Author:        Fernando Fernandez Mancera <ffmancera@riseup.net>
AuthorDate:    Tue, 01 Apr 2025 11:23:03 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 11 Apr 2025 07:28:20 +02:00

x86/i8253: Call clockevent_i8253_disable() with interrupts disabled

There's a lockdep false positive warning related to i8253_lock:

  WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
  ...
  systemd-sleep/3324 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
  ffffffffb2c23398 (i8253_lock){+.+.}-{2:2}, at: pcspkr_event+0x3f/0xe0 [pcspkr]

  ...
  ... which became HARDIRQ-irq-unsafe at:
  ...
    lock_acquire+0xd0/0x2f0
    _raw_spin_lock+0x30/0x40
    clockevent_i8253_disable+0x1c/0x60
    pit_timer_init+0x25/0x50
    hpet_time_init+0x46/0x50
    x86_late_time_init+0x1b/0x40
    start_kernel+0x962/0xa00
    x86_64_start_reservations+0x24/0x30
    x86_64_start_kernel+0xed/0xf0
    common_startup_64+0x13e/0x141
  ...

Lockdep complains due pit_timer_init() using the lock in an IRQ-unsafe
fashion, but it's a false positive, because there is no deadlock
possible at that point due to init ordering: at the point where
pit_timer_init() is called there is no other possible usage of
i8253_lock because the system is still in the very early boot stage
with no interrupts.

But in any case, pit_timer_init() should disable interrupts before
calling clockevent_i8253_disable() out of general principle, and to
keep lockdep working even in this scenario.

Use scoped_guard() for that, as suggested by Thomas Gleixner.

[ mingo: Cleaned up the changelog. ]

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/Z-uwd4Bnn7FcCShX@gmail.com
---
 arch/x86/kernel/i8253.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/i8253.c b/arch/x86/kernel/i8253.c
index 80e262b..cb9852a 100644
--- a/arch/x86/kernel/i8253.c
+++ b/arch/x86/kernel/i8253.c
@@ -46,7 +46,8 @@ bool __init pit_timer_init(void)
 		 * VMMs otherwise steal CPU time just to pointlessly waggle
 		 * the (masked) IRQ.
 		 */
-		clockevent_i8253_disable();
+		scoped_guard(irq)
+			clockevent_i8253_disable();
 		return false;
 	}
 	clockevent_i8253_init(true);