cpuidle: fix new C-states not functional after AC disconnect

Hi,

there is a long-standing regression about new C-states not working after disconnecting AC power from a laptop if the cpuidle driver "acpi-idle" is used. It was reported here:

[1] https://bugzilla.kernel.org/show_bug.cgi?id=42870 (March 5th 2012)
[2] https://bugzilla.kernel.org/show_bug.cgi?id=43349 (June 7th 2012)
[3] https://lkml.org/lkml/2012/10/16/518 (October 19th 2012)

In [1] Andreas proposed a patch that initialized the missing power_usage values from within acpi_idle in the same way as cpuidle does. In [2] I proposed a patch to use the power values provided by ACPI to initialize the power_usage variables.
In [3] Julius proposed a patch to call the initialization function set_power_states() not only once, but always when the C-states change.

Currently, Daniel Lezcano seems to be working on an intrusive change of not using the power_usage value at all for choosing a C-state:

[4] https://lkml.org/lkml/2012/12/14/155

As I could not find any of these patches in any git trees to be merged for 3.8, I propose an other, least intrusive patch for the time being. It is attached an initializes _all_ power_usage values in the first place.

As this is a real power consumption regression since 3.2, I really ask you to apply anything and push it to stable, too!

Kind regards,
Thomas Schlichter

From: Thomas Schlichter <[email protected]>
Date: Sun, 2 Sep 2012 14:35:34 +0200
Subject: [PATCH 1/2] Get power info before updating the C-states

acpi_processor_get_power_info() has to be called before acpi_processor_setup_cpuidle_states() to have the latest information available. This fixes the missing C-state information after AC-->DC transition.

---
 drivers/acpi/processor_idle.c |    1 +
 1 file changed, 1 insertion(+)

index f1a5da4..17ed60b 100644
@@ -1149,6 +1149,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 		}
 
 		/* Populate Updated C-state information */
+		acpi_processor_get_power_info(pr);
 		acpi_processor_setup_cpuidle_states(pr);
 
 		/* Enable all cpuidle devices */

1.7.10.4

From: Thomas Schlichter <[email protected]>
Date: Sat, 12 Jan 2013 15:30:43 +0100
Subject: [PATCH 2/2] cpuidle: Initialize power_usage for all C-states

By now, only the power_usage values of currently available C-states are initialized in the function set_power_states(). But if the number of C-states increases later (e.g. when using acpi_idle and unplugging the AC power from the laptop) the power_usage values of the new C-states stay uninitialized.

Fix this by initializing all CPUIDLE_STATE_MAX C-stages in the first place.

---
 drivers/cpuidle/driver.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c2b281a..960d98d 100644
@@ -37,7 +37,7 @@ static void set_power_states(struct cpuidle_driver *drv)
 	 * an power value of -1.  So we use -2, -3, etc, for other
 	 * c-states.
 	 */
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++)
+	for (i = CPUIDLE_DRIVER_STATE_START; i < CPUIDLE_STATE_MAX; i++)
 		drv->states[i].power_usage = -1 - i;
 }
 

1.7.10.4

Rafael, is possible to apply the patch [1/2] I previously sent ?

https://patchwork.kernel.org/patch/1878691/

So we get this bug fixed.

I will resend the patch [2/2] as soon as possible.

-- Daniel

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I need to talk about this with Len. That should happen tomorrow if everything goes well.

OK

Thanks,
Rafael

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Dear Rafael,

thank you for applying Daniel's patch. I tested 3.8-rc4 and found that one patch ist still missing to fix the problem of not usable C-state after disconnect. I had it attached with my last e-mail as patch 1. For your conveniency, I have attached it here again.

With that patch the problem is fixed for me. So please consider applying this, too.

Kind regards,
Thomas

Am Sonntag, 13. Januar 2013, 21:04:45 schrieb Rafael J. Wysocki:

From: Thomas Schlichter <[email protected]>
Date: Sun, 2 Sep 2012 14:35:34 +0200
Subject: [PATCH] Get power info before updating the C-states

acpi_processor_get_power_info() has to be called before acpi_processor_setup_cpuidle_states() to have the latest information available. This fixes the missing C-state information after AC-->DC transition.

---
 drivers/acpi/processor_idle.c |    1 +
 1 file changed, 1 insertion(+)

index f1a5da4..17ed60b 100644
@@ -1149,6 +1149,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 		}
 
 		/* Populate Updated C-state information */
+		acpi_processor_get_power_info(pr);
 		acpi_processor_setup_cpuidle_states(pr);
 
 		/* Enable all cpuidle devices */

1.7.10.4

Dear Rafael,

I just recognized that I did not really describe the problem that my patch fixes. Therefore I attached the output of following command after disconnecting the AC power without and with my patch:

grep -r "" /sys/devices/system/cpu/cpu0/cpuidle/

One can clearly see that some information is missing and the C3 state is not used.

Kind regards,
Thomas

Am Freitag, 18. Januar 2013, 21:34:29 schrieb Thomas Schlichter:

/sys/devices/system/cpu/cpu0/cpuidle/state0/desc:CPUIDLE CORE POLL IDLE /sys/devices/system/cpu/cpu0/cpuidle/state0/name:POLL /sys/devices/system/cpu/cpu0/cpuidle/state0/time:0 /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state0/power:4294967295 /sys/devices/system/cpu/cpu0/cpuidle/state0/usage:0 /sys/devices/system/cpu/cpu0/cpuidle/state0/latency:0 /sys/devices/system/cpu/cpu0/cpuidle/state1/desc:ACPI HLT /sys/devices/system/cpu/cpu0/cpuidle/state1/name:C1 /sys/devices/system/cpu/cpu0/cpuidle/state1/time:147579 /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state1/power:0 /sys/devices/system/cpu/cpu0/cpuidle/state1/usage:42 /sys/devices/system/cpu/cpu0/cpuidle/state1/latency:1 /sys/devices/system/cpu/cpu0/cpuidle/state2/desc:ACPI IOPORT 0x4014 /sys/devices/system/cpu/cpu0/cpuidle/state2/name:C2 /sys/devices/system/cpu/cpu0/cpuidle/state2/time:36250374 /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state2/power:0 /sys/devices/system/cpu/cpu0/cpuidle/state2/usage:2867 /sys/devices/system/cpu/cpu0/cpuidle/state2/latency:1 /sys/devices/system/cpu/cpu0/cpuidle/state3/desc:<null> /sys/devices/system/cpu/cpu0/cpuidle/state3/name:<null> /sys/devices/system/cpu/cpu0/cpuidle/state3/time:0 /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state3/power:0 /sys/devices/system/cpu/cpu0/cpuidle/state3/usage:0 /sys/devices/system/cpu/cpu0/cpuidle/state3/latency:0

/sys/devices/system/cpu/cpu0/cpuidle/state0/desc:CPUIDLE CORE POLL IDLE /sys/devices/system/cpu/cpu0/cpuidle/state0/name:POLL /sys/devices/system/cpu/cpu0/cpuidle/state0/time:0 /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state0/power:4294967295 /sys/devices/system/cpu/cpu0/cpuidle/state0/usage:0 /sys/devices/system/cpu/cpu0/cpuidle/state0/latency:0 /sys/devices/system/cpu/cpu0/cpuidle/state1/desc:ACPI HLT /sys/devices/system/cpu/cpu0/cpuidle/state1/name:C1 /sys/devices/system/cpu/cpu0/cpuidle/state1/time:4644 /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state1/power:0 /sys/devices/system/cpu/cpu0/cpuidle/state1/usage:35 /sys/devices/system/cpu/cpu0/cpuidle/state1/latency:1 /sys/devices/system/cpu/cpu0/cpuidle/state2/desc:ACPI IOPORT 0x4014 /sys/devices/system/cpu/cpu0/cpuidle/state2/name:C2 /sys/devices/system/cpu/cpu0/cpuidle/state2/time:44430 /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state2/power:0 /sys/devices/system/cpu/cpu0/cpuidle/state2/usage:52 /sys/devices/system/cpu/cpu0/cpuidle/state2/latency:1 /sys/devices/system/cpu/cpu0/cpuidle/state3/desc:ACPI IOPORT 0x4016 /sys/devices/system/cpu/cpu0/cpuidle/state3/name:C3 /sys/devices/system/cpu/cpu0/cpuidle/state3/time:25798363 /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:0 /sys/devices/system/cpu/cpu0/cpuidle/state3/power:0 /sys/devices/system/cpu/cpu0/cpuidle/state3/usage:1826 /sys/devices/system/cpu/cpu0/cpuidle/state3/latency:146

Hi Rafael,

Have you made a decision on picking up Daniel's first change yet? I think it really is the cleanest way to solve this problem.

Yes, it should be in 3.8-rc already, isn't it there?

Rafael

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.