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

Previous thread: [Patch-next] Remove notify_die in do_machine_check functioin by Jin Dongming on Wednesday, May 26, 2010 - 7:40 pm. (8 messages)

Next thread: [PATCH 7/8] ACPI: acpi_idle: touch TS_POLLING only in the non-MWAIT case by Len Brown on Wednesday, May 26, 2010 - 7:42 pm. (1 message)
From: Len Brown
Date: Wednesday, May 26, 2010 - 7:42 pm

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 ...
From: Andrew Morton
Date: Wednesday, May 26, 2010 - 8:44 pm

(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



There's been a half-hearted attempt to describe sysfs files in

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




__init?

The patch adds trailing whitespace.  Has various other checkpatch

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

--

From: Len Brown
Date: Thursday, May 27, 2010 - 8:57 pm

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

yes, that was a bug!

thanks,
Len Brown, Intel Open Source Technology Center

--

From: Andi Kleen
Date: Sunday, May 30, 2010 - 2:20 am

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.
--

From: Thomas Renninger
Date: Thursday, May 27, 2010 - 1:53 am

Then it should better be declared bool instead of tristate until it
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
--

From: Len Brown
Date: Thursday, May 27, 2010 - 6:44 pm

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

It means less power savings savings than optimal

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
--

From: Thomas Renninger
Date: Friday, May 28, 2010 - 12:46 am

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

Thanks,

    Thomas

--

From: Len Brown
Date: Friday, May 28, 2010 - 10:38 am

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


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

--

From: Thomas Renninger
Date: Friday, May 28, 2010 - 9:17 pm

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 ...
From: Kevin Hilman
Date: Thursday, May 27, 2010 - 7:14 am

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
--

From: Arjan van de Ven
Date: Thursday, May 27, 2010 - 7:22 am

On Thu, 27 May 2010 07:14:46 -0700

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
--

From: Kevin Hilman
Date: Thursday, May 27, 2010 - 7:36 am

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

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
--

From: Len Brown
Date: Thursday, May 27, 2010 - 5:22 pm

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
--

From: Kevin Hilman
Date: Friday, May 28, 2010 - 10:28 am

OK, fair enough.

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

Kevin


--

From: Igor Stoppa
Date: Thursday, May 27, 2010 - 7:51 am

Hi,




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

--

From: Arjan van de Ven
Date: Thursday, May 27, 2010 - 8:14 pm

On Thu, 27 May 2010 17:51:00 +0300

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

--

From: Kevin Hilman
Date: Friday, May 28, 2010 - 10:27 am

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


--

From: Arjan van de Ven
Date: Friday, May 28, 2010 - 5:38 pm

On Fri, 28 May 2010 10:27:32 -0700

yes.



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

From: Chase Douglas
Date: Thursday, May 27, 2010 - 7:32 pm

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

--

From: Len Brown
Date: Thursday, May 27, 2010 - 9:16 pm

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

--

From: Chase Douglas
Date: Friday, May 28, 2010 - 8:09 am

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

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

-- Chase

--

From: Len Brown
Date: Friday, May 28, 2010 - 10:43 am

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

cheers,
-Len

--

From: Chase Douglas
Date: Friday, May 28, 2010 - 12:51 pm

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

--

From: Chase Douglas
Date: Friday, May 28, 2010 - 1:14 pm

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

--

Previous thread: [Patch-next] Remove notify_die in do_machine_check functioin by Jin Dongming on Wednesday, May 26, 2010 - 7:40 pm. (8 messages)

Next thread: [PATCH 7/8] ACPI: acpi_idle: touch TS_POLLING only in the non-MWAIT case by Len Brown on Wednesday, May 26, 2010 - 7:42 pm. (1 message)