idle-test patches queued for upstream

idle-test patches queued for upstream

[Len Brown]

Please look over and test this patch set.
(If you test linux-next, you already have it)

There are a few simple patches, leading up to a new intel_idle driver.

Note that you can get the patch series as a single patch here:
http://ftp.kernel.org/pub/linux/kernel/people/lenb/idle/patches/2.6.34/idle-test-2.6.34.diff.gz

or pull from this git branch
git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-idle-2.6.git idle-test

Both are vs 2.6.34.

Why is it good to have a native intel_idle driver?

Basically, we think we can do better than ACPI.
Indeed, on my (production level commerically available) Nehalem desktop
the ACPI tables are broken and an ACPI OS idles at 100W.  With this
driver the box idles at 85W.

Thanks,
-Len

[PATCH 1/8] cpuidle: fail to register if !CONFIG_CPU_IDLE

[Len Brown]

From: Len Brown <len.brown@intel.com>

Signed-off-by: Len Brown <len.brown@intel.com>
---
 include/linux/cpuidle.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index dcf77fa..0fe1efe 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -137,16 +137,16 @@ extern void cpuidle_disable_device(struct cpuidle_device *dev);
 #else
 
 static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
-{return 0;}
+{return -ENODEV;}
 static inline void cpuidle_unregister_driver(struct cpuidle_driver *drv) { }
 static inline int cpuidle_register_device(struct cpuidle_device *dev)
-{return 0;}
+{return -ENODEV;}
 static inline void cpuidle_unregister_device(struct cpuidle_device *dev) { }
 
 static inline void cpuidle_pause_and_lock(void) { }
 static inline void cpuidle_resume_and_unlock(void) { }
 static inline int cpuidle_enable_device(struct cpuidle_device *dev)
-{return 0;}
+{return -ENODEV;}
 static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
 
 #endif
-- 
1.6.0.6

[PATCH 2/8] cpuidle: add cpuidle_unregister_driver() error check

[Len Brown]

From: Len Brown <len.brown@intel.com>

When cpuidle_unregister_driver() is called with a driver
other than the one that was successfully registered, do nothing.

Previously we'd NULL-out the one that was registered.
But that required the callers to remember what this
routine already remembers.  With this check, the callers
can be simplified.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/cpuidle/driver.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 2257004..30bcd44 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -49,7 +49,8 @@ void cpuidle_unregister_driver(struct cpuidle_driver *drv)
 		return;
 
 	spin_lock(&cpuidle_driver_lock);
-	cpuidle_curr_driver = NULL;
+	if (drv == cpuidle_curr_driver)
+		cpuidle_curr_driver = NULL;
 	spin_unlock(&cpuidle_driver_lock);
 }
 
-- 
1.6.0.6

Re: [PATCH 2/8] cpuidle: add cpuidle_unregister_driver() error check

[Andrew Morton]

On Wed, 26 May 2010 22:42:25 -0400 Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> When cpuidle_unregister_driver() is called with a driver
> other than the one that was successfully registered, do nothing.
> 
> Previously we'd NULL-out the one that was registered.
> But that required the callers to remember what this
> routine already remembers.  With this check, the callers
> can be simplified.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/cpuidle/driver.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 2257004..30bcd44 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -49,7 +49,8 @@ void cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  		return;
>  
>  	spin_lock(&cpuidle_driver_lock);
> -	cpuidle_curr_driver = NULL;
> +	if (drv == cpuidle_curr_driver)
> +		cpuidle_curr_driver = NULL;
>  	spin_unlock(&cpuidle_driver_lock);
>  }

This can only happen as a result of a coding bug, yes?  If so, the
kernel should go BUG() rather than secretly concealing the problem.

Also (alternatively), the `drv' arg to this function is superfluous?

[PATCH-v2 2/8] cpuidle: add cpuidle_unregister_driver() error check

[Len Brown]

From: Len Brown <len.brown@intel.com>

BUG_ON() cpuidle_unregister_driver() being called with
a driver not matching the onee that successfully registered.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/cpuidle/driver.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 2257004..5b6fd0f 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -45,8 +45,7 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
  */
 void cpuidle_unregister_driver(struct cpuidle_driver *drv)
 {
-	if (!drv)
-		return;
+	BUG_ON(drv != cpuidle_curr_driver)
 
 	spin_lock(&cpuidle_driver_lock);
 	cpuidle_curr_driver = NULL;
-- 
1.6.0.6

Re: [PATCH-v2 2/8] cpuidle: add cpuidle_unregister_driver() error check

[Andrew Morton]

On Thu, 27 May 2010 01:11:03 -0400 (EDT) Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> BUG_ON() cpuidle_unregister_driver() being called with
> a driver not matching the onee that successfully registered.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/cpuidle/driver.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 2257004..5b6fd0f 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -45,8 +45,7 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
>   */
>  void cpuidle_unregister_driver(struct cpuidle_driver *drv)
>  {
> -	if (!drv)
> -		return;
> +	BUG_ON(drv != cpuidle_curr_driver)
>  
>  	spin_lock(&cpuidle_driver_lock);
>  	cpuidle_curr_driver = NULL;

Actually, WARN_ON is nicer ;)   There's no point in killing the user's
machine over this recoverable error.

[PATCH 3/8] cpuidle: make cpuidle_curr_driver static

[Len Brown]

From: Len Brown <len.brown@intel.com>

cpuidle_register_driver() sets cpuidle_curr_driver
cpuidle_unregister_driver() clears cpuidle_curr_driver

We should't expose cpuidle_curr_driver to
potential modification except via these interfaces.
So make it static and create cpuidle_get_driver() to observe it.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/cpuidle/cpuidle.c |   12 +++++++-----
 drivers/cpuidle/cpuidle.h |    1 -
 drivers/cpuidle/driver.c  |   11 ++++++++++-
 drivers/cpuidle/sysfs.c   |    5 +++--
 include/linux/cpuidle.h   |    1 +
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 12fdd39..1994885 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -156,7 +156,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
 
 	if (dev->enabled)
 		return 0;
-	if (!cpuidle_curr_driver || !cpuidle_curr_governor)
+	if (!cpuidle_get_driver() || !cpuidle_curr_governor)
 		return -EIO;
 	if (!dev->state_count)
 		return -EINVAL;
@@ -207,7 +207,7 @@ void cpuidle_disable_device(struct cpuidle_device *dev)
 {
 	if (!dev->enabled)
 		return;
-	if (!cpuidle_curr_driver || !cpuidle_curr_governor)
+	if (!cpuidle_get_driver() || !cpuidle_curr_governor)
 		return;
 
 	dev->enabled = 0;
@@ -271,10 +271,11 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
 {
 	int ret;
 	struct sys_device *sys_dev = get_cpu_sysdev((unsigned long)dev->cpu);
+	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
 
 	if (!sys_dev)
 		return -EINVAL;
-	if (!try_module_get(cpuidle_curr_driver->owner))
+	if (!try_module_get(cpuidle_driver->owner))
 		return -EINVAL;
 
 	init_completion(&dev->kobj_unregister);
@@ -284,7 +285,7 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
 	per_cpu(cpuidle_devices, dev->cpu) = dev;
 	list_add(&dev->device_list, &cpuidle_detected_devices);
 	if ((ret = cpuidle_add_sysfs(sys_dev))) {
-		module_put(cpuidle_curr_driver->owner);
+		module_put(cpuidle_driver->owner);
 		return ret;
 	}
 
@@ -325,6 +326,7 @@ EXPORT_SYMBOL_GPL(cpuidle_register_device);
 void cpuidle_unregister_device(struct cpuidle_device *dev)
 {
 	struct sys_device *sys_dev = get_cpu_sysdev((unsigned long)dev->cpu);
+	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
 
 	if (dev->registered == 0)
 		return;
@@ -340,7 +342,7 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
 
 	cpuidle_resume_and_unlock();
 
-	module_put(cpuidle_curr_driver->owner);
+	module_put(cpuidle_driver->owner);
 }
 
 EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
index 9476ba3..33e50d5 100644
--- a/drivers/cpuidle/cpuidle.h
+++ b/drivers/cpuidle/cpuidle.h
@@ -9,7 +9,6 @@
 
 /* For internal use only */
 extern struct cpuidle_governor *cpuidle_curr_governor;
-extern struct cpuidle_driver *cpuidle_curr_driver;
 extern struct list_head cpuidle_governors;
 extern struct list_head cpuidle_detected_devices;
 extern struct mutex cpuidle_lock;
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 30bcd44..b381d29 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -14,7 +14,7 @@
 
 #include "cpuidle.h"
 
-struct cpuidle_driver *cpuidle_curr_driver;
+static struct cpuidle_driver *cpuidle_curr_driver;
 DEFINE_SPINLOCK(cpuidle_driver_lock);
 
 /**
@@ -40,6 +40,15 @@ int cpuidle_register_driver(struct cpuidle_driver *drv)
 EXPORT_SYMBOL_GPL(cpuidle_register_driver);
 
 /**
+ * cpuidle_get_driver - return the current driver
+ */
+struct cpuidle_driver *cpuidle_get_driver(void)
+{
+	return cpuidle_curr_driver;
+}
+EXPORT_SYMBOL_GPL(cpuidle_get_driver);
+
+/**
  * cpuidle_unregister_driver - unregisters a driver
  * @drv: the driver
  */
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 0ba9c8b..0310ffa 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -47,10 +47,11 @@ static ssize_t show_current_driver(struct sysdev_class *class,
 				   char *buf)
 {
 	ssize_t ret;
+	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
 
 	spin_lock(&cpuidle_driver_lock);
-	if (cpuidle_curr_driver)
-		ret = sprintf(buf, "%s\n", cpuidle_curr_driver->name);
+	if (cpuidle_driver)
+		ret = sprintf(buf, "%s\n", cpuidle_driver->name);
 	else
 		ret = sprintf(buf, "none\n");
 	spin_unlock(&cpuidle_driver_lock);
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 0fe1efe..5f539cf 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -125,6 +125,7 @@ struct cpuidle_driver {
 #ifdef CONFIG_CPU_IDLE
 
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
+struct cpuidle_driver *cpuidle_get_driver(void);
 extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
 extern int cpuidle_register_device(struct cpuidle_device *dev);
 extern void cpuidle_unregister_device(struct cpuidle_device *dev);
-- 
1.6.0.6

[PATCH-v2 3/8] cpuidle: make cpuidle_curr_driver static

[Len Brown]

From: Len Brown <len.brown@intel.com>

cpuidle_register_driver() sets cpuidle_curr_driver
cpuidle_unregister_driver() clears cpuidle_curr_driver

We should't expose cpuidle_curr_driver to
potential modification except via these interfaces.
So make it static and create cpuidle_get_driver() to observe it.

Signed-off-by: Len Brown <len.brown@intel.com>
---
Tony noticed that since IA64 builds with CONFIG_CPU_IDLE=n
that we need an inline for the new cpuidle_get_driver() routine
to make the ACPI driver build happy.

 drivers/cpuidle/cpuidle.c |   12 +++++++-----
 drivers/cpuidle/cpuidle.h |    1 -
 drivers/cpuidle/driver.c  |   11 ++++++++++-
 drivers/cpuidle/sysfs.c   |    5 +++--
 include/linux/cpuidle.h   |    2 ++
 5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 12fdd39..1994885 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -156,7 +156,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
 
 	if (dev->enabled)
 		return 0;
-	if (!cpuidle_curr_driver || !cpuidle_curr_governor)
+	if (!cpuidle_get_driver() || !cpuidle_curr_governor)
 		return -EIO;
 	if (!dev->state_count)
 		return -EINVAL;
@@ -207,7 +207,7 @@ void cpuidle_disable_device(struct cpuidle_device *dev)
 {
 	if (!dev->enabled)
 		return;
-	if (!cpuidle_curr_driver || !cpuidle_curr_governor)
+	if (!cpuidle_get_driver() || !cpuidle_curr_governor)
 		return;
 
 	dev->enabled = 0;
@@ -271,10 +271,11 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
 {
 	int ret;
 	struct sys_device *sys_dev = get_cpu_sysdev((unsigned long)dev->cpu);
+	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
 
 	if (!sys_dev)
 		return -EINVAL;
-	if (!try_module_get(cpuidle_curr_driver->owner))
+	if (!try_module_get(cpuidle_driver->owner))
 		return -EINVAL;
 
 	init_completion(&dev->kobj_unregister);
@@ -284,7 +285,7 @@ static int __cpuidle_register_device(struct cpuidle_device *dev)
 	per_cpu(cpuidle_devices, dev->cpu) = dev;
 	list_add(&dev->device_list, &cpuidle_detected_devices);
 	if ((ret = cpuidle_add_sysfs(sys_dev))) {
-		module_put(cpuidle_curr_driver->owner);
+		module_put(cpuidle_driver->owner);
 		return ret;
 	}
 
@@ -325,6 +326,7 @@ EXPORT_SYMBOL_GPL(cpuidle_register_device);
 void cpuidle_unregister_device(struct cpuidle_device *dev)
 {
 	struct sys_device *sys_dev = get_cpu_sysdev((unsigned long)dev->cpu);
+	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
 
 	if (dev->registered == 0)
 		return;
@@ -340,7 +342,7 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
 
 	cpuidle_resume_and_unlock();
 
-	module_put(cpuidle_curr_driver->owner);
+	module_put(cpuidle_driver->owner);
 }
 
 EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h
index 9476ba3..33e50d5 100644
--- a/drivers/cpuidle/cpuidle.h
+++ b/drivers/cpuidle/cpuidle.h
@@ -9,7 +9,6 @@
 
 /* For internal use only */
 extern struct cpuidle_governor *cpuidle_curr_governor;
-extern struct cpuidle_driver *cpuidle_curr_driver;
 extern struct list_head cpuidle_governors;
 extern struct list_head cpuidle_detected_devices;
 extern struct mutex cpuidle_lock;
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index 5b6fd0f..4a68d40 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -14,7 +14,7 @@
 
 #include "cpuidle.h"
 
-struct cpuidle_driver *cpuidle_curr_driver;
+static struct cpuidle_driver *cpuidle_curr_driver;
 DEFINE_SPINLOCK(cpuidle_driver_lock);
 
 /**
@@ -40,6 +40,15 @@ int cpuidle_register_driver(struct cpuidle_driver *drv)
 EXPORT_SYMBOL_GPL(cpuidle_register_driver);
 
 /**
+ * cpuidle_get_driver - return the current driver
+ */
+struct cpuidle_driver *cpuidle_get_driver(void)
+{
+	return cpuidle_curr_driver;
+}
+EXPORT_SYMBOL_GPL(cpuidle_get_driver);
+
+/**
  * cpuidle_unregister_driver - unregisters a driver
  * @drv: the driver
  */
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 0ba9c8b..0310ffa 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -47,10 +47,11 @@ static ssize_t show_current_driver(struct sysdev_class *class,
 				   char *buf)
 {
 	ssize_t ret;
+	struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
 
 	spin_lock(&cpuidle_driver_lock);
-	if (cpuidle_curr_driver)
-		ret = sprintf(buf, "%s\n", cpuidle_curr_driver->name);
+	if (cpuidle_driver)
+		ret = sprintf(buf, "%s\n", cpuidle_driver->name);
 	else
 		ret = sprintf(buf, "none\n");
 	spin_unlock(&cpuidle_driver_lock);
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 0fe1efe..ee27a1f 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -125,6 +125,7 @@ struct cpuidle_driver {
 #ifdef CONFIG_CPU_IDLE
 
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
+struct cpuidle_driver *cpuidle_get_driver(void);
 extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
 extern int cpuidle_register_device(struct cpuidle_device *dev);
 extern void cpuidle_unregister_device(struct cpuidle_device *dev);
@@ -138,6 +139,7 @@ extern void cpuidle_disable_device(struct cpuidle_device *dev);
 
 static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
 {return -ENODEV;}
+static inline cpuidle_driver *cpuidle_get_driver(void) {return NULL;}
 static inline void cpuidle_unregister_driver(struct cpuidle_driver *drv) { }
 static inline int cpuidle_register_device(struct cpuidle_device *dev)
 {return -ENODEV;}
-- 
1.6.0.6

RE: [PATCH-v2 3/8] cpuidle: make cpuidle_curr_driver static

[Luck, Tony]

+static inline cpuidle_driver *cpuidle_get_driver(void) {return NULL;}

Close, but no cigar yet. You missed a "struct" keyword on this line.
Add that, and it builds and boots for me.

-Tony

RE: [PATCH-v2 3/8] cpuidle: make cpuidle_curr_driver static

[Len Brown]

> ...it builds and boots for me.

Thanks Tony, what would I do without you:-)

[PATCH 4/8] ACPI: allow a native cpuidle driver to displace ACPI

[Len Brown]

From: Len Brown <len.brown@intel.com>

The ACPI driver would fail probe when it found that
another driver had previously registered with cpuidle.

But this is a natural situation, as a native hardware
cpuidle driver should be able to bind instead of ACPI,
and the ACPI processor driver should be able to handle
yielding control of C-states while still handling
P-states and T-states.

Add a KERN_DEBUG line showing when acpi_idle
does successfully register.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/processor_driver.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 5675d97..deefa85 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -616,7 +616,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
 	acpi_processor_get_limit_info(pr);
 
 
-	acpi_processor_power_init(pr, device);
+	if (cpuidle_get_driver() == &acpi_idle_driver)
+		acpi_processor_power_init(pr, device);
 
 	pr->cdev = thermal_cooling_device_register("Processor", device,
 						&processor_cooling_ops);
@@ -920,9 +921,10 @@ static int __init acpi_processor_init(void)
 	if (!acpi_processor_dir)
 		return -ENOMEM;
 #endif
-	result = cpuidle_register_driver(&acpi_idle_driver);
-	if (result < 0)
-		goto out_proc;
+
+	if (!cpuidle_register_driver(&acpi_idle_driver))
+		printk(KERN_DEBUG "ACPI: %s registered with cpuidle\n",
+			acpi_idle_driver.name);
 
 	result = acpi_bus_register_driver(&acpi_processor_driver);
 	if (result < 0)
@@ -941,7 +943,6 @@ static int __init acpi_processor_init(void)
 out_cpuidle:
 	cpuidle_unregister_driver(&acpi_idle_driver);
 
-out_proc:
 #ifdef CONFIG_ACPI_PROCFS
 	remove_proc_entry(ACPI_PROCESSOR_CLASS, acpi_root_dir);
 #endif
-- 
1.6.0.6

[PATCH 5/8] sched: clarify commment for TS_POLLING

[Len Brown]

From: Len Brown <len.brown@intel.com>

TS_POLLING set tells the scheduler a task will poll
need_resched() to look for work.

TS_POLLING clear tells resched_task() and wake_up_idle_cpu()
that the remote CPU is sleeping in idle, and thus requires
a reschedule interrupt to wake them to notice work.

Update the description of TS_POLLING to reflect how it works.
"cleared when sleeping in idle, requiring reschedule interrupt"

Signed-off-by: Len Brown <len.brown@intel.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/include/asm/thread_info.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index e0d2890..0a14da2 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -241,8 +241,8 @@ static inline struct thread_info *current_thread_info(void)
 #define TS_USEDFPU		0x0001	/* FPU was used by this task
 					   this quantum (SMP) */
 #define TS_COMPAT		0x0002	/* 32bit syscall active (64BIT)*/
-#define TS_POLLING		0x0004	/* true if in idle loop
-					   and not sleeping */
+#define TS_POLLING		0x0004	/* clear when sleeping in idle
+					   requiring reschedule interrupt */
 #define TS_RESTORE_SIGMASK	0x0008	/* restore signal mask in do_signal() */
 #define TS_XSAVE		0x0010	/* Use xsave/xrstor */
 
-- 
1.6.0.6

[PATCH-v2 5/8] sched: clarify commment for TS_POLLING

[Len Brown]

From: Len Brown <len.brown@intel.com>


TS_POLLING set tells the scheduler an idle_task will poll
need_resched() to look for work.

TS_POLLING clear tells resched_task() and wake_up_idle_cpu()
that the remote CPU's idle_task is now sleeping in idle,
and thus requires a reschedule interrupt notice work.

Update the description of TS_POLLING to reflect how it works.
"idle task polling need_resched, skip sending interrupt"

Wordsmithing-by: Milton Miller <miltonm@bga.com>
Signed-off-by: Len Brown <len.brown@intel.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/include/asm/thread_info.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index e0d2890..8121869 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -241,8 +241,8 @@ static inline struct thread_info *current_thread_info(void)
 #define TS_USEDFPU		0x0001	/* FPU was used by this task
 					   this quantum (SMP) */
 #define TS_COMPAT		0x0002	/* 32bit syscall active (64BIT)*/
-#define TS_POLLING		0x0004	/* true if in idle loop
-					   and not sleeping */
+#define TS_POLLING		0x0004	/* idle task polling need_resched,
+					   skip sending interrupt */
 #define TS_RESTORE_SIGMASK	0x0008	/* restore signal mask in do_signal() */
 #define TS_XSAVE		0x0010	/* Use xsave/xrstor */
 
-- 
1.6.0.6

[PATCH 6/8] acpi_pad: uses MONITOR/MWAIT, so it doesn't need to clear TS_POLLING

[Len Brown]

From: Len Brown <len.brown@intel.com>

api_pad exclusively uses MONITOR/MWAIT to sleep in idle,
so it does not need the wakeup IPI during idle sleep
that is provoked by clearing TS_POLLING.

Signed-off-by: Len Brown <len.brown@intel.com>
Cc: Shaohua Li <shaohua.li@intel.com>
---
 drivers/acpi/acpi_pad.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c
index 6212213..7edf053 100644
--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -168,13 +168,6 @@ static int power_saving_thread(void *data)
 
 		do_sleep = 0;
 
-		current_thread_info()->status &= ~TS_POLLING;
-		/*
-		 * TS_POLLING-cleared state must be visible before we test
-		 * NEED_RESCHED:
-		 */
-		smp_mb();
-
 		expire_time = jiffies + HZ * (100 - idle_pct) / 100;
 
 		while (!need_resched()) {
@@ -200,8 +193,6 @@ static int power_saving_thread(void *data)
 			}
 		}
 
-		current_thread_info()->status |= TS_POLLING;
-
 		/*
 		 * current sched_rt has threshold for rt task running time.
 		 * When a rt task uses 95% CPU time, the rt thread will be
-- 
1.6.0.6

[PATCH 7/8] ACPI: acpi_idle: touch TS_POLLING only in the non-MWAIT case

[Len Brown]

From: Len Brown <len.brown@intel.com>

commit d306ebc28649b89877a22158fe0076f06cc46f60
(ACPI: Be in TS_POLLING state during mwait based C-state entry)
fixed an important power & performance issue where ACPI c2 and c3 C-states
were clearing TS_POLLING even when using MWAIT (ACPI_STATE_FFH).
That bug had been causing us to receive redundant scheduling interrups
when we had already been woken up by MONITOR/MWAIT.

Following up on that...

In the MWAIT case, we don't have to subsequently
check need_resched(), as that c heck was there
for the TS_POLLING-clearing case.

Note that not only does the cpuidle calling function
already check need_resched() before calling us, the
low-level entry into monitor/mwait calls it twice --
guaranteeing that a write to the trigger address
can not go un-noticed.

Also, in this case, we don't have to set TS_POLLING
when we wake, because we never cleared it.

Signed-off-by: Len Brown <len.brown@intel.com>
Cc: Venkatesh Pallipadi <venki@google.com>
---
 drivers/acpi/processor_idle.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 5939e7f..a4166e2 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -881,6 +881,7 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 		return(acpi_idle_enter_c1(dev, state));
 
 	local_irq_disable();
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -888,12 +889,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 		 * NEED_RESCHED:
 		 */
 		smp_mb();
-	}
 
-	if (unlikely(need_resched())) {
-		current_thread_info()->status |= TS_POLLING;
-		local_irq_enable();
-		return 0;
+		if (unlikely(need_resched())) {
+			current_thread_info()->status |= TS_POLLING;
+			local_irq_enable();
+			return 0;
+		}
 	}
 
 	/*
@@ -918,7 +919,8 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 
 	local_irq_enable();
-	current_thread_info()->status |= TS_POLLING;
+	if (cx->entry_method != ACPI_CSTATE_FFH)
+		current_thread_info()->status |= TS_POLLING;
 
 	cx->usage++;
 
@@ -968,6 +970,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	}
 
 	local_irq_disable();
+
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -975,12 +978,12 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		 * NEED_RESCHED:
 		 */
 		smp_mb();
-	}
 
-	if (unlikely(need_resched())) {
-		current_thread_info()->status |= TS_POLLING;
-		local_irq_enable();
-		return 0;
+		if (unlikely(need_resched())) {
+			current_thread_info()->status |= TS_POLLING;
+			local_irq_enable();
+			return 0;
+		}
 	}
 
 	acpi_unlazy_tlb(smp_processor_id());
@@ -1032,7 +1035,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 
 	local_irq_enable();
-	current_thread_info()->status |= TS_POLLING;
+	if (cx->entry_method != ACPI_CSTATE_FFH)
+		current_thread_info()->status |= TS_POLLING;
 
 	cx->usage++;
 
-- 
1.6.0.6

[PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Len Brown]

From: Len Brown <len.brown@intel.com>

This EXPERIMENTAL driver supersedes acpi_idle
on modern Intel processors. (Nehalem and Atom Processors).

For CONFIG_INTEL_IDLE=y, intel_idle probes before acpi_idle,
no matter if CONFIG_ACPI_PROCESSOR=y or =m.

Boot with "intel_idle.max_cstate=0" to disable the driver
and to fall back on ACPI.

CONFIG_INTEL_IDLE=m is not recommended unless the system
has a method to guarantee intel_idle loads before ACPI's
processor_idle.

This driver does not yet know about cpu online/offline
and thus will not yet play well with cpu-hotplug.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 MAINTAINERS                     |    7 +
 drivers/Makefile                |    2 +-
 drivers/acpi/processor_driver.c |    6 +-
 drivers/idle/Kconfig            |   11 +
 drivers/idle/Makefile           |    1 +
 drivers/idle/intel_idle.c       |  446 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 471 insertions(+), 2 deletions(-)
 create mode 100755 drivers/idle/intel_idle.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d329b05..276e79b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2850,6 +2850,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
 S:	Maintained
 F:	drivers/input/
 
+INTEL IDLE DRIVER
+M:	Len Brown <lenb@kernel.org>
+L:	linux-pm@lists.linux-foundation.org
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-idle-2.6.git
+S:	Supported
+F:	drivers/idle/intel_idle.c
+
 INTEL FRAMEBUFFER DRIVER (excluding 810 and 815)
 M:	Maik Broemme <mbroemme@plusserver.de>
 L:	linux-fbdev@vger.kernel.org
diff --git a/drivers/Makefile b/drivers/Makefile
index f42a030..91874e0 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_PCI)		+= pci/
 obj-$(CONFIG_PARISC)		+= parisc/
 obj-$(CONFIG_RAPIDIO)		+= rapidio/
 obj-y				+= video/
+obj-y				+= idle/
 obj-$(CONFIG_ACPI)		+= acpi/
 obj-$(CONFIG_SFI)		+= sfi/
 # PnP must come after ACPI since it will eventually need to check if acpi
@@ -91,7 +92,6 @@ obj-$(CONFIG_EISA)		+= eisa/
 obj-y				+= lguest/
 obj-$(CONFIG_CPU_FREQ)		+= cpufreq/
 obj-$(CONFIG_CPU_IDLE)		+= cpuidle/
-obj-y				+= idle/
 obj-$(CONFIG_MMC)		+= mmc/
 obj-$(CONFIG_MEMSTICK)		+= memstick/
 obj-$(CONFIG_NEW_LEDS)		+= leds/
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index deefa85..b2a6d73 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -922,9 +922,13 @@ static int __init acpi_processor_init(void)
 		return -ENOMEM;
 #endif
 
-	if (!cpuidle_register_driver(&acpi_idle_driver))
+	if (!cpuidle_register_driver(&acpi_idle_driver)) {
 		printk(KERN_DEBUG "ACPI: %s registered with cpuidle\n",
 			acpi_idle_driver.name);
+	} else {
+                printk(KERN_DEBUG "ACPI: intel_idle yielding to %s",
+                        cpuidle_get_driver()->name);
+	}
 
 	result = acpi_bus_register_driver(&acpi_processor_driver);
 	if (result < 0)
diff --git a/drivers/idle/Kconfig b/drivers/idle/Kconfig
index f15e90a..fb5c518 100644
--- a/drivers/idle/Kconfig
+++ b/drivers/idle/Kconfig
@@ -1,3 +1,14 @@
+config INTEL_IDLE
+	tristate "Cpuidle Driver for Intel Processors"
+	depends on CPU_IDLE
+	depends on X86
+	depends on CPU_SUP_INTEL
+	depends on EXPERIMENTAL
+	help
+	  Enable intel_idle, a cpuidle driver that includes knowledge of
+	  native Intel hardware idle features.  The acpi_idle driver
+	  can be configured at the same time, in order to handle
+	  processors intel_idle does not support.
 
 menu "Memory power savings"
 depends on X86_64
diff --git a/drivers/idle/Makefile b/drivers/idle/Makefile
index 5f68fc3..23d295c 100644
--- a/drivers/idle/Makefile
+++ b/drivers/idle/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_I7300_IDLE)			+= i7300_idle.o
+obj-$(CONFIG_INTEL_IDLE)			+= intel_idle.o
 
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
new file mode 100755
index 0000000..12f31d9
--- /dev/null
+++ b/drivers/idle/intel_idle.c
@@ -0,0 +1,446 @@
+/*
+ * intel_idle.c - native hardware idle loop for modern Intel processors
+ *
+ * Copyright (c) 2010, Intel Corporation.
+ * Len Brown <len.brown@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+/*
+ * intel_idle is a cpuidle driver that loads on specific Intel processors
+ * in lieu of the legacy ACPI processor_idle driver.  The intent is to
+ * make Linux more efficient on these processors, as intel_idle knows
+ * more than ACPI, as well as make Linux more immune to ACPI BIOS bugs.
+ */
+
+/*
+ * Design Assumptions
+ *
+ * All CPUs have same idle states as boot CPU
+ *
+ * Chipset BM_STS (bus master status) bit is a NOP
+ * 	for preventing entry into deep C-stats
+ */
+
+/*
+ * Known issues
+ *
+ * The driver currently initializes for_each_online_cpu() upon modprobe.
+ * It it unaware cpu online/offline and cpui hotplug
+ *
+ * ACPI has a .suspend hack to turn off deep c-statees during suspend
+ * to avoid complications with the lapic timer workaround
+ * will need to address that situation here too.
+ *
+ * There is currently no automatic probing/loading mechanism
+ * if the driver is built as a module.
+ */
+
+/*
+ * todo test
+ * test: build & run with acpi=off
+ * test: repeated load/unload
+ * test: suspend/resume vs timer workaround
+ */
+
+/*
+ * todo cpuidle
+ * 	cpuidle should supply the counters for each driver
+ * 	since they are private to cpuidle, not the driver
+ */
+
+
+/* un-comment DEBUG to enable pr_debug() statements */
+#define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/cpuidle.h>
+#include <linux/clockchips.h>
+#include <linux/hrtimer.h>	/* ktime_get_real() */
+#include <trace/events/power.h>
+#include <linux/sched.h>
+
+#define INTEL_IDLE_VERSION "0.3"
+#define PREFIX "intel_idle: "
+
+#define MWAIT_SUBSTATE_MASK	(0xf)
+#define MWAIT_CSTATE_MASK	(0xf)
+#define MWAIT_SUBSTATE_SIZE	(4)
+#define MWAIT_MAX_NUM_CSTATES	8
+#define CPUID_MWAIT_LEAF (5)
+#define CPUID5_ECX_EXTENSIONS_SUPPORTED (0x1)
+#define CPUID5_ECX_INTERRUPT_BREAK	(0x2)
+
+#define mwait_hint_to_cstate(hint) ((((hint) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1)
+#define lapic_timer_reliable(cstate) (lapic_timer_reliable_states & (1 << (cstate)))
+
+static struct cpuidle_driver intel_idle_driver = {
+	.name = "intel_idle",
+	.owner = THIS_MODULE,
+};
+static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;	/* intel_idle.max_cstate=0 disables driver */
+static int power_policy = 7; /* 0 = max perf; 15 = max powersave */
+
+static unsigned int substates;
+#define get_num_mwait_substates(cstate) ((substates >> ((cstate) * 4)) && 0xF)
+
+/* Reliable LAPIC Timer States, bit 1 for C1 etc.  */
+static unsigned int lapic_timer_reliable_states;
+
+static struct cpuidle_device *intel_idle_cpuidle_devices;
+static int intel_idle(struct cpuidle_device *dev, struct cpuidle_state *state);
+
+/*
+ * These attributes are be visible under
+ * /sys/devices/system/cpu/cpu.../cpuidle/state.../
+ *
+ * name 
+ * 	Hardware name of the state, from datasheet
+ * desc
+ * 	MWAIT param
+ * driver_data
+ * 	token passed to intel_idle()
+ * flags
+ * 	CPUIDLE_FLAG_TIME_VALID
+ * 		we return valid times in all states
+ * 	CPUIDLE_FLAG_SHALLOW
+ * 		lapic timer keeps running
+ * exit_latency
+ * 	[usec]
+ * power_usage
+ * 	mW (TBD)
+ * target_residency
+ * 	currently we multiply exit_latency by 4
+ * 	[usec]
+ * usage
+ * 	instance counter
+ * time
+ * 	residency counter [usec]
+ */
+
+static struct cpuidle_state *cpuidle_state_table;
+
+/*
+ * States are indexed by the cstate number,
+ * which is also the index into the MWAIT hint array.
+ * Thus C0 is a dummy.
+ */
+static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
+	{ "NHM-C1", "MWAIT 0x00", (void *) 0x00,
+		CPUIDLE_FLAG_TIME_VALID,
+		3, 1000, 6, 0, 0, &intel_idle },
+	{ "NHM-C3", "MWAIT 0x10", (void *) 0x10,
+		CPUIDLE_FLAG_TIME_VALID,
+		20, 500, 80, 0, 0, &intel_idle },
+	{ "NHM-C6", "MWAIT 0x20", (void *) 0x20,
+		CPUIDLE_FLAG_TIME_VALID,
+		200, 350, 800, 0, 0, &intel_idle },
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0}
+};
+
+static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
+	{ "ATM-C1", "MWAIT 0x00", (void *) 0x00,
+		CPUIDLE_FLAG_TIME_VALID,
+		1, 1000, 4, 0, 0, &intel_idle },
+	{ "ATM-C2", "MWAIT 0x10", (void *) 0x10,
+		CPUIDLE_FLAG_TIME_VALID,
+		20, 500, 80, 0, 0, &intel_idle },
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
+	{ "ATM-C4", "MWAIT 0x30", (void *) 0x30,
+		CPUIDLE_FLAG_TIME_VALID,
+		100, 250, 400, 0, 0, &intel_idle },
+	{ "ATM-C6", "MWAIT 0x40", (void *) 0x40,
+		CPUIDLE_FLAG_TIME_VALID,
+		200, 150, 800, 0, 0, &intel_idle },
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
+	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0}
+};
+
+/*
+ * choose_substate()
+ *
+ * Run-time decision on which C-state substate to invoke
+ * If power_policy = 0, choose shallowest substate (0)
+ * If power_policy = 15, choose deepest substate
+ * If power_policy = middle, choose middle substate etc.
+ */
+static int choose_substate(int cstate)
+{
+	unsigned int num_substates;
+
+	power_policy &= 0xF;	/* valid range: 0-15 */
+	cstate &= 7;	/* valid range: 0-7 */
+
+	num_substates = get_num_mwait_substates(cstate);
+
+	if (num_substates <= 1)
+		return 0;
+	
+	return ((power_policy + (power_policy + 1) * (num_substates - 1)) / 16);
+}
+
+/**
+ * intel_idle
+ * @dev: cpuidle_device
+ * @state: cpuidle state
+ *
+ */
+static int intel_idle(struct cpuidle_device *dev, struct cpuidle_state *state)
+{
+	unsigned long ecx = 1; /* break on interrupt flag */
+	unsigned long eax = (unsigned long)cpuidle_get_statedata(state);
+	unsigned int cstate;
+	ktime_t kt_before, kt_after;
+	s64 usec_delta;
+	int cpu = smp_processor_id();
+
+	cstate = mwait_hint_to_cstate(eax);
+
+	eax = eax + choose_substate(cstate);
+
+	local_irq_disable();
+
+	if (!lapic_timer_reliable(cstate))
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu); 
+
+	kt_before = ktime_get_real();
+
+	stop_critical_timings();
+#ifndef MODULE
+	trace_power_start(POWER_CSTATE, (eax >> 4) + 1);
+#endif
+	if (!need_resched()) {
+
+		__monitor((void *)&current_thread_info()->flags, 0, 0);
+		smp_mb();
+		if (!need_resched())
+			__mwait(eax, ecx);
+	}
+
+	start_critical_timings();
+
+	kt_after = ktime_get_real();
+	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
+
+	local_irq_enable();
+	
+	if (!lapic_timer_reliable(cstate))
+ 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu); 
+
+	return usec_delta;
+}
+
+
+/*
+ * intel_idle_probe()
+ */
+static int intel_idle_probe(void)
+{
+	unsigned int eax, ebx, ecx, edx;
+
+	if (max_cstate == 0) {
+		pr_debug(PREFIX "disabled\n" );
+		return -1;
+	}
+
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+		return -1;
+ 
+	if (!boot_cpu_has(X86_FEATURE_MWAIT))
+		return -1;
+
+	if (boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF)
+		return -1;
+
+	cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx);
+
+	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED) ||
+		!(ecx & CPUID5_ECX_INTERRUPT_BREAK))
+			return -1;
+
+	pr_debug(PREFIX "MWAIT substates: 0x%x\n", edx);
+
+#ifdef DEBUG
+	if (substates == 0)	/* can over-ride via modparam */
+#endif
+		substates = edx;
+
+	/*
+ 	 * Bail out if non-stop TSC unavailable.
+ 	 * Nehalem and newer have it.
+ 	 *
+ 	 * Atom and Core2 will will require
+ 	 * mark_tsc_unstable("TSC halts in idle")
+ 	 * when have a state deeper than C1
+ 	 */
+	if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
+		return -1;
+
+	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
+		lapic_timer_reliable_states = 0xFFFFFFFF;
+
+	if (boot_cpu_data.x86 != 6)	/* family 6 */
+		return -1;
+
+	switch (boot_cpu_data.x86_model) {
+
+	case 0x1A:	/* Core i7, Xeon 5500 series */
+	case 0x1E:	/* Core i7 and i5 Processor - Lynnfield, Jasper Forest */
+	case 0x1F:	/* Core i7 and i5 Processor - Nehalem */
+	case 0x2E:	/* Nehalem-EX Xeon */
+		lapic_timer_reliable_states = (1 << 1);	 /* C1 */
+
+	case 0x25:	/* Westmere */
+	case 0x2C:	/* Westmere */
+
+		cpuidle_state_table = nehalem_cstates;
+		break;
+#ifdef notyet
+	case 0x1C:	/* 28 - Atom Processor */
+		cpuidle_state_table = atom_cstates;
+		break;
+#endif
+
+	case 0x17:	/* 23 - Core 2 Duo */
+		lapic_timer_reliable_states = (1 << 2) | (1 << 1); /* C2, C1 */
+
+	default:
+		pr_debug(PREFIX "does not run on family %d model %d\n",
+			boot_cpu_data.x86, boot_cpu_data.x86_model);
+		return -1;
+	}
+
+	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
+		" model 0x%X\n", boot_cpu_data.x86_model);
+
+pr_debug(PREFIX "lapic_timer_reliable_states 0x%x\n", lapic_timer_reliable_states);
+	return 0;
+}
+
+/*
+ * intel_idle_cpuidle_devices_init()
+ * allocate, initialize, register cpuidle_devices
+ */
+static int intel_idle_cpuidle_devices_init(void)
+{
+	int i, cstate;
+	struct cpuidle_device *dev;
+
+	intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (intel_idle_cpuidle_devices == NULL)
+		return -1;
+
+	for_each_online_cpu(i) {
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
+
+		dev->state_count = 1;
+
+		for (cstate = 1; cstate < MWAIT_MAX_NUM_CSTATES; ++cstate) {
+			int num_substates;
+
+			if (cstate > max_cstate) {
+				printk(PREFIX "max_cstate %d exceeded", max_cstate);
+				break;
+			}
+
+			/* does the state exist in CPUID.MWAIT? */
+			num_substates = get_num_mwait_substates(cstate);
+			if (num_substates == 0)
+				continue;
+
+			/* does the state exist in the driver's table? */
+			if (cpuidle_state_table[cstate].name == NULL) {
+				pr_debug(PREFIX "unaware of model 0x%x MWAIT %x\
+					please contact lenb@kernel.org", boot_cpu_data.x86_model, cstate);
+				continue;
+			}
+			dev->states[dev->state_count] = cpuidle_state_table[cstate];	/* structure copy */
+			dev->state_count += 1;
+		}
+
+		dev->cpu = i;
+		if (cpuidle_register_device(dev)) {
+			pr_debug(PREFIX "cpuidle_register_device %d failed!\n", i);
+			free_percpu(intel_idle_cpuidle_devices);
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * intel_idle_cpuidle_devices_uninit()
+ * unregister, free cpuidle_devices
+ */
+static void intel_idle_cpuidle_devices_uninit(void)
+{
+	int i;
+	struct cpuidle_device *dev;
+
+	for_each_online_cpu(i) {
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
+		cpuidle_unregister_device(dev);
+	}
+
+	free_percpu(intel_idle_cpuidle_devices);
+	return;
+}
+
+static int intel_idle_init(void) 
+{
+
+	if (intel_idle_probe())
+		return -1;
+
+	if (cpuidle_register_driver(&intel_idle_driver)) {
+		pr_debug(PREFIX "unable to register with cpuidle due to %s",
+			cpuidle_get_driver()->name);
+		return -1;
+	}
+
+	if (intel_idle_cpuidle_devices_init()) {
+		cpuidle_unregister_driver(&intel_idle_driver);
+		return -1;
+	}
+
+	return 0;	
+}
+
+static void __exit intel_idle_exit(void)
+{
+	intel_idle_cpuidle_devices_uninit();
+	cpuidle_unregister_driver(&intel_idle_driver);
+
+	return;
+}
+
+module_init(intel_idle_init);
+module_exit(intel_idle_exit);
+
+module_param(power_policy, int, 0644);
+module_param(max_cstate, int, 0444);
+#ifdef DEBUG
+module_param(substates, int, 0444);
+#endif
+
+MODULE_AUTHOR("Len Brown <len.brown@intel.com>");
+MODULE_DESCRIPTION("Cpuidle driver for Intel Hardware v" INTEL_IDLE_VERSION);
+MODULE_LICENSE("GPL");
-- 
1.6.0.6

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Andrew Morton]

On Wed, 26 May 2010 22:42:31 -0400 Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> This EXPERIMENTAL driver supersedes acpi_idle
> on modern Intel processors. (Nehalem and Atom Processors).
> 
> For CONFIG_INTEL_IDLE=y, intel_idle probes before acpi_idle,
> no matter if CONFIG_ACPI_PROCESSOR=y or =m.
> 
> Boot with "intel_idle.max_cstate=0" to disable the driver
> and to fall back on ACPI.
> 
> CONFIG_INTEL_IDLE=m is not recommended unless the system
> has a method to guarantee intel_idle loads before ACPI's
> processor_idle.
> 
> This driver does not yet know about cpu online/offline
> and thus will not yet play well with cpu-hotplug.
> 
>
> ...
>
> +/*
> + * Known issues
> + *
> + * The driver currently initializes for_each_online_cpu() upon modprobe.
> + * It it unaware cpu online/offline and cpui hotplug

(typo).

Implications?  What happens when an additional CPU is brought online? 
It melts?  ;)

CPU hotplug support is usually pretty simple to provide.  But it tends
to affect the overall code structure and is best designed-in at the
outset.

> + * ACPI has a .suspend hack to turn off deep c-statees during suspend
> + * to avoid complications with the lapic timer workaround
> + * will need to address that situation here too.
> + *
> + * There is currently no automatic probing/loading mechanism
> + * if the driver is built as a module.
> + */
>
> ...
>
> +#define mwait_hint_to_cstate(hint) ((((hint) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1)
> +#define lapic_timer_reliable(cstate) (lapic_timer_reliable_states & (1 << (cstate)))

Could have been implemented in C.  Nicer to look at, better typechecking.

> +static struct cpuidle_driver intel_idle_driver = {
> +	.name = "intel_idle",
> +	.owner = THIS_MODULE,
> +};
> +static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;	/* intel_idle.max_cstate=0 disables driver */
> +static int power_policy = 7; /* 0 = max perf; 15 = max powersave */
> +
> +static unsigned int substates;
> +#define get_num_mwait_substates(cstate) ((substates >> ((cstate) * 4)) && 0xF)

ddiittoo..

> +/* Reliable LAPIC Timer States, bit 1 for C1 etc.  */
> +static unsigned int lapic_timer_reliable_states;
> +
> +static struct cpuidle_device *intel_idle_cpuidle_devices;
> +static int intel_idle(struct cpuidle_device *dev, struct cpuidle_state *state);
> +
> +/*
> + * These attributes are be visible under
> + * /sys/devices/system/cpu/cpu.../cpuidle/state.../
> + *
> + * name 
> + * 	Hardware name of the state, from datasheet
> + * desc
> + * 	MWAIT param
> + * driver_data
> + * 	token passed to intel_idle()
> + * flags
> + * 	CPUIDLE_FLAG_TIME_VALID
> + * 		we return valid times in all states
> + * 	CPUIDLE_FLAG_SHALLOW
> + * 		lapic timer keeps running
> + * exit_latency
> + * 	[usec]
> + * power_usage
> + * 	mW (TBD)
> + * target_residency
> + * 	currently we multiply exit_latency by 4
> + * 	[usec]
> + * usage
> + * 	instance counter
> + * time
> + * 	residency counter [usec]
> + */

There's been a half-hearted attempt to describe sysfs files in
Documentation/ABI/.

>
> ...
>
> +static struct cpuidle_state nehalem_cstates[MWAIT_MAX_NUM_CSTATES] = {
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "NHM-C1", "MWAIT 0x00", (void *) 0x00,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		3, 1000, 6, 0, 0, &intel_idle },
> +	{ "NHM-C3", "MWAIT 0x10", (void *) 0x10,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		20, 500, 80, 0, 0, &intel_idle },
> +	{ "NHM-C6", "MWAIT 0x20", (void *) 0x20,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		200, 350, 800, 0, 0, &intel_idle },
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0}
> +};
> +
> +static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "ATM-C1", "MWAIT 0x00", (void *) 0x00,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		1, 1000, 4, 0, 0, &intel_idle },
> +	{ "ATM-C2", "MWAIT 0x10", (void *) 0x10,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		20, 500, 80, 0, 0, &intel_idle },
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "ATM-C4", "MWAIT 0x30", (void *) 0x30,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		100, 250, 400, 0, 0, &intel_idle },
> +	{ "ATM-C6", "MWAIT 0x40", (void *) 0x40,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		200, 150, 800, 0, 0, &intel_idle },
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0}
> +};

Could omit all the zeroes.  Could possibly also omit the "" strings,
with suitable handling.

These would be better in { .name = value, } form.

>
> ...
>
> +static int intel_idle_probe(void)
> +{
> +	unsigned int eax, ebx, ecx, edx;
> +
> +	if (max_cstate == 0) {
> +		pr_debug(PREFIX "disabled\n" );
> +		return -1;
> +	}
> +
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> +		return -1;
> + 
> +	if (!boot_cpu_has(X86_FEATURE_MWAIT))
> +		return -1;
> +
> +	if (boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF)
> +		return -1;
> +
> +	cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx);
> +
> +	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED) ||
> +		!(ecx & CPUID5_ECX_INTERRUPT_BREAK))
> +			return -1;
> +
> +	pr_debug(PREFIX "MWAIT substates: 0x%x\n", edx);
> +
> +#ifdef DEBUG
> +	if (substates == 0)	/* can over-ride via modparam */
> +#endif
> +		substates = edx;
> +
> +	/*
> + 	 * Bail out if non-stop TSC unavailable.
> + 	 * Nehalem and newer have it.
> + 	 *
> + 	 * Atom and Core2 will will require
> + 	 * mark_tsc_unstable("TSC halts in idle")
> + 	 * when have a state deeper than C1
> + 	 */
> +	if (!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
> +		return -1;
> +
> +	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
> +		lapic_timer_reliable_states = 0xFFFFFFFF;
> +
> +	if (boot_cpu_data.x86 != 6)	/* family 6 */
> +		return -1;
> +
> +	switch (boot_cpu_data.x86_model) {
> +
> +	case 0x1A:	/* Core i7, Xeon 5500 series */
> +	case 0x1E:	/* Core i7 and i5 Processor - Lynnfield, Jasper Forest */
> +	case 0x1F:	/* Core i7 and i5 Processor - Nehalem */
> +	case 0x2E:	/* Nehalem-EX Xeon */
> +		lapic_timer_reliable_states = (1 << 1);	 /* C1 */
> +
> +	case 0x25:	/* Westmere */
> +	case 0x2C:	/* Westmere */
> +
> +		cpuidle_state_table = nehalem_cstates;
> +		break;
> +#ifdef notyet
> +	case 0x1C:	/* 28 - Atom Processor */
> +		cpuidle_state_table = atom_cstates;
> +		break;
> +#endif
> +
> +	case 0x17:	/* 23 - Core 2 Duo */
> +		lapic_timer_reliable_states = (1 << 2) | (1 << 1); /* C2, C1 */
> +
> +	default:
> +		pr_debug(PREFIX "does not run on family %d model %d\n",
> +			boot_cpu_data.x86, boot_cpu_data.x86_model);
> +		return -1;
> +	}
> +
> +	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
> +		" model 0x%X\n", boot_cpu_data.x86_model);
> +
> +pr_debug(PREFIX "lapic_timer_reliable_states 0x%x\n", lapic_timer_reliable_states);

layout went funny.

> +	return 0;
> +}
> +
> +/*
> + * intel_idle_cpuidle_devices_init()
> + * allocate, initialize, register cpuidle_devices
> + */
> +static int intel_idle_cpuidle_devices_init(void)
> +{
> +	int i, cstate;
> +	struct cpuidle_device *dev;
> +
> +	intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
> +	if (intel_idle_cpuidle_devices == NULL)
> +		return -1;
> +
> +	for_each_online_cpu(i) {
> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
> +
> +		dev->state_count = 1;
> +
> +		for (cstate = 1; cstate < MWAIT_MAX_NUM_CSTATES; ++cstate) {
> +			int num_substates;
> +
> +			if (cstate > max_cstate) {
> +				printk(PREFIX "max_cstate %d exceeded", max_cstate);
> +				break;
> +			}
> +
> +			/* does the state exist in CPUID.MWAIT? */
> +			num_substates = get_num_mwait_substates(cstate);
> +			if (num_substates == 0)
> +				continue;
> +
> +			/* does the state exist in the driver's table? */
> +			if (cpuidle_state_table[cstate].name == NULL) {
> +				pr_debug(PREFIX "unaware of model 0x%x MWAIT %x\
> +					please contact lenb@kernel.org", boot_cpu_data.x86_model, cstate);
> +				continue;
> +			}
> +			dev->states[dev->state_count] = cpuidle_state_table[cstate];	/* structure copy */
> +			dev->state_count += 1;
> +		}
> +
> +		dev->cpu = i;
> +		if (cpuidle_register_device(dev)) {
> +			pr_debug(PREFIX "cpuidle_register_device %d failed!\n", i);
> +			free_percpu(intel_idle_cpuidle_devices);
> +			return -EIO;

Should this unregister all the thus-far-registered devices?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * intel_idle_cpuidle_devices_uninit()
> + * unregister, free cpuidle_devices
> + */
> +static void intel_idle_cpuidle_devices_uninit(void)
> +{
> +	int i;
> +	struct cpuidle_device *dev;
> +
> +	for_each_online_cpu(i) {
> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
> +		cpuidle_unregister_device(dev);
> +	}
> +
> +	free_percpu(intel_idle_cpuidle_devices);
> +	return;
> +}
> +
> +static int intel_idle_init(void) 

__init?

The patch adds trailing whitespace.  Has various other checkpatch
problems.

> +{
> +
> +	if (intel_idle_probe())
> +		return -1;
> +
> +	if (cpuidle_register_driver(&intel_idle_driver)) {
> +		pr_debug(PREFIX "unable to register with cpuidle due to %s",
> +			cpuidle_get_driver()->name);
> +		return -1;
> +	}
> +
> +	if (intel_idle_cpuidle_devices_init()) {
> +		cpuidle_unregister_driver(&intel_idle_driver);
> +		return -1;
> +	}

All these hard-coded -1's are a bit sloppy.  Could use -ENODEV, -EIO,
etc.  The only real value in this is to get better diagnostics out of
do_one_initcall() when initcall_debug was selected.

> +	return 0;	
> +}
> +

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Len Brown]

> ... What happens when an additional CPU is brought online? 
> It melts?  ;)

With the current driver, processors hot-added after modprobe
will use C1 only, and not use deeper C-states.

Taking online processors offline and bringing them back online
(like we do during suspend) works fine.

> > +		dev->cpu = i;
> > +		if (cpuidle_register_device(dev)) {
> > +			pr_debug(PREFIX "cpuidle_register_device %d failed!\n", i);
> > +			free_percpu(intel_idle_cpuidle_devices);
> > +			return -EIO;
> 
> Should this unregister all the thus-far-registered devices?

yes, that was a bug!

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Andi Kleen]

Len Brown <lenb@kernel.org> writes:

>> ... What happens when an additional CPU is brought online? 
>> It melts?  ;)
>
> With the current driver, processors hot-added after modprobe
> will use C1 only, and not use deeper C-states.

I agree with Andrew that it will likely not be a lot of 
effort to restructure the driver to handle cpu hotplug
properly, and it's better done from the beginning.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Thomas Renninger]

On Thursday 27 May 2010 04:42:31 Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
... 
> CONFIG_INTEL_IDLE=m is not recommended unless the system
> has a method to guarantee intel_idle loads before ACPI's
> processor_idle.
Then it should better be declared bool instead of tristate until it
works.

> This driver does not yet know about cpu online/offline
> and thus will not yet play well with cpu-hotplug.
What means does not play well yet, suspend or manually offlining a core 
will eventually (for sure?) hang the machine?

If this is known broken, should this already be spread through
linux-next?

    Thomas

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Len Brown]

On Thu, 27 May 2010, Thomas Renninger wrote:

> On Thursday 27 May 2010 04:42:31 Len Brown wrote:
> > From: Len Brown <len.brown@intel.com>
> ... 
> > CONFIG_INTEL_IDLE=m is not recommended unless the system
> > has a method to guarantee intel_idle loads before ACPI's
> > processor_idle.

> Then it should better be declared bool instead of tristate until it
> works.

intel_idle as a module works fine, and tristate should be retained.

If user-space chooses to load intel_idle before acpi processor,
then it correctly handlees idle states and acpi
correctly yields.  If user space gets them in the other order,
then user-space gets what it asked for.

The fact that a typical desktop distro load acpi-cpufreq first,
and that depends on the acpi processor driver should not prohibit
intel_idle from being modular.

Indeed, intel_idle has every right to be moduler on a system
where CONFIG_ACPI=n...

> > This driver does not yet know about cpu online/offline
> > and thus will not yet play well with cpu-hotplug.

> What means does not play well yet, suspend or manually offlining a core 
> will eventually (for sure?) hang the machine?

It means less power savings savings than optimal
for processors not present at module load time.

> If this is known broken, should this already be spread through
> linux-next?

If you know somebody with a system that supports CPU hot-add
on one of the processors supported by intel_idle, and they
are willing to test linux-next, please have them contact me.

thanks,
-Len Brown, Intel Open Source Technology Center

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Thomas Renninger]

On Friday 28 May 2010 03:44:16 Len Brown wrote:
> On Thu, 27 May 2010, Thomas Renninger wrote:
...
> > > This driver does not yet know about cpu online/offline
> > > and thus will not yet play well with cpu-hotplug.
I thought this is also about soft on/offlining.
> 
> > What means does not play well yet, suspend or manually offlining a
> > core will eventually (for sure?) hang the machine?
> 
> It means less power savings savings than optimal
> for processors not present at module load time.
Ok, not really sever...
> 
> > If this is known broken, should this already be spread through
> > linux-next?
> 
> If you know somebody with a system that supports CPU hot-add
> on one of the processors supported by intel_idle, and they
> are willing to test linux-next, please have them contact me.
Real CPU hotplug is broken with acpi processor driver as well,
eventually it got addressed lately. Anyway, not sever...

Thanks,

    Thomas

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Len Brown]

On Fri, 28 May 2010, Thomas Renninger wrote:

> On Friday 28 May 2010 03:44:16 Len Brown wrote:
> > On Thu, 27 May 2010, Thomas Renninger wrote:
> ...
> > > > This driver does not yet know about cpu online/offline
> > > > and thus will not yet play well with cpu-hotplug.

> I thought this is also about soft on/offlining.

Right, the driver is ignorant of soft on/offlining,
but doesn't break soft on/offlining.
So rather than un-registering from cpuidle on
an offline event and re-registering on an on-line event,
it just stays registered.

The vast majority of on/offline is suspend to ram,
and this works just fine there.

Soft off-lining is somewhat broken for power management
independent of this driver, of course.  As it uses only C1
it can negatively impact the ability of the online processors
to save power and enter turbo mode...

> > > What means does not play well yet, suspend or manually offlining a
> > > core will eventually (for sure?) hang the machine?
> > 
> > It means less power savings savings than optimal
> > for processors not present at module load time.

> Ok, not really sever...

Agreed, not severe.

> > > If this is known broken, should this already be spread through
> > > linux-next?
> > 
> > If you know somebody with a system that supports CPU hot-add
> > on one of the processors supported by intel_idle, and they
> > are willing to test linux-next, please have them contact me.

> Real CPU hotplug is broken with acpi processor driver as well,
> eventually it got addressed lately. Anyway, not sever...

Yeah, if I had a system with real cpu hotplug, I'd go ahead
and test it -- but I've never even seen one.

thanks,
-Len Brown, Intel Open Source Technology Center

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Thomas Renninger]

On Friday 28 May 2010 03:44:16 am Len Brown wrote:
> On Thu, 27 May 2010, Thomas Renninger wrote:
> > On Thursday 27 May 2010 04:42:31 Len Brown wrote:
...
> > If this is known broken, should this already be spread through
> > linux-next?
>
> If you know somebody with a system that supports CPU hot-add
> on one of the processors supported by intel_idle, and they
> are willing to test linux-next, please have them contact me.
This was my attempt to fix it.
There still is an issue, eventually someone sees it,
but it may give a picture what is missing.

        Thomas

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index b5658cd..62cd8a3 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -445,7 +445,28 @@ static int acpi_processor_get_info(struct acpi_device *device)
 		    (acpi_processor_hotadd_init(pr->handle, &pr->id))) {
 			return -ENODEV;
 		}
+		/*
+		 * CPU not initialized yet, the rest of .add must not touch
+		 * (struct cpuinfo_x86) cpu_data(cpu)!
+		 * If it does, move it from .add to init_components to
+		 * get it done, once the cpu got online and set up the
+		 * first time. Also look into the CPU_ONLINE notify code.
+		 */
+		pr->flags.hotplugged_uninitialized = 1;
+	} else {
+		struct cpuinfo_x86 *c;
+		c = &cpu_data(pr->id);
+		/*
+		 * Same as above, but the driver could have been unloaded
+		 * before the cpu got onlined the first time.
+		 */
+		if (c->x86 == 0 && c->x86_model == 0 && c->x86_mask == 0) {
+			printk(KERN_INFO "CPU %d not initialized yet!\n",
+			       pr->id);
+			pr->flags.hotplugged_uninitialized = 1;
+		}
 	}
+
 	/*
 	 * On some boxes several processors use the same processor bus id.
 	 * But they are located in different scope. For example:
@@ -535,16 +556,94 @@ static void acpi_processor_notify(struct acpi_device *device, u32 event)
 	return;
 }
 
+/*
+ * Add everything that must be run on the CPU that gets initialized
+ * or that depends on cpu_data(cpu) here.
+ * Otherwise things will break in cpu hotadd case as this info is not
+ * yet available in acpi_processor_add or acpi_processor_get_info()
+ */
+
+static int __cpuinit acpi_processor_init_components(struct acpi_processor *pr)
+{
+	int result = 0;
+	struct acpi_device *device;
+
+	acpi_bus_get_device(pr->handle, &device);
+
+	printk(KERN_INFO "%s - cpu: %d\n", __func__, pr->id);
+
+#ifdef CONFIG_CPU_FREQ
+	acpi_processor_ppc_has_changed(pr, 0);
+#endif
+	acpi_processor_get_throttling_info(pr);
+	acpi_processor_get_limit_info(pr);
+
+
+	acpi_processor_power_init(pr, device);
+
+	pr->cdev = thermal_cooling_device_register("Processor", device,
+						&processor_cooling_ops);
+	if (IS_ERR(pr->cdev)) {
+		result = PTR_ERR(pr->cdev);
+		goto err_power_exit;
+	}
+
+	dev_dbg(&device->dev, "registered as cooling_device%d\n",
+		 pr->cdev->id);
+
+	result = sysfs_create_link(&device->dev.kobj,
+				   &pr->cdev->device.kobj,
+				   "thermal_cooling");
+	if (result) {
+		printk(KERN_ERR PREFIX "Create sysfs link\n");
+		goto err_thermal_unregister;
+	}
+	result = sysfs_create_link(&pr->cdev->device.kobj,
+				   &device->dev.kobj,
+				   "device");
+	if (result) {
+		printk(KERN_ERR PREFIX "Create sysfs link\n");
+		goto err_remove_sysfs;
+	}
+
+	return 0;
+
+err_remove_sysfs:
+	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
+err_thermal_unregister:
+	thermal_cooling_device_unregister(pr->cdev);
+err_power_exit:
+	acpi_processor_power_exit(pr, device);
+
+	return result;
+}
+
+
 static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		unsigned long action, void *hcpu)
 {
 	unsigned int cpu = (unsigned long)hcpu;
 	struct acpi_processor *pr = per_cpu(processors, cpu);
+	int ret;
 
 	if (action == CPU_ONLINE && pr) {
-		acpi_processor_ppc_has_changed(pr, 0);
-		acpi_processor_cst_has_changed(pr);
-		acpi_processor_tstate_has_changed(pr);
+		if (pr->flags.hotplugged_uninitialized) {
+			/* We only run into this if a CPU got hotplugged
+			 * and not fully initialized yet in .add
+			 * We have to do it here and only once,
+			 * because in .add cpu_data(cpu) is not set up yet
+			 */
+			acpi_processor_set_pdc(pr);
+			ret = acpi_processor_init_components(pr);
+			printk(KERN_INFO "Do initialize: %d - CPU: %d\n",
+			       ret, pr->id);
+			pr->flags.hotplugged_uninitialized = 0;
+		} else {
+			printk(KERN_INFO "already initialized: %d\n", pr->id);
+			acpi_processor_ppc_has_changed(pr, 0);
+			acpi_processor_cst_has_changed(pr);
+			acpi_processor_tstate_has_changed(pr);
+		}
 	}
 	return NOTIFY_OK;
 }
@@ -608,48 +707,14 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
 		goto err_remove_fs;
 	}
 
-#ifdef CONFIG_CPU_FREQ
-	acpi_processor_ppc_has_changed(pr, 0);
-#endif
-	acpi_processor_get_throttling_info(pr);
-	acpi_processor_get_limit_info(pr);
-
-
-	acpi_processor_power_init(pr, device);
-
-	pr->cdev = thermal_cooling_device_register("Processor", device,
-						&processor_cooling_ops);
-	if (IS_ERR(pr->cdev)) {
-		result = PTR_ERR(pr->cdev);
-		goto err_power_exit;
-	}
-
-	dev_dbg(&device->dev, "registered as cooling_device%d\n",
-		 pr->cdev->id);
-
-	result = sysfs_create_link(&device->dev.kobj,
-				   &pr->cdev->device.kobj,
-				   "thermal_cooling");
-	if (result) {
-		printk(KERN_ERR PREFIX "Create sysfs link\n");
-		goto err_thermal_unregister;
-	}
-	result = sysfs_create_link(&pr->cdev->device.kobj,
-				   &device->dev.kobj,
-				   "device");
-	if (result) {
-		printk(KERN_ERR PREFIX "Create sysfs link\n");
-		goto err_remove_sysfs;
+	if (!pr->flags.hotplugged_uninitialized) {
+		result = acpi_processor_init_components(pr);
+		if (result)
+			goto err_remove_fs;
 	}
 
 	return 0;
 
-err_remove_sysfs:
-	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
-err_thermal_unregister:
-	thermal_cooling_device_unregister(pr->cdev);
-err_power_exit:
-	acpi_processor_power_exit(pr, device);
 err_remove_fs:
 	acpi_processor_remove_fs(device);
 err_free_cpumask:
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 86825dd..e7716ec 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -207,6 +207,7 @@ struct acpi_processor_flags {
 	u8 has_cst:1;
 	u8 power_setup_done:1;
 	u8 bm_rld_set:1;
+	u8 hotplugged_uninitialized:1;
 };
 
 struct acpi_processor {

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Kevin Hilman]

Len Brown <lenb@kernel.org> writes:

> From: Len Brown <len.brown@intel.com>
>
> This EXPERIMENTAL driver supersedes acpi_idle
> on modern Intel processors. (Nehalem and Atom Processors).
>
> For CONFIG_INTEL_IDLE=y, intel_idle probes before acpi_idle,
> no matter if CONFIG_ACPI_PROCESSOR=y or =m.
>
> Boot with "intel_idle.max_cstate=0" to disable the driver
> and to fall back on ACPI.
>
> CONFIG_INTEL_IDLE=m is not recommended unless the system
> has a method to guarantee intel_idle loads before ACPI's
> processor_idle.
>
> This driver does not yet know about cpu online/offline
> and thus will not yet play well with cpu-hotplug.
>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  MAINTAINERS                     |    7 +
>  drivers/Makefile                |    2 +-
>  drivers/acpi/processor_driver.c |    6 +-
>  drivers/idle/Kconfig            |   11 +
>  drivers/idle/Makefile           |    1 +
>  drivers/idle/intel_idle.c       |  446 +++++++++++++++++++++++++++++++++++++++

Any reason this arch-specific driver needs to be in drivers/idle
instead of under a platform specific dir like arch/x86?

On embedded SoCs that have never had ACPI, we have our
platform-specific CPUidle drivers in with the rest of our platform
specific code.

Kevin

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Arjan van de Ven]

On Thu, 27 May 2010 07:14:46 -0700
Kevin Hilman <khilman@deeprootsystems.com> wrote:

> Len Brown <lenb@kernel.org> writes:
> 
> > From: Len Brown <len.brown@intel.com>
> >
> > This EXPERIMENTAL driver supersedes acpi_idle
> > on modern Intel processors. (Nehalem and Atom Processors).
> >
> > For CONFIG_INTEL_IDLE=y, intel_idle probes before acpi_idle,
> > no matter if CONFIG_ACPI_PROCESSOR=y or =m.
> >
> > Boot with "intel_idle.max_cstate=0" to disable the driver
> > and to fall back on ACPI.
> >
> > CONFIG_INTEL_IDLE=m is not recommended unless the system
> > has a method to guarantee intel_idle loads before ACPI's
> > processor_idle.
> >
> > This driver does not yet know about cpu online/offline
> > and thus will not yet play well with cpu-hotplug.
> >
> > Signed-off-by: Len Brown <len.brown@intel.com>
> > ---
> >  MAINTAINERS                     |    7 +
> >  drivers/Makefile                |    2 +-
> >  drivers/acpi/processor_driver.c |    6 +-
> >  drivers/idle/Kconfig            |   11 +
> >  drivers/idle/Makefile           |    1 +
> >  drivers/idle/intel_idle.c       |  446
> > +++++++++++++++++++++++++++++++++++++++
> 
> Any reason this arch-specific driver needs to be in drivers/idle
> instead of under a platform specific dir like arch/x86?
> 
> On embedded SoCs that have never had ACPI, we have our
> platform-specific CPUidle drivers in with the rest of our platform
> specific code.
> 

it's really inconvenient to have such drivers hidden in the
architecture code; it's much more convenient for cpuidle developers
if they're all in one place.
Think of it this way: you're not putting the NIC driver for your SOC in
a architecture directory either...


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Kevin Hilman]

Arjan van de Ven <arjan@infradead.org> writes:

> On Thu, 27 May 2010 07:14:46 -0700
> Kevin Hilman <khilman@deeprootsystems.com> wrote:
>
>> Len Brown <lenb@kernel.org> writes:
>> 
>> > From: Len Brown <len.brown@intel.com>
>> >
>> > This EXPERIMENTAL driver supersedes acpi_idle
>> > on modern Intel processors. (Nehalem and Atom Processors).
>> >
>> > For CONFIG_INTEL_IDLE=y, intel_idle probes before acpi_idle,
>> > no matter if CONFIG_ACPI_PROCESSOR=y or =m.
>> >
>> > Boot with "intel_idle.max_cstate=0" to disable the driver
>> > and to fall back on ACPI.
>> >
>> > CONFIG_INTEL_IDLE=m is not recommended unless the system
>> > has a method to guarantee intel_idle loads before ACPI's
>> > processor_idle.
>> >
>> > This driver does not yet know about cpu online/offline
>> > and thus will not yet play well with cpu-hotplug.
>> >
>> > Signed-off-by: Len Brown <len.brown@intel.com>
>> > ---
>> >  MAINTAINERS                     |    7 +
>> >  drivers/Makefile                |    2 +-
>> >  drivers/acpi/processor_driver.c |    6 +-
>> >  drivers/idle/Kconfig            |   11 +
>> >  drivers/idle/Makefile           |    1 +
>> >  drivers/idle/intel_idle.c       |  446
>> > +++++++++++++++++++++++++++++++++++++++
>> 
>> Any reason this arch-specific driver needs to be in drivers/idle
>> instead of under a platform specific dir like arch/x86?
>> 
>> On embedded SoCs that have never had ACPI, we have our
>> platform-specific CPUidle drivers in with the rest of our platform
>> specific code.
>> 
>
> it's really inconvenient to have such drivers hidden in the
> architecture code;

I'm not sure how puting architecture-specific code into an
architecture-specific directory is hiding it, but maybe I'm missing
something.

> it's much more convenient for cpuidle developers if they're all in
> one place.

So should we move all the embedded SoC specific CPUidle drivers into
drivers/idle too?

To me that would be much less convenient as I expect to maintain my
platform-specific CPUidle driver along with the rest of my
platform-specific code.

Kevin

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Len Brown]

> >> >  drivers/idle/intel_idle.c       |  446

> >> Any reason this arch-specific driver needs to be in drivers/idle
> >> instead of under a platform specific dir like arch/x86?

> To me that would be much less convenient as I expect to maintain my
> platform-specific CPUidle driver along with the rest of my
> platform-specific code.

I guess the reason is conveneince of the maintainer (me).

A good case could be made to put this driver under
drivers/cpuidle/, arch/x86/, drivers/platform/x86/,
as well as drivers/idle/ -- and maybe someplace else
that I didn't think of.

Maybe if I maintained all of arch/x86/, then I'd naturally
propose putting it someplace under arch/x86/.  But like i7300_idle,
it runs on only a sub-set of x86 boxes, so it seemed to
make sense to put it with i7300_idle.

BTW. There is an sfi_idle driver in the pipeline as well
with the same naming issue.  It is x86 specific, sfi-specific,
and cpuidle-specific -- so a case could be made to put it in
arch/x86/, drivers/sfi/, drivers/cpuidle/ -- but it will
just as likely land in drivers/idle/.

cheers,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Kevin Hilman]

On Thu, 2010-05-27 at 20:22 -0400, Len Brown wrote:
> > >> >  drivers/idle/intel_idle.c       |  446
> 
> > >> Any reason this arch-specific driver needs to be in drivers/idle
> > >> instead of under a platform specific dir like arch/x86?
> 
> > To me that would be much less convenient as I expect to maintain my
> > platform-specific CPUidle driver along with the rest of my
> > platform-specific code.
> 
> I guess the reason is conveneince of the maintainer (me).

OK, fair enough.

At least that makes more sense to me than "keeping all users in one
place."

Kevin

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Igor Stoppa]

Hi,


ext Arjan van de Ven wrote:

> it's really inconvenient to have such drivers hidden in the
> architecture code; it's much more convenient for cpuidle developers
> if they're all in one place.
>   

why?

> Think of it this way: you're not putting the NIC driver for your SOC in
> a architecture directory either...
>
>   

That would be a separate chip, usually, which has its own driver.

The SOC has some bus interface and the arch-specific part of it, if any, 
is in the arch directory.

igor

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Arjan van de Ven]

On Thu, 27 May 2010 17:51:00 +0300
Igor Stoppa <igor.stoppa@nokia.com> wrote:

> Hi,
> 
> 
> ext Arjan van de Ven wrote:
> 
> > it's really inconvenient to have such drivers hidden in the
> > architecture code; it's much more convenient for cpuidle developers
> > if they're all in one place.
> >   
> 
> why?

because you have all the users of your API in one place.

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Kevin Hilman]

On Thu, 2010-05-27 at 20:14 -0700, Arjan van de Ven wrote:
> On Thu, 27 May 2010 17:51:00 +0300
> Igor Stoppa <igor.stoppa@nokia.com> wrote:
> 
> > Hi,
> > 
> > 
> > ext Arjan van de Ven wrote:
> > 
> > > it's really inconvenient to have such drivers hidden in the
> > > architecture code; it's much more convenient for cpuidle developers
> > > if they're all in one place.
> > >   
> > 
> > why?
> 
> because you have all the users of your API in one place.
> 

That's a rather x86-centric definition of "all".

Are you suggesting we move all the embedded SoC specific CPUidle drivers
to drivers/idle as well?

Kevin

Re: [linux-pm] [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Arjan van de Ven]

On Fri, 28 May 2010 10:27:32 -0700
Kevin Hilman <khilman@deeprootsystems.com> wrote:

> On Thu, 2010-05-27 at 20:14 -0700, Arjan van de Ven wrote:
> > On Thu, 27 May 2010 17:51:00 +0300
> > Igor Stoppa <igor.stoppa@nokia.com> wrote:
> > 
> > > Hi,
> > > 
> > > 
> > > ext Arjan van de Ven wrote:
> > > 
> > > > it's really inconvenient to have such drivers hidden in the
> > > > architecture code; it's much more convenient for cpuidle
> > > > developers if they're all in one place.
> > > >   
> > > 
> > > why?
> > 
> > because you have all the users of your API in one place.
> > 
> 
> That's a rather x86-centric definition of "all".
> 
> Are you suggesting we move all the embedded SoC specific CPUidle
> drivers to drivers/idle as well?

yes.



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Chase Douglas]

On Wed, 2010-05-26 at 22:42 -0400, Len Brown wrote:
> +static struct cpuidle_state atom_cstates[MWAIT_MAX_NUM_CSTATES] = {
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "ATM-C1", "MWAIT 0x00", (void *) 0x00,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		1, 1000, 4, 0, 0, &intel_idle },
> +	{ "ATM-C2", "MWAIT 0x10", (void *) 0x10,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		20, 500, 80, 0, 0, &intel_idle },
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "ATM-C4", "MWAIT 0x30", (void *) 0x30,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		100, 250, 400, 0, 0, &intel_idle },
> +	{ "ATM-C6", "MWAIT 0x40", (void *) 0x40,
> +		CPUIDLE_FLAG_TIME_VALID,
> +		200, 150, 800, 0, 0, &intel_idle },
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0},
> +	{ "", "", 0, 0, 0, 0, 0, 0, 0, 0}
> +};

I see that you have updated this code in your tree to disable C4 and C6
on atom. This has piqued my curiosity. I've now seen 2 atom netbooks
from different OEMs that hide C4 when you plug the power in. After the
first machine I thought, "must be a BIOS/ACPI bug," but now I'm
beginning to wonder if there's some issue with atom C4 states? That's
beside the fact that I've not seen C6 on either machine at all. Do you
have any insight?

Thanks,

-- Chase

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Len Brown]

> I see that you have updated this code in your tree to disable C4 and C6
> on atom. This has piqued my curiosity. I've now seen 2 atom netbooks
> from different OEMs that hide C4 when you plug the power in. After the
> first machine I thought, "must be a BIOS/ACPI bug," but now I'm
> beginning to wonder if there's some issue with atom C4 states? That's
> beside the fact that I've not seen C6 on either machine at all. Do you
> have any insight?

The reasoning behind ACPI taking deep C-states away
when on AC is the assumption that users on AC
care more about low latency and high performance
than they care about power savings, heat, and noise.

I have an Acer aspire-1 that exposes only C1 on AC
but adds C2 and C4 when on DC.

Many believe that the BIOS is not the right place to
make this policy decision.  Others feel that decision
is dated no matter where it is made -- particularly in
light of the latest EPA Energy STAR certification measuring
power ONLY when plugged into AC...

So my intent is to give Linux control over this decision,
via PM_QOS or otherwise.  At the moment C4 is commented out
because when i first tested it failed the lapic timer workaround.
C6 is commented out because I've not found that any BIOS supporting
it -- so it is possibly de-featured or has some other limitations that
I need to find out about.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Chase Douglas]

On Fri, 2010-05-28 at 00:16 -0400, Len Brown wrote:
> > I see that you have updated this code in your tree to disable C4 and C6
> > on atom. This has piqued my curiosity. I've now seen 2 atom netbooks
> > from different OEMs that hide C4 when you plug the power in. After the
> > first machine I thought, "must be a BIOS/ACPI bug," but now I'm
> > beginning to wonder if there's some issue with atom C4 states? That's
> > beside the fact that I've not seen C6 on either machine at all. Do you
> > have any insight?
> 
> The reasoning behind ACPI taking deep C-states away
> when on AC is the assumption that users on AC
> care more about low latency and high performance
> than they care about power savings, heat, and noise.

Maybe they forgot that a netbook is still a *lap*top when they did this,
cause I can't keep mine on my lap when it's plugged in :). The CPU idles
at 60 centigrade when powered on, and drops to a manageable 30-ish
centigrade when running on battery.

> So my intent is to give Linux control over this decision,
> via PM_QOS or otherwise.  At the moment C4 is commented out
> because when i first tested it failed the lapic timer workaround.

I am looking forward to testing out these changes on my netbook once
this issue get sorted out!

-- Chase

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Len Brown]

On Fri, 28 May 2010, Chase Douglas wrote:

> On Fri, 2010-05-28 at 00:16 -0400, Len Brown wrote:
> > > I see that you have updated this code in your tree to disable C4 and C6
> > > on atom. This has piqued my curiosity. I've now seen 2 atom netbooks
> > > from different OEMs that hide C4 when you plug the power in. After the
> > > first machine I thought, "must be a BIOS/ACPI bug," but now I'm
> > > beginning to wonder if there's some issue with atom C4 states? That's
> > > beside the fact that I've not seen C6 on either machine at all. Do you
> > > have any insight?
> > 
> > The reasoning behind ACPI taking deep C-states away
> > when on AC is the assumption that users on AC
> > care more about low latency and high performance
> > than they care about power savings, heat, and noise.
> 
> Maybe they forgot that a netbook is still a *lap*top when they did this,
> cause I can't keep mine on my lap when it's plugged in :). The CPU idles
> at 60 centigrade when powered on, and drops to a manageable 30-ish
> centigrade when running on battery.
> 
> > So my intent is to give Linux control over this decision,
> > via PM_QOS or otherwise.  At the moment C4 is commented out
> > because when i first tested it failed the lapic timer workaround.
> 
> I am looking forward to testing out these changes on my netbook once
> this issue get sorted out!

Atom C4 works in the current driver in the git tree -- test it now!

cheers,
-Len

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Chase Douglas]

On Fri, 2010-05-28 at 13:43 -0400, Len Brown wrote:
> Atom C4 works in the current driver in the git tree -- test it now!

Sadly, it seems this isn't helping much. My netbook still idles at ~60
centigrade on and off power. I will verify that before this change I was
able to idle cooler when off power just to be sure.

I do see the C4 state both on and off power, so that's nice at least.
I'm just perplexed why my netbook runs so hot when idle. I even killed X
to see if it was really the gpu that was heating everything inside up,
but it didn't change anything.

-- Chase

Re: [PATCH 8/8] intel_idle: create a native cpuidle driver for select intel processors

[Chase Douglas]

On Fri, 2010-05-28 at 15:51 -0400, Chase Douglas wrote:
> On Fri, 2010-05-28 at 13:43 -0400, Len Brown wrote:
> > Atom C4 works in the current driver in the git tree -- test it now!
> 
> Sadly, it seems this isn't helping much. My netbook still idles at ~60
> centigrade on and off power. I will verify that before this change I was
> able to idle cooler when off power just to be sure.

Well, now I seem unable to reproduce the lower temperatures at all... I
tried the same kernel without your patches and mostly got the same
temperatures. I tried the Ubuntu 10.04 LTS 2.6.32 based kernel and it
was two or three centigrade cooler, but not the tens of centigrade
cooler I remembered. I also can't see a big change anymore when power is
plugged in or not. Maybe it was all a matter of usage in different
environments making me think it was related to being plugged into power
or not. Oy vey...

-- Chase

(No subject header)

[Milton Miller]

[Hmm, why did this not appear in patchwork.kernel.org?  Now
I have to guess a CC list.]

On Wed, 26 May 2010 around 22:43:50 -0400 (EDT), Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> TS_POLLING set tells the scheduler a task will poll
> need_resched() to look for work.
> 

True

> TS_POLLING clear tells resched_task() and wake_up_idle_cpu()
> that the remote CPU is sleeping in idle, and thus requires
> a reschedule interrupt to wake them to notice work.

No, that only applies to the idle task.


> 
> Update the description of TS_POLLING to reflect how it works.
> "cleared when sleeping in idle, requiring reschedule interrupt"

That would imply its set for every normal task that is not in some
kind of sleep state.


> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> Acked-by: Peter Zijlstra <peterz@infradead.org>

> -#define TS_POLLING		0x0004	/* true if in idle loop
> -					   and not sleeping */
> +#define TS_POLLING		0x0004	/* clear when sleeping in idle
> +					   requiring reschedule interrupt */

How about "idle task polling need_resched, skip sending interrupt"?

milton

Re: (No subject header)

[Len Brown]

On Thu, 27 May 2010, Milton Miller wrote:

> 
> [Hmm, why did this not appear in patchwork.kernel.org?  Now
> I have to guess a CC list.]
> 
> On Wed, 26 May 2010 around 22:43:50 -0400 (EDT), Len Brown wrote:
> > From: Len Brown <len.brown@intel.com>
> > 
> > TS_POLLING set tells the scheduler a task will poll
> > need_resched() to look for work.
> > 
> 
> True
> 
> > TS_POLLING clear tells resched_task() and wake_up_idle_cpu()
> > that the remote CPU is sleeping in idle, and thus requires
> > a reschedule interrupt to wake them to notice work.
> 
> No, that only applies to the idle task.
> 
> 
> > 
> > Update the description of TS_POLLING to reflect how it works.
> > "cleared when sleeping in idle, requiring reschedule interrupt"
> 
> That would imply its set for every normal task that is not in some
> kind of sleep state.

you're right, just the idle task sets this flag.

> > Signed-off-by: Len Brown <len.brown@intel.com>
> > Acked-by: Peter Zijlstra <peterz@infradead.org>
> 
> > -#define TS_POLLING		0x0004	/* true if in idle loop
> > -					   and not sleeping */
> > +#define TS_POLLING		0x0004	/* clear when sleeping in idle
> > +					   requiring reschedule interrupt */
> 
> How about "idle task polling need_resched, skip sending interrupt"?

I think that is an improvement over my wording.

Though technically we're not polling need_resched in the case
I have in mind.  The hardware is snooping any write to the thread flags
via MONITOR/MWAIT trigger address.

cheers,
-Len Brown, Intel Open Source Technology Center

Re: [linux-pm] idle-test patches queued for upstream

[Thomas Renninger]

On Thursday 27 May 2010 04:42:23 Len Brown wrote:
> Please look over and test this patch set.
> (If you test linux-next, you already have it)
> 
> There are a few simple patches, leading up to a new intel_idle driver.
> 
> Note that you can get the patch series as a single patch here:
> http://ftp.kernel.org/pub/linux/kernel/people/lenb/idle/patches/2.6.34/idle-test-2.6.34.diff.gz
> 
> or pull from this git branch
> git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-idle-2.6.git idle-test
> 
> Both are vs 2.6.34.
> 
> Why is it good to have a native intel_idle driver?
> 
> Basically, we think we can do better than ACPI.
Why exactly? Is there any info missing in the ACPI tables?
Or is this just to be more independent from OEMs?

> Indeed, on my (production level commerically available) Nehalem desktop
> the ACPI tables are broken and an ACPI OS idles at 100W.  With this
> driver the box idles at 85W.
What exactly was broken there?

IMO this is a step backward.
CPUfreq runs rather well on nearly every machine supporting it without
tons of static frequency tables in kernel. Even powernow-k8 might get merged
into acpi-cpufreq.

Intel set up a huge ACPI API for this and now it's not used anymore?!?
Will these parts get obsoleted in a future spec?
While for C-states there are not that many static entries needed, another
drawback could be that OEMs will disable/hide C-states on purpose.

Using ACPI table based C-states by default and using intel_idle.enable=1
or similar for workarounds sounds safer.
At least as long as the driver is experimental.

Does Windows use ACPI C-state info for idle?

Thanks,

   Thomas

Re: [linux-pm] idle-test patches queued for upstream

[Len Brown]

> > ... we think we can do better than ACPI.

> Why exactly? Is there any info missing in the ACPI tables?
> Or is this just to be more independent from OEMs?

ACPI has a few fundmental flaws here.  One is that it reports
exit latency instead of break-even power duration.
The other is that it requires a BIOS writer to
get the tables right.

Both of these are fatal flaws.

There are also more subtle problems, like bogus ACPI implementations
mapping LAPIC breaking C-states to ACPI-C2, causing Linux to need
to assume the LAPIC is always broken in in C2 -- which is erroneous.

I'll be speaking on this topic at length at Linuxcon this summer.

> > Indeed, on my (production level commerically available) Nehalem desktop
> > the ACPI tables are broken and an ACPI OS idles at 100W.  With this
> > driver the box idles at 85W.

> What exactly was broken there?

Dell's BIOS developer botched a bug fix immediately before the system
went to market and disabled support for all ACPI C-states except C1.
After several month of shipping systems, they still were unable
to ship them with a fixed BIOS.

Of course, besides a 15% idle power hit,the other effect of that BIOS issue
was to disable all Turbo frequencies -- which is a somewhat important
feature on a Core-i7 desktop...

> IMO this is a step backward.

I don't dispute your right to have an opinion:-)

> CPUfreq runs rather well on nearly every machine supporting it without
> tons of static frequency tables in kernel. Even powernow-k8 might get merged
> into acpi-cpufreq.

There are a couple of important differences between cpufreq and idle
state enumeration.  p-states are per-bin within each model.
Idle states not only span bins within a model, they span multiple
models which span multiple years.  Note also the idle tables are
validated at run-time by CPUID.MWAIT, which means the same
table can be used for multiple parts -- the parts themselves
know which states they have -- and they can tell us.

So I don't expect a proliferation of idle tables in intel_idle.

I do expect to tune some of the latencies based on some of
the information that Intel instructs BIOS writers to convey,
but they fail to convey.  In particular, the actual latencies
and power break-even points of the same model in different
configurations are actually different.  I've not seen a single
BIOS get that part rigiht.

I expect a new table to cover sandy bridge plus the generation after it.

> Intel set up a huge ACPI API for this and now it's not used anymore?!?
> Will these parts get obsoleted in a future spec?

Both p-states and c-states will be moving to a more native enumeration
method - but there will still be BIOS ACPI support wrapping that
enumeration as long as somebody wants to run a legacy ACPI OS that
knows nothing else.

> While for C-states there are not that many static entries needed, another
> drawback could be that OEMs will disable/hide C-states on purpose.

Yes, there is a real possibility that a system has a device in it
that malfunctions when a deep C-state is used.  On Linux, we
invented PM_QOS to address exactly this problem.

The number of devices requiring PM_QOS users is still quite small.

> Using ACPI table based C-states by default and using intel_idle.enable=1
> or similar for workarounds sounds safer.
> At least as long as the driver is experimental.

I plan to remove the EXPERIMENTAL in 1 release.

> Does Windows use ACPI C-state info for idle?

Yes, Windows uses ACPI.
On the Dell above, that is why Linux consumes 15% less idle power
and why Linux can take advantage of turbo mode and Windows can not.

cheers,
Len Brown, Intel Open Source Technology Center

Re: [linux-pm] idle-test patches queued for upstream

[Thomas Renninger]

On Friday 28 May 2010 02:59:07 Len Brown wrote:
> > > ... we think we can do better than ACPI.
> 
> > Why exactly? Is there any info missing in the ACPI tables?
> > Or is this just to be more independent from OEMs?
> 
> ACPI has a few fundmental flaws here.  One is that it reports
> exit latency instead of break-even power duration.
> The other is that it requires a BIOS writer to
> get the tables right.
This is a general ACPI problem...
 
> > Using ACPI table based C-states by default and using
> > intel_idle.enable=1
> > or similar for workarounds sounds safer.
> > At least as long as the driver is experimental.
> 
> I plan to remove the EXPERIMENTAL in 1 release.
> 
> > Does Windows use ACPI C-state info for idle?

> Yes, Windows uses ACPI.
> On the Dell above, that is why Linux consumes 15% less idle power
> and why Linux can take advantage of turbo mode and Windows can not.
You always propageted to stay Windows compatible...
Now we go the untested way.
Let's see how much machines will break...

Thanks for clarifications,

    Thomas

Re: [linux-pm] idle-test patches queued for upstream

[Len Brown]

> > Yes, Windows uses ACPI.
> > On the Dell above, that is why Linux consumes 15% less idle power
> > and why Linux can take advantage of turbo mode and Windows can not.

> You always propageted to stay Windows compatible...
> Now we go the untested way.
> Let's see how much machines will break...
> 
> Thanks for clarifications,

Don't get me wrong, Linux' ACPI "compatibility" is a huge asset.

However, there are opportunities for Linux to do better than Windows,
and I believe this is one of them.  Yes, when we take the risk of
doing something that Windows does not do, there will always be
the possibility that we will fail.

cheers,
-Len Brown, Intel Open Source Technology Center

Re: [linux-pm] idle-test patches queued for upstream

[Pavel Machek]

On Thu 2010-05-27 20:59:07, Len Brown wrote:
> > > ... we think we can do better than ACPI.
> 
> > Why exactly? Is there any info missing in the ACPI tables?
> > Or is this just to be more independent from OEMs?
> 
> ACPI has a few fundmental flaws here.  One is that it reports
> exit latency instead of break-even power duration.
> The other is that it requires a BIOS writer to
> get the tables right.
> 
> Both of these are fatal flaws.

Intel is co-author of ACPI spec, right? So what about fixing those?
 
> > > Indeed, on my (production level commerically available) Nehalem desktop
> > > the ACPI tables are broken and an ACPI OS idles at 100W.  With this
> > > driver the box idles at 85W.
> 
> > What exactly was broken there?
> 
> Dell's BIOS developer botched a bug fix immediately before the system
> went to market and disabled support for all ACPI C-states except C1.
> After several month of shipping systems, they still were unable
> to ship them with a fixed BIOS.

I always thought that cpu vendors have ways to work with bios manufacturers...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html