[PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL

[PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL

[Thomas Gleixner]

pm_idle_save resp. pm_idle_old can be NULL when the restore code in 
acpi_processor_cst_has_changed() resp. cpuidle_uninstall_idle_handler()
is called. This can set pm_idle unconditinally to NULL, which causes the
kernel to panic when calling pm_idle in the x86 idle code. This was
covered by an extra check for !pm_idle in the x86 idle code, which was
removed during the x86 idle code refactoring.

Instead of restoring the pm_idle check in the x86 code prevent the
acpi/cpuidle code to set pm_idle to NULL.

Reported by: Dhaval Giani http://lkml.org/lkml/2008/7/2/309
Based on a debug patch from Ingo Molnar

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

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index b7f2963..283c08f 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1332,9 +1332,15 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	if (!pr->flags.power_setup_done)
 		return -ENODEV;
 
-	/* Fall back to the default idle loop */
-	pm_idle = pm_idle_save;
-	synchronize_sched();	/* Relies on interrupts forcing exit from idle. */
+	/*
+	 * Fall back to the default idle loop, when pm_idle_save had
+	 * been initialized.
+	 */
+	if (pm_idle_save) {
+		pm_idle = pm_idle_save;
+		/* Relies on interrupts forcing exit from idle. */
+		synchronize_sched();
+	}
 
 	pr->flags.power = 0;
 	result = acpi_processor_get_power_info(pr);
@@ -1896,7 +1902,8 @@ int acpi_processor_power_exit(struct acpi_processor *pr,
 
 	/* Unregister the idle handler when processor #0 is removed. */
 	if (pr->id == 0) {
-		pm_idle = pm_idle_save;
+		if (pm_idle_save)
+			pm_idle = pm_idle_save;
 
 		/*
 		 * We are about to unload the current idle thread pm callback
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 5405769..5ce07b5 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -94,7 +94,7 @@ void cpuidle_install_idle_handler(void)
  */
 void cpuidle_uninstall_idle_handler(void)
 {
-	if (enabled_devices && (pm_idle != pm_idle_old)) {
+	if (enabled_devices && pm_idle_old && (pm_idle != pm_idle_old)) {
 		pm_idle = pm_idle_old;
 		cpuidle_kick_cpus();
 	}

Re: [PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL

[Andi Kleen]

> +	/*
> +	 * Fall back to the default idle loop, when pm_idle_save had
> +	 * been initialized.
> +	 */
> +	if (pm_idle_save) {
> +		pm_idle = pm_idle_save;
> +		/* Relies on interrupts forcing exit from idle. */
> +		synchronize_sched();
> +	}

I think it would be better to fall  back to default_idle (which
might need to be exported) when the old pointer is NULL. Now with your patch 
the cpuidle idle code would run with inconsistent state for some time,
which is probably not good.

-Andi

Re: [PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL

[Thomas Gleixner]

On Mon, 28 Jul 2008, Andi Kleen wrote:
> > +	/*
> > +	 * Fall back to the default idle loop, when pm_idle_save had
> > +	 * been initialized.
> > +	 */
> > +	if (pm_idle_save) {
> > +		pm_idle = pm_idle_save;
> > +		/* Relies on interrupts forcing exit from idle. */
> > +		synchronize_sched();
> > +	}
> 
> I think it would be better to fall  back to default_idle (which
> might need to be exported) when the old pointer is NULL.

No, falling back to default_idle is wrong as hell. When pm_idle was
set to mwait_idle or whatever then you override pm_idle to
default_idle for no good reason.

The problem here is that the acpi/cpuidle code can be in a state where
the _save/old variables _ARE_ NULL because they had not been
initialized with the original pm_idle before the module is removed or
the cst state changes. So all we have to do is to prevent pm_idle to
be set to NULL.

> Now with your patch 
> the cpuidle idle code would run with inconsistent state for some time,
> which is probably not good.

Err, this happens when the original pm_idle pointer _is_ restored. But
if the _save/old pointer is NULL we crash the system and that's what
my patch prevents.

So nothing runs with an inconsistent state. If pm_idle_save contains
the original pm_idle value we restore, if it is NULL we do not touch
pm_idle and keep the original value.

Thanks,

	tglx

Re: [PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL

[Andi Kleen]

> The problem here is that the acpi/cpuidle code can be in a state where
> the _save/old variables _ARE_ NULL because they had not been
> initialized with the original pm_idle before the module is removed or
> the cst state changes. So all we have to do is to prevent pm_idle to
> be set to NULL.

It still seems wrong to me to fall back to the cpuidle idle function
instead of the earlier idle function just because cpuidle was loaded
in a weird way. 

But yes mwait_idle could be set up later after the saving state.
I suggested default_idle because it is safe, but yes it is probably
not the optimal choice.

Perhaps to solve this cleanly we really need to go to a hierarchy
of idle functions registered with priority instead of this fragile
pointer saving/restoring. 

-Andi

Re: [PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL

[Thomas Gleixner]

On Mon, 28 Jul 2008, Andi Kleen wrote:

> > The problem here is that the acpi/cpuidle code can be in a state where
> > the _save/old variables _ARE_ NULL because they had not been
> > initialized with the original pm_idle before the module is removed or
> > the cst state changes. So all we have to do is to prevent pm_idle to
> > be set to NULL.
> 
> It still seems wrong to me to fall back to the cpuidle idle function
> instead of the earlier idle function just because cpuidle was loaded
> in a weird way. 

WTF are you talking about ? 

pm_idle_save is initialized with pm_idle (the original idle function
selected by the arch code).

When CST changes or the acpi/cpuidle modules are removed we need to
restore the original pm_idle function from pm_idle_save.

When pm_idle_save was not initialized, which can happen, then we wrote
NULL to pm_idle, which is obviously wrong. We simply want to avoid
that we write NULL into pm_idle.

That's all what the patch does. Nothing else. 

This problem was hidden by the magic

   if (!pm_idle)
      pm_idle = default_idle;

in the arch/x86 code which was removed when I refactored the pm_idle
initialization code.

Just get it. That "if (!pm_idle)" check in the arch code was just
papering over the fact that the acpi code set pm_idle to NULL under
certain conditions.

Thanks,

	tglx

RE: [PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Andi Kleen [mailto:andi@firstfloor.org]
>Sent: Monday, July 28, 2008 12:53 PM
>To: Thomas Gleixner
>Cc: Andi Kleen; Linus Torvalds; Andrew Morton; LKML; Ingo
>Molnar; Dhaval Giani; Pallipadi, Venkatesh; Len Brown
>Subject: Re: [PATCH] ACPI/CPUIDLE: prevent setting pm_idle to NULL
>
>> The problem here is that the acpi/cpuidle code can be in a
>state where
>> the _save/old variables _ARE_ NULL because they had not been
>> initialized with the original pm_idle before the module is removed or
>> the cst state changes. So all we have to do is to prevent pm_idle to
>> be set to NULL.
>
>It still seems wrong to me to fall back to the cpuidle idle function
>instead of the earlier idle function just because cpuidle was loaded
>in a weird way.
>

CPUIDLE code does not have any problem here, as it always saves the
pm_idle from initcall, which should be set to right idle, default or mwait.
It is just a safety check getting added.

Real problem reported here is in acpi processor_idle without CPUIDLE, and
in a case where there are no real ACPI C-states on the platform,
it was ending up marking pm_idle = NULL at run time (cpu offline/online path),
as there is no saved idle pointer.

Thanks,
Venki