Patchwork MIPS: Enable interrupts in arch_cpu_idle()

login
register
mail settings
Submitter Thomas Gleixner
Date May 2, 2013, 2:33 p.m.
Message ID <alpine.LFD.2.02.1305021527010.3972@ionos>
Download mbox | patch
Permalink /patch/2511961/
State New, archived
Headers show

Comments

Thomas Gleixner - May 2, 2013, 2:33 p.m.
commit cdbedc61c8 (mips: Use generic idle loop) broke MIPS as I did
not realize that MIPS wants to invoke the wait instructions with
interrupts enabled. Don't ask why that works correctly; Ralf suggested
to get thoroughly drunk before even thinking about it. Looking sober
at commit c65a5480 ([MIPS] Fix potential latency problem due to
non-atomic cpu_wait) is not recommended.

Enable interrupts in arch_cpu_idle() on mips to repair the issue.

Reported-and-tested-by: Jonas Gorski <jogo@openwrt.org>
Reported-by: EunBong Song <eunb.song@samsung.com>
Booze-recommended-by: Ralf Baechle <ralf@linux-mips.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Ralf Baechle - May 2, 2013, 2:58 p.m.
On Thu, May 02, 2013 at 04:33:52PM +0200, Thomas Gleixner wrote:

> commit cdbedc61c8 (mips: Use generic idle loop) broke MIPS as I did
> not realize that MIPS wants to invoke the wait instructions with
> interrupts enabled. Don't ask why that works correctly; Ralf suggested
> to get thoroughly drunk before even thinking about it. Looking sober
> at commit c65a5480 ([MIPS] Fix potential latency problem due to
> non-atomic cpu_wait) is not recommended.
> 
> Enable interrupts in arch_cpu_idle() on mips to repair the issue.
> 
> Reported-and-tested-by: Jonas Gorski <jogo@openwrt.org>
> Reported-by: EunBong Song <eunb.song@samsung.com>
> Booze-recommended-by: Ralf Baechle <ralf@linux-mips.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

A very sobering commit message ;-)

The underlying issue is that the WAIT instruction on MIPS is architecturally
defined as "[...] It is implementation-dependent whether the pipeline
restarts when a non-enabled interrupt is requested.  [...]"

arch_local_irq_disable() disables interrupts by clearing the IE bit in
the CPU status register, thus disabling all interrupts.  If no a WAIT
instruction is executed it's legal for a CPU to stop the pipeline for
good.  Which obviously is pretty stupidtastik behaviour.

For a while we just used to live with the race condition resulting from
not disabling interrupts in the idle loop.  Then c65a5480 fixed this by
checking if we're returning to the WAIT instruction in the idle loop
when returning from an interrupt and iff so, rolling back the
program counter to point to the if (test_thread_flag(TIF_NEED_RESCHED))
test at the beginning of rollback_r4k_wait.

Linux knows which CPUs have the problematic behaviour and uses this special
variant of the idle loop only where needed.

  Ralf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Linus Torvalds - May 2, 2013, 4:45 p.m.
On Thu, May 2, 2013 at 7:58 AM, Ralf Baechle <ralf@linux-mips.org> wrote:
>
> For a while we just used to live with the race condition resulting from
> not disabling interrupts in the idle loop.  Then c65a5480 fixed this by
> checking if we're returning to the WAIT instruction in the idle loop
> when returning from an interrupt and iff so, rolling back the
> program counter to point to the if (test_thread_flag(TIF_NEED_RESCHED))
> test at the beginning of rollback_r4k_wait.

Umm. That sounds buggy. What if there was an interrupt *between* the
two places, not right at the wait instruction?

It seriously sounds like MIPS should do this by enabling interrupts in
the *assembly* code immediately preceding the wait instruction. IOW,
you'd effectively have the same kind of "sti; halt" kind of sequence
that x86 has. Not "enable interrupts" + C compiler puts random
instructions here + "wait".

                       Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

Index: linux-2.6/arch/mips/kernel/process.c
===================================================================
--- linux-2.6.orig/arch/mips/kernel/process.c
+++ linux-2.6/arch/mips/kernel/process.c
@@ -50,13 +50,18 @@  void arch_cpu_idle_dead(void)
 }
 #endif
 
-void arch_cpu_idle(void)
+static void smtc_idle_hook(void)
 {
 #ifdef CONFIG_MIPS_MT_SMTC
 	extern void smtc_idle_loop_hook(void);
-
 	smtc_idle_loop_hook();
 #endif
+}
+
+void arch_cpu_idle(void)
+{
+	local_irq_enable();
+	smtc_idle_hook();
 	if (cpu_wait)
 		(*cpu_wait)();
 	else