On Wed, 29 Jan 2014, Olof Johansson wrote:

Hi,

On Wed, Jan 29, 2014 at 9:45 AM, Nicolas Pitre [off-list ref] wrote:

quoted

The core idle loop now takes care of it.

Signed-off-by: Nicolas Pitre <redacted>
Acked-by: Daniel Lezcano <redacted>
---
 arch/x86/kernel/process.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 3fb8d95ab8..4505e2a950 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c

@@ -298,10 +298,7 @@ void arch_cpu_idle_dead(void)
  */
 void arch_cpu_idle(void)
 {
-       if (cpuidle_idle_call())
-               x86_idle();
-       else
-               local_irq_enable();
+       x86_idle();

You're taking out the local_irq_enable() here but I don't see the
equivalent of adding it back in the 1/6 patch that moves the
cpuidle_idle_call() up to common code. It seems that one of the call
paths through cpuidle_idle_call() don't re-enable it on its own.

When cpuidle_idle_call() returns non-zero, IRQs are left disabled.  When 
it returns zero then IRQs should be disabled.  Same goes for cpuidle 
drivers.  That's the theory at least.

Looking into some cpuidle drivers for x86 I found at least one that 
doesn't respect this convention.  Damn.

Even if this is the right thing to do, why it's OK to do so should
probably be documented in the patch description.

Better yet, I'm going to amend patch 1/6 with the below to make things 
more reliable while still identifying misbehaving drivers.

diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c
index ffcd3ee9af..14ca43430a 100644
--- a/kernel/cpu/idle.c
+++ b/kernel/cpu/idle.c

@@ -98,7 +98,8 @@ static void cpu_idle_loop(void)
 					rcu_idle_enter();
 					if (cpuidle_idle_call())
 						arch_cpu_idle();
-					WARN_ON_ONCE(irqs_disabled());
+					if (WARN_ON_ONCE(irqs_disabled()))
+						local_irq_enable();
 					rcu_idle_exit();
 					start_critical_timings();
 				} else {