[PATCH v2 0/1] loongson: Fix idle VS timer enqueue

[PATCH v2 0/1] loongson: Fix idle VS timer enqueue

[Marco Crivellari]

This patch aim to fix issues if timers are queued because __arch_cpu_idle()
only checks if TIF_NEED_RESCHED is set before going to sleep.

Changes are described in [1], and [2] improve the patch further.

The following patch is the result of [1] and [2] combined.
Code compiled successfully, but I couldn't test the changes because of some
trouble emulating a loongarch machine; I tried to boot with and without
this patch, but a panic occurred, due likely to a wrong VM configuration
(panic occurred right after the i8042 probing).


- [1] https://lore.kernel.org/all/20230811170049.308866-5-frederic@kernel.org/#t
- [2] https://lore.kernel.org/all/06cf8750-901d-2f65-fb9e-81980688e017@loongson.cn/

---
Changes in v2:
 - arch/loongarch/kernel/genex.S: code improved as suggested by Huacai: 
	https://lore.kernel.org/loongarch/CAAhV-H6OKwmxhF_iWJEc8OEZ==h8dTdorTwkE8oMUMWAbU6xeg@mail.gmail.com/
 - arch/loongarch/kernel/reset.c: asm inline inside the while loops

Marco Crivellari (1):
  loongson: Fix idle VS timer enqueue

 arch/loongarch/kernel/genex.S | 31 +++++++++++++++++--------------
 arch/loongarch/kernel/idle.c  |  3 +--
 arch/loongarch/kernel/reset.c | 18 +++++++++---------
 3 files changed, 27 insertions(+), 25 deletions(-)

-- 
2.48.1

[PATCH v2 1/1] [PATCH] loongson: Fix idle VS timer enqueue

[Marco Crivellari]

Loongson re-enables interrupts on its idle routine and performs a
TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.

The IRQs firing between the check and the idling instruction may set the
TIF_NEED_RESCHED flag. In order to deal with the such a race, IRQs
interrupting __arch_cpu_idle() rollback their return address to the
beginning of __arch_cpu_idle() so that TIF_NEED_RESCHED is checked
again before going back to sleep.

However idle IRQs can also queue timers that may require a tick
reprogramming through a new generic idle loop iteration but those timers
would go unnoticed here because __arch_cpu_idle() only checks
TIF_NEED_RESCHED. It doesn't check for pending timers.

Fix this with fast-forwarding idle IRQs return value to the end of the
idle routine instead of the beginning, so that the generic idle loop
handles both TIF_NEED_RESCHED and pending timers.

Fixes: 0603839b18f4 ("LoongArch: Add exception/interrupt handling")
Cc: WANG Xuerui <kernel@xen0n.name>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 arch/loongarch/kernel/genex.S | 31 +++++++++++++++++--------------
 arch/loongarch/kernel/idle.c  |  3 +--
 arch/loongarch/kernel/reset.c | 18 +++++++++---------
 3 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/arch/loongarch/kernel/genex.S b/arch/loongarch/kernel/genex.S
index 86d5d90ebefe..9623298a5cf1 100644
--- a/arch/loongarch/kernel/genex.S
+++ b/arch/loongarch/kernel/genex.S
@@ -18,28 +18,31 @@
 
 	.align	5
 SYM_FUNC_START(__arch_cpu_idle)
-	/* start of rollback region */
-	LONG_L	t0, tp, TI_FLAGS
-	nop
-	andi	t0, t0, _TIF_NEED_RESCHED
-	bnez	t0, 1f
-	nop
-	nop
-	nop
+	/* start of idle interrupt region */
+	ori		t0, zero, CSR_CRMD_IE
+	/* idle instruction needs irq enabled */
+	csrxchg	t0, t0, LOONGARCH_CSR_CRMD
+	/*
+	 * If an interrupt lands here; between enabling interrupts above and
+	 * going idle on the next instruction, we must *NOT* go idle since the
+	 * interrupt could have set TIF_NEED_RESCHED or caused an timer to need
+	 * reprogramming. Fall through -- see handle_vint() below -- and have
+	 * the idle loop take care of things.
+	 */
 	idle	0
-	/* end of rollback region */
-1:	jr	ra
+	/* end of idle interrupt region */
+SYM_INNER_LABEL(__arch_cpu_idle_exit, SYM_L_LOCAL)
+	jr	ra
 SYM_FUNC_END(__arch_cpu_idle)
 
 SYM_CODE_START(handle_vint)
 	UNWIND_HINT_UNDEFINED
 	BACKUP_T0T1
 	SAVE_ALL
-	la_abs	t1, __arch_cpu_idle
+	la_abs	t1, __arch_cpu_idle_exit
 	LONG_L	t0, sp, PT_ERA
-	/* 32 byte rollback region */
-	ori	t0, t0, 0x1f
-	xori	t0, t0, 0x1f
+	/* 3 instructions idle interrupt region */
+	ori	t0, t0, 0x0c
 	bne	t0, t1, 1f
 	LONG_S	t0, sp, PT_ERA
 1:	move	a0, sp
diff --git a/arch/loongarch/kernel/idle.c b/arch/loongarch/kernel/idle.c
index 0b5dd2faeb90..54b247d8cdb6 100644
--- a/arch/loongarch/kernel/idle.c
+++ b/arch/loongarch/kernel/idle.c
@@ -11,7 +11,6 @@
 
 void __cpuidle arch_cpu_idle(void)
 {
-	raw_local_irq_enable();
-	__arch_cpu_idle(); /* idle instruction needs irq enabled */
+	__arch_cpu_idle();
 	raw_local_irq_disable();
 }
diff --git a/arch/loongarch/kernel/reset.c b/arch/loongarch/kernel/reset.c
index 1ef8c6383535..8fd8c44b02cb 100644
--- a/arch/loongarch/kernel/reset.c
+++ b/arch/loongarch/kernel/reset.c
@@ -32,9 +32,9 @@ void machine_halt(void)
 	pr_notice("\n\n** You can safely turn off the power now **\n\n");
 	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
 
-	while (true) {
-		__arch_cpu_idle();
-	}
+	while (1) {
+		asm volatile("idle 0" : : : "memory");
+	};
 }
 
 void machine_power_off(void)
@@ -52,9 +52,9 @@ void machine_power_off(void)
 	efi.reset_system(EFI_RESET_SHUTDOWN, EFI_SUCCESS, 0, NULL);
 #endif
 
-	while (true) {
-		__arch_cpu_idle();
-	}
+	while (1) {
+		asm volatile("idle 0" : : : "memory");
+	};
 }
 
 void machine_restart(char *command)
@@ -73,7 +73,7 @@ void machine_restart(char *command)
 	if (!acpi_disabled)
 		acpi_reboot();
 
-	while (true) {
-		__arch_cpu_idle();
-	}
+	while (1) {
+		asm volatile("idle 0" : : : "memory");
+	};
 }
-- 
2.48.1

Re: [PATCH v2 1/1] [PATCH] loongson: Fix idle VS timer enqueue

[kernel test robot]

Hi Marco,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.14-rc1 next-20250207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Crivellari/loongson-Fix-idle-VS-timer-enqueue/20250208-182213
base:   linus/master
patch link:    https://lore.kernel.org/r/20250208101924.20743-2-marco.crivellari%40suse.com
patch subject: [PATCH v2 1/1] [PATCH] loongson: Fix idle VS timer enqueue
config: loongarch-randconfig-r054-20250209 (https://download.01.org/0day-ci/archive/20250210/202502100258.jJThV6DP-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502100258.jJThV6DP-lkp@intel.com/

cocci warnings: (new ones prefixed by >>)
>> arch/loongarch/kernel/reset.c:37:2-3: Unneeded semicolon
   arch/loongarch/kernel/reset.c:57:2-3: Unneeded semicolon
   arch/loongarch/kernel/reset.c:78:2-3: Unneeded semicolon

vim +37 arch/loongarch/kernel/reset.c

    22	
    23	void machine_halt(void)
    24	{
    25	#ifdef CONFIG_SMP
    26		preempt_disable();
    27		smp_send_stop();
    28	#endif
    29		local_irq_disable();
    30		clear_csr_ecfg(ECFG0_IM);
    31	
    32		pr_notice("\n\n** You can safely turn off the power now **\n\n");
    33		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
    34	
    35		while (1) {
    36			asm volatile("idle 0" : : : "memory");
  > 37		};
    38	}
    39	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/1] [PATCH] loongson: Fix idle VS timer enqueue

[Huacai Chen]

Hi, Marco,

Don't care about the lkp robot, I have queued this patch with some
small modifications.
https://github.com/chenhuacai/linux/commit/a8aa673ea46c03b3f62992ffa4ffe810ac84f6e3

Huacai

On Sat, Feb 8, 2025 at 6:19 PM Marco Crivellari
<marco.crivellari@suse.com> wrote:
>
> Loongson re-enables interrupts on its idle routine and performs a
> TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.
>
> The IRQs firing between the check and the idling instruction may set the
> TIF_NEED_RESCHED flag. In order to deal with the such a race, IRQs
> interrupting __arch_cpu_idle() rollback their return address to the
> beginning of __arch_cpu_idle() so that TIF_NEED_RESCHED is checked
> again before going back to sleep.
>
> However idle IRQs can also queue timers that may require a tick
> reprogramming through a new generic idle loop iteration but those timers
> would go unnoticed here because __arch_cpu_idle() only checks
> TIF_NEED_RESCHED. It doesn't check for pending timers.
>
> Fix this with fast-forwarding idle IRQs return value to the end of the
> idle routine instead of the beginning, so that the generic idle loop
> handles both TIF_NEED_RESCHED and pending timers.
>
> Fixes: 0603839b18f4 ("LoongArch: Add exception/interrupt handling")
> Cc: WANG Xuerui <kernel@xen0n.name>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> ---
>  arch/loongarch/kernel/genex.S | 31 +++++++++++++++++--------------
>  arch/loongarch/kernel/idle.c  |  3 +--
>  arch/loongarch/kernel/reset.c | 18 +++++++++---------
>  3 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/arch/loongarch/kernel/genex.S b/arch/loongarch/kernel/genex.S
> index 86d5d90ebefe..9623298a5cf1 100644
> --- a/arch/loongarch/kernel/genex.S
> +++ b/arch/loongarch/kernel/genex.S
> @@ -18,28 +18,31 @@
>
>         .align  5
>  SYM_FUNC_START(__arch_cpu_idle)
> -       /* start of rollback region */
> -       LONG_L  t0, tp, TI_FLAGS
> -       nop
> -       andi    t0, t0, _TIF_NEED_RESCHED
> -       bnez    t0, 1f
> -       nop
> -       nop
> -       nop
> +       /* start of idle interrupt region */
> +       ori             t0, zero, CSR_CRMD_IE
> +       /* idle instruction needs irq enabled */
> +       csrxchg t0, t0, LOONGARCH_CSR_CRMD
> +       /*
> +        * If an interrupt lands here; between enabling interrupts above and
> +        * going idle on the next instruction, we must *NOT* go idle since the
> +        * interrupt could have set TIF_NEED_RESCHED or caused an timer to need
> +        * reprogramming. Fall through -- see handle_vint() below -- and have
> +        * the idle loop take care of things.
> +        */
>         idle    0
> -       /* end of rollback region */
> -1:     jr      ra
> +       /* end of idle interrupt region */
> +SYM_INNER_LABEL(__arch_cpu_idle_exit, SYM_L_LOCAL)
> +       jr      ra
>  SYM_FUNC_END(__arch_cpu_idle)
>
>  SYM_CODE_START(handle_vint)
>         UNWIND_HINT_UNDEFINED
>         BACKUP_T0T1
>         SAVE_ALL
> -       la_abs  t1, __arch_cpu_idle
> +       la_abs  t1, __arch_cpu_idle_exit
>         LONG_L  t0, sp, PT_ERA
> -       /* 32 byte rollback region */
> -       ori     t0, t0, 0x1f
> -       xori    t0, t0, 0x1f
> +       /* 3 instructions idle interrupt region */
> +       ori     t0, t0, 0x0c
>         bne     t0, t1, 1f
>         LONG_S  t0, sp, PT_ERA
>  1:     move    a0, sp
> diff --git a/arch/loongarch/kernel/idle.c b/arch/loongarch/kernel/idle.c
> index 0b5dd2faeb90..54b247d8cdb6 100644
> --- a/arch/loongarch/kernel/idle.c
> +++ b/arch/loongarch/kernel/idle.c
> @@ -11,7 +11,6 @@
>
>  void __cpuidle arch_cpu_idle(void)
>  {
> -       raw_local_irq_enable();
> -       __arch_cpu_idle(); /* idle instruction needs irq enabled */
> +       __arch_cpu_idle();
>         raw_local_irq_disable();
>  }
> diff --git a/arch/loongarch/kernel/reset.c b/arch/loongarch/kernel/reset.c
> index 1ef8c6383535..8fd8c44b02cb 100644
> --- a/arch/loongarch/kernel/reset.c
> +++ b/arch/loongarch/kernel/reset.c
> @@ -32,9 +32,9 @@ void machine_halt(void)
>         pr_notice("\n\n** You can safely turn off the power now **\n\n");
>         console_flush_on_panic(CONSOLE_FLUSH_PENDING);
>
> -       while (true) {
> -               __arch_cpu_idle();
> -       }
> +       while (1) {
> +               asm volatile("idle 0" : : : "memory");
> +       };
>  }
>
>  void machine_power_off(void)
> @@ -52,9 +52,9 @@ void machine_power_off(void)
>         efi.reset_system(EFI_RESET_SHUTDOWN, EFI_SUCCESS, 0, NULL);
>  #endif
>
> -       while (true) {
> -               __arch_cpu_idle();
> -       }
> +       while (1) {
> +               asm volatile("idle 0" : : : "memory");
> +       };
>  }
>
>  void machine_restart(char *command)
> @@ -73,7 +73,7 @@ void machine_restart(char *command)
>         if (!acpi_disabled)
>                 acpi_reboot();
>
> -       while (true) {
> -               __arch_cpu_idle();
> -       }
> +       while (1) {
> +               asm volatile("idle 0" : : : "memory");
> +       };
>  }
> --
> 2.48.1
>

Re: [PATCH v2 1/1] [PATCH] loongson: Fix idle VS timer enqueue

[Marco Crivellari]

Hi Huacai,

Thanks a lot, I will keep that in mind!
That's very kind of you, sorry for the trouble.

Marco


On Mon, Feb 10, 2025 at 3:19 AM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Marco,
>
> Don't care about the lkp robot, I have queued this patch with some
> small modifications.
> https://github.com/chenhuacai/linux/commit/a8aa673ea46c03b3f62992ffa4ffe810ac84f6e3
>
> Huacai
>
> On Sat, Feb 8, 2025 at 6:19 PM Marco Crivellari
> <marco.crivellari@suse.com> wrote:
> >
> > Loongson re-enables interrupts on its idle routine and performs a
> > TIF_NEED_RESCHED check afterwards before putting the CPU to sleep.
> >
> > The IRQs firing between the check and the idling instruction may set the
> > TIF_NEED_RESCHED flag. In order to deal with the such a race, IRQs
> > interrupting __arch_cpu_idle() rollback their return address to the
> > beginning of __arch_cpu_idle() so that TIF_NEED_RESCHED is checked
> > again before going back to sleep.
> >
> > However idle IRQs can also queue timers that may require a tick
> > reprogramming through a new generic idle loop iteration but those timers
> > would go unnoticed here because __arch_cpu_idle() only checks
> > TIF_NEED_RESCHED. It doesn't check for pending timers.
> >
> > Fix this with fast-forwarding idle IRQs return value to the end of the
> > idle routine instead of the beginning, so that the generic idle loop
> > handles both TIF_NEED_RESCHED and pending timers.
> >
> > Fixes: 0603839b18f4 ("LoongArch: Add exception/interrupt handling")
> > Cc: WANG Xuerui <kernel@xen0n.name>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
> > ---
> >  arch/loongarch/kernel/genex.S | 31 +++++++++++++++++--------------
> >  arch/loongarch/kernel/idle.c  |  3 +--
> >  arch/loongarch/kernel/reset.c | 18 +++++++++---------
> >  3 files changed, 27 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/loongarch/kernel/genex.S b/arch/loongarch/kernel/genex.S
> > index 86d5d90ebefe..9623298a5cf1 100644
> > --- a/arch/loongarch/kernel/genex.S
> > +++ b/arch/loongarch/kernel/genex.S
> > @@ -18,28 +18,31 @@
> >
> >         .align  5
> >  SYM_FUNC_START(__arch_cpu_idle)
> > -       /* start of rollback region */
> > -       LONG_L  t0, tp, TI_FLAGS
> > -       nop
> > -       andi    t0, t0, _TIF_NEED_RESCHED
> > -       bnez    t0, 1f
> > -       nop
> > -       nop
> > -       nop
> > +       /* start of idle interrupt region */
> > +       ori             t0, zero, CSR_CRMD_IE
> > +       /* idle instruction needs irq enabled */
> > +       csrxchg t0, t0, LOONGARCH_CSR_CRMD
> > +       /*
> > +        * If an interrupt lands here; between enabling interrupts above and
> > +        * going idle on the next instruction, we must *NOT* go idle since the
> > +        * interrupt could have set TIF_NEED_RESCHED or caused an timer to need
> > +        * reprogramming. Fall through -- see handle_vint() below -- and have
> > +        * the idle loop take care of things.
> > +        */
> >         idle    0
> > -       /* end of rollback region */
> > -1:     jr      ra
> > +       /* end of idle interrupt region */
> > +SYM_INNER_LABEL(__arch_cpu_idle_exit, SYM_L_LOCAL)
> > +       jr      ra
> >  SYM_FUNC_END(__arch_cpu_idle)
> >
> >  SYM_CODE_START(handle_vint)
> >         UNWIND_HINT_UNDEFINED
> >         BACKUP_T0T1
> >         SAVE_ALL
> > -       la_abs  t1, __arch_cpu_idle
> > +       la_abs  t1, __arch_cpu_idle_exit
> >         LONG_L  t0, sp, PT_ERA
> > -       /* 32 byte rollback region */
> > -       ori     t0, t0, 0x1f
> > -       xori    t0, t0, 0x1f
> > +       /* 3 instructions idle interrupt region */
> > +       ori     t0, t0, 0x0c
> >         bne     t0, t1, 1f
> >         LONG_S  t0, sp, PT_ERA
> >  1:     move    a0, sp
> > diff --git a/arch/loongarch/kernel/idle.c b/arch/loongarch/kernel/idle.c
> > index 0b5dd2faeb90..54b247d8cdb6 100644
> > --- a/arch/loongarch/kernel/idle.c
> > +++ b/arch/loongarch/kernel/idle.c
> > @@ -11,7 +11,6 @@
> >
> >  void __cpuidle arch_cpu_idle(void)
> >  {
> > -       raw_local_irq_enable();
> > -       __arch_cpu_idle(); /* idle instruction needs irq enabled */
> > +       __arch_cpu_idle();
> >         raw_local_irq_disable();
> >  }
> > diff --git a/arch/loongarch/kernel/reset.c b/arch/loongarch/kernel/reset.c
> > index 1ef8c6383535..8fd8c44b02cb 100644
> > --- a/arch/loongarch/kernel/reset.c
> > +++ b/arch/loongarch/kernel/reset.c
> > @@ -32,9 +32,9 @@ void machine_halt(void)
> >         pr_notice("\n\n** You can safely turn off the power now **\n\n");
> >         console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> >
> > -       while (true) {
> > -               __arch_cpu_idle();
> > -       }
> > +       while (1) {
> > +               asm volatile("idle 0" : : : "memory");
> > +       };
> >  }
> >
> >  void machine_power_off(void)
> > @@ -52,9 +52,9 @@ void machine_power_off(void)
> >         efi.reset_system(EFI_RESET_SHUTDOWN, EFI_SUCCESS, 0, NULL);
> >  #endif
> >
> > -       while (true) {
> > -               __arch_cpu_idle();
> > -       }
> > +       while (1) {
> > +               asm volatile("idle 0" : : : "memory");
> > +       };
> >  }
> >
> >  void machine_restart(char *command)
> > @@ -73,7 +73,7 @@ void machine_restart(char *command)
> >         if (!acpi_disabled)
> >                 acpi_reboot();
> >
> > -       while (true) {
> > -               __arch_cpu_idle();
> > -       }
> > +       while (1) {
> > +               asm volatile("idle 0" : : : "memory");
> > +       };
> >  }
> > --
> > 2.48.1
> >



-- 

Marco Crivellari

L3 Support Engineer