[PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The following scenario is possible with the current cpuidle code and
the ACPI cpuidle driver:
(1) acpi_processor_cst_has_changed() is called,
(2) cpuidle_disable_device() is called,
(3) cpuidle_remove_state_sysfs() is called to remove the (presumably
    outdated) states info from sysfs,
(3) acpi_processor_get_power_info() is called, the first entry in the
    pr->power.states[] table is filled with zeros,
(4) acpi_processor_setup_cpuidle() is called and it doesn't fill the
    first entry in pr->power.states[],
(5) cpuidle_enable_device() is called,
(6) __cpuidle_register_device() is _not_ called, since the device has
    already been registered,
(7) Consequently, poll_idle_init() is _not_ called either,
(8) cpuidle_add_state_sysfs() is called to create the sysfs attributes
    for the new states and it uses the bogus first table entry from
    acpi_processor_get_power_info() for creating state0.

This problem will be avoided if cpuidle_enable_device() calls
poll_idle_init() for dev->registered different from zero.

Reported-by: Len Brown <len.brown@intel.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/cpuidle/cpuidle.c |   80 +++++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 39 deletions(-)

Index: linux-2.6/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-2.6.orig/drivers/cpuidle/cpuidle.c
+++ linux-2.6/drivers/cpuidle/cpuidle.c
@@ -154,6 +154,45 @@ void cpuidle_resume_and_unlock(void)
 
 EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock);
 
+#ifdef CONFIG_ARCH_HAS_CPU_RELAX
+static int poll_idle(struct cpuidle_device *dev, struct cpuidle_state *st)
+{
+	ktime_t	t1, t2;
+	s64 diff;
+	int ret;
+
+	t1 = ktime_get();
+	local_irq_enable();
+	while (!need_resched())
+		cpu_relax();
+
+	t2 = ktime_get();
+	diff = ktime_to_us(ktime_sub(t2, t1));
+	if (diff > INT_MAX)
+		diff = INT_MAX;
+
+	ret = (int) diff;
+	return ret;
+}
+
+static void poll_idle_init(struct cpuidle_device *dev)
+{
+	struct cpuidle_state *state = &dev->states[0];
+
+	cpuidle_set_statedata(state, NULL);
+
+	snprintf(state->name, CPUIDLE_NAME_LEN, "C0");
+	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
+	state->exit_latency = 0;
+	state->target_residency = 0;
+	state->power_usage = -1;
+	state->flags = CPUIDLE_FLAG_POLL;
+	state->enter = poll_idle;
+}
+#else
+static void poll_idle_init(struct cpuidle_device *dev) {}
+#endif /* CONFIG_ARCH_HAS_CPU_RELAX */
+
 /**
  * cpuidle_enable_device - enables idle PM for a CPU
  * @dev: the CPU
@@ -176,6 +215,8 @@ int cpuidle_enable_device(struct cpuidle
 		ret = __cpuidle_register_device(dev);
 		if (ret)
 			return ret;
+	} else {
+		poll_idle_init(dev);
 	}
 
 	if ((ret = cpuidle_add_state_sysfs(dev)))
@@ -232,45 +273,6 @@ void cpuidle_disable_device(struct cpuid
 
 EXPORT_SYMBOL_GPL(cpuidle_disable_device);
 
-#ifdef CONFIG_ARCH_HAS_CPU_RELAX
-static int poll_idle(struct cpuidle_device *dev, struct cpuidle_state *st)
-{
-	ktime_t	t1, t2;
-	s64 diff;
-	int ret;
-
-	t1 = ktime_get();
-	local_irq_enable();
-	while (!need_resched())
-		cpu_relax();
-
-	t2 = ktime_get();
-	diff = ktime_to_us(ktime_sub(t2, t1));
-	if (diff > INT_MAX)
-		diff = INT_MAX;
-
-	ret = (int) diff;
-	return ret;
-}
-
-static void poll_idle_init(struct cpuidle_device *dev)
-{
-	struct cpuidle_state *state = &dev->states[0];
-
-	cpuidle_set_statedata(state, NULL);
-
-	snprintf(state->name, CPUIDLE_NAME_LEN, "C0");
-	snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
-	state->exit_latency = 0;
-	state->target_residency = 0;
-	state->power_usage = -1;
-	state->flags = CPUIDLE_FLAG_POLL;
-	state->enter = poll_idle;
-}
-#else
-static void poll_idle_init(struct cpuidle_device *dev) {}
-#endif /* CONFIG_ARCH_HAS_CPU_RELAX */
-
 /**
  * __cpuidle_register_device - internal register function called before register
  * and enable routines

Re: [PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[Len Brown]

works for me!

/sys/devices/system/cpu/cpu0/cpuidle/state0/desc:<null>
/sys/devices/system/cpu/cpu0/cpuidle/state0/latency:0
/sys/devices/system/cpu/cpu0/cpuidle/state0/name:<null>
/sys/devices/system/cpu/cpu0/cpuidle/state0/power:4294967295
/sys/devices/system/cpu/cpu0/cpuidle/state0/time:1715
/sys/devices/system/cpu/cpu0/cpuidle/state0/usage:8

becomes

/sys/devices/system/cpu/cpu0/cpuidle/state0/desc:CPUIDLE CORE POLL IDLE
/sys/devices/system/cpu/cpu0/cpuidle/state0/latency:0
/sys/devices/system/cpu/cpu0/cpuidle/state0/name:C0
/sys/devices/system/cpu/cpu0/cpuidle/state0/power:4294967295
/sys/devices/system/cpu/cpu0/cpuidle/state0/time:28716
/sys/devices/system/cpu/cpu0/cpuidle/state0/usage:11

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[Len Brown]

>  /**
>   * cpuidle_enable_device - enables idle PM for a CPU
>   * @dev: the CPU
> @@ -176,6 +215,8 @@ int cpuidle_enable_device(struct cpuidle
>  		ret = __cpuidle_register_device(dev);
>  		if (ret)
>  			return ret;
> +	} else {
> +		poll_idle_init(dev);
>  	}

how about calling poll_idle_init() unconditionally here
and deleting the call to it from within __cpuidle_register_device()?

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[Rafael J. Wysocki]

On Tuesday, January 11, 2011, Len Brown wrote:
> >  /**
> >   * cpuidle_enable_device - enables idle PM for a CPU
> >   * @dev: the CPU
> > @@ -176,6 +215,8 @@ int cpuidle_enable_device(struct cpuidle
> >  		ret = __cpuidle_register_device(dev);
> >  		if (ret)
> >  			return ret;
> > +	} else {
> > +		poll_idle_init(dev);
> >  	}
> 
> how about calling poll_idle_init() unconditionally here
> and deleting the call to it from within __cpuidle_register_device()?

Fine by me, as long as poll_idle_init() is called before the conditional. :-)

Thanks,
Rafael

Re: [PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[Rafael J. Wysocki]

On Tuesday, January 11, 2011, Rafael J. Wysocki wrote:
> On Tuesday, January 11, 2011, Len Brown wrote:
> > >  /**
> > >   * cpuidle_enable_device - enables idle PM for a CPU
> > >   * @dev: the CPU
> > > @@ -176,6 +215,8 @@ int cpuidle_enable_device(struct cpuidle
> > >  		ret = __cpuidle_register_device(dev);
> > >  		if (ret)
> > >  			return ret;
> > > +	} else {
> > > +		poll_idle_init(dev);
> > >  	}
> > 
> > how about calling poll_idle_init() unconditionally here
> > and deleting the call to it from within __cpuidle_register_device()?
> 
> Fine by me, as long as poll_idle_init() is called before the conditional. :-)

In fact, it even doesn't need to be called before the conditional.

So fine by me anyway.

Thanks,
Rafael

Re: [PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[Thomas Renninger]

On Tuesday 11 January 2011 01:05:53 Rafael J. Wysocki wrote:
> On Tuesday, January 11, 2011, Rafael J. Wysocki wrote:
> > On Tuesday, January 11, 2011, Len Brown wrote:
> > > >  /**
> > > >   * cpuidle_enable_device - enables idle PM for a CPU
> > > >   * @dev: the CPU
> > > > @@ -176,6 +215,8 @@ int cpuidle_enable_device(struct cpuidle
> > > >  		ret = __cpuidle_register_device(dev);
> > > >  		if (ret)
> > > >  			return ret;
> > > > +	} else {
> > > > +		poll_idle_init(dev);
> > > >  	}
> > > 
> > > how about calling poll_idle_init() unconditionally here
> > > and deleting the call to it from within __cpuidle_register_device()?
> > 
> > Fine by me, as long as poll_idle_init() is called before the conditional. :-)
> 
> In fact, it even doesn't need to be called before the conditional.
> 
> So fine by me anyway.
What exactly was broken?
Is it only sysfs values?
Looks like an uninitialized "poll" state can cause cpuidle
to not enter "poll" state when it should or enter "poll" when
it should not.
Hm, if cpuidle would try to call state[0]->enter,
it might even segfault?
Even not that many machines might be affected because most won't
implement runtime C-state changes, shouldn't this still be
submitted for stable@ kernels?

Thanks,

    Thomas

Re: [PATCH] cpuidle: Make cpuidle_enable_device() call poll_idle_init()

[Rafael J. Wysocki]

On Tuesday, January 11, 2011, Thomas Renninger wrote:
> On Tuesday 11 January 2011 01:05:53 Rafael J. Wysocki wrote:
> > On Tuesday, January 11, 2011, Rafael J. Wysocki wrote:
> > > On Tuesday, January 11, 2011, Len Brown wrote:
> > > > >  /**
> > > > >   * cpuidle_enable_device - enables idle PM for a CPU
> > > > >   * @dev: the CPU
> > > > > @@ -176,6 +215,8 @@ int cpuidle_enable_device(struct cpuidle
> > > > >  		ret = __cpuidle_register_device(dev);
> > > > >  		if (ret)
> > > > >  			return ret;
> > > > > +	} else {
> > > > > +		poll_idle_init(dev);
> > > > >  	}
> > > > 
> > > > how about calling poll_idle_init() unconditionally here
> > > > and deleting the call to it from within __cpuidle_register_device()?
> > > 
> > > Fine by me, as long as poll_idle_init() is called before the conditional. :-)
> > 
> > In fact, it even doesn't need to be called before the conditional.
> > 
> > So fine by me anyway.
> What exactly was broken?
> Is it only sysfs values?

Not only that, the entire state[0] was busted.

> Looks like an uninitialized "poll" state can cause cpuidle
> to not enter "poll" state when it should or enter "poll" when
> it should not.
> Hm, if cpuidle would try to call state[0]->enter,
> it might even segfault?

Yes, in theory.

> Even not that many machines might be affected because most won't
> implement runtime C-state changes, shouldn't this still be
> submitted for stable@ kernels?

I think it should.

Thanks,
Rafael