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

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

[Thomas Schlichter]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

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

[-- Attachment #2: 0001-Get-power-info-before-updating-the-C-states.patch --]
[-- Type: text/x-patch, Size: 1016 bytes --]

>From 5296fec02cfadf10daf6395ad6b55075381b2439 Mon Sep 17 00:00:00 2001
From: Thomas Schlichter <thomas.schlichter@web.de>
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.

Signed-off-by: Thomas Schlichter <thomas.schlichter@web.de>
---
 drivers/acpi/processor_idle.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index f1a5da4..17ed60b 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -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

[-- Attachment #3: 0002-cpuidle-Initialize-power_usage-for-all-C-states.patch --]
[-- Type: text/x-patch, Size: 1264 bytes --]

>From 5c635b3530d4f9b41811f7dde12e0d6f5f7dcc54 Mon Sep 17 00:00:00 2001
From: Thomas Schlichter <thomas.schlichter@web.de>
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.

Signed-off-by: Thomas Schlichter <thomas.schlichter@web.de>
---
 drivers/cpuidle/driver.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index c2b281a..960d98d 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -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

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Daniel Lezcano]

On 01/13/2013 01:34 PM, Thomas Schlichter wrote:
> 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!

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

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Sedat Dilek]

Hi Thomas,

I read your original LKML posting in [1].

[ QUOTE ]
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...
[ /QUOTE ]

You can find the patches at LKML ([2] and [3]) or patchwork ([4] and [5]).

As a Linux-Next "customer" I know about Daniel's work for-next [5] but
I did not found any other GIT repository of him.
Striving through Daniel's other GIT repos I found idlestat [6] and
powerdebug [7] tools which I want to test.
( So, thank you for your email! )

Have fun!

Regards,
- Sedat -

[0] http://marc.info/?l=linux-acpi&m=135808048905350&w=2
[1] https://lkml.org/lkml/2012/12/14/153
[2] https://lkml.org/lkml/2012/12/14/149
[3] https://patchwork.kernel.org/patch/1878691/
[4] https://patchwork.kernel.org/patch/1878701/
[5] http://git.linaro.org/gitweb?p=people/dlezcano/cpuidle-next.git
[6] http://git.linaro.org/gitweb?p=people/dlezcano/idlestat.git;a=summary
[7] http://git.linaro.org/gitweb?p=people/dlezcano/powerdebug.git;a=summary

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Rafael J. Wysocki]

On Sunday, January 13, 2013 03:41:34 PM Daniel Lezcano wrote:
> On 01/13/2013 01:34 PM, Thomas Schlichter wrote:
> > 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!
> 
> Rafael, is possible to apply the patch [1/2] I previously sent ?
> 
> https://patchwork.kernel.org/patch/1878691/

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

> So we get this bug fixed.
> 
> I will resend the patch [2/2] as soon as possible.

OK

Thanks,
Rafael


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

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Sedat Dilek]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

0001: Refreshed 1-2 as v3 against Linux v3.8-rc3.
0002: v2 of 2-2 applied cleanly after 1-2 was refreshed!

Have fun!

- Sedat -

On Sun, Jan 13, 2013 at 8:30 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> Hi Thomas,
>
> I read your original LKML posting in [1].
>
> [ QUOTE ]
> 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...
> [ /QUOTE ]
>
> You can find the patches at LKML ([2] and [3]) or patchwork ([4] and [5]).
>
> As a Linux-Next "customer" I know about Daniel's work for-next [5] but
> I did not found any other GIT repository of him.
> Striving through Daniel's other GIT repos I found idlestat [6] and
> powerdebug [7] tools which I want to test.
> ( So, thank you for your email! )
>
> Have fun!
>
> Regards,
> - Sedat -
>
> [0] http://marc.info/?l=linux-acpi&m=135808048905350&w=2
> [1] https://lkml.org/lkml/2012/12/14/153
> [2] https://lkml.org/lkml/2012/12/14/149
> [3] https://patchwork.kernel.org/patch/1878691/
> [4] https://patchwork.kernel.org/patch/1878701/
> [5] http://git.linaro.org/gitweb?p=people/dlezcano/cpuidle-next.git
> [6] http://git.linaro.org/gitweb?p=people/dlezcano/idlestat.git;a=summary
> [7] http://git.linaro.org/gitweb?p=people/dlezcano/powerdebug.git;a=summary

[-- Attachment #2: 0001-cpuidle-remove-the-power_specified-field-in-the-driv.patch --]
[-- Type: application/octet-stream, Size: 5420 bytes --]

From afd71a6f5e11d074053fb9dd687ff2ce19767e6b Mon Sep 17 00:00:00 2001
From: Sedat Dilek <sedat.dilek@gmail.com>
Date: Sun, 13 Jan 2013 21:17:41 +0100
Subject: [PATCH 1/2] cpuidle - remove the power_specified field in the driver

From: Daniel Lezcano <daniel.lezcano@linaro.org>

This patch follows the discussion about reinitializing the power usage
when a C-state is added/removed.

 https://lkml.org/lkml/2012/10/16/518

We realized the power usage field is never filled and when it is
filled for tegra, the power_specified flag is not set making all these
values to be resetted when the driver is initialized with the set_power_state
function.

Julius and I feel this is over-engineered and the power_specified
flag could be simply removed and continue assuming the states are
backward sorted.

The menu governor select function is simplified as the power is ordered.
Actually the condition is always true with the current code.

The cpuidle_play_dead function is also simplified by doing a reverse lookup
on the array.

The set_power_states function is removed as it does no make sense anymore.

As a consequence, this patch fix also the bug where on the dynamic C-state
system, the power fields is not initialized.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Julius Werner <jwerner@chromium.org>

[ dileks: v3: Refreshed original patch against Linux v3.8-rc3. ]
---
 drivers/cpuidle/cpuidle.c        |   17 ++++-------------
 drivers/cpuidle/driver.c         |   25 -------------------------
 drivers/cpuidle/governors/menu.c |    8 ++------
 include/linux/cpuidle.h          |    2 +-
 4 files changed, 7 insertions(+), 45 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index fb4a7dd..e1f6860 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -69,24 +69,15 @@ int cpuidle_play_dead(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
-	int i, dead_state = -1;
-	int power_usage = INT_MAX;
+	int i;
 
 	if (!drv)
 		return -ENODEV;
 
 	/* Find lowest-power state that supports long-term idle */
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
-		struct cpuidle_state *s = &drv->states[i];
-
-		if (s->power_usage < power_usage && s->enter_dead) {
-			power_usage = s->power_usage;
-			dead_state = i;
-		}
-	}
-
-	if (dead_state != -1)
-		return drv->states[dead_state].enter_dead(dev, dead_state);
+	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--)
+		if (drv->states[i].enter_dead)
+			return drv->states[i].enter_dead(dev, i);
 
 	return -ENODEV;
 }
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
index c2b281a..422c7b6 100644
--- a/drivers/cpuidle/driver.c
+++ b/drivers/cpuidle/driver.c
@@ -19,34 +19,9 @@ DEFINE_SPINLOCK(cpuidle_driver_lock);
 static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu);
 static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu);
 
-static void set_power_states(struct cpuidle_driver *drv)
-{
-	int i;
-
-	/*
-	 * cpuidle driver should set the drv->power_specified bit
-	 * before registering if the driver provides
-	 * power_usage numbers.
-	 *
-	 * If power_specified is not set,
-	 * we fill in power_usage with decreasing values as the
-	 * cpuidle code has an implicit assumption that state Cn
-	 * uses less power than C(n-1).
-	 *
-	 * With CONFIG_ARCH_HAS_CPU_RELAX, C0 is already assigned
-	 * 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++)
-		drv->states[i].power_usage = -1 - i;
-}
-
 static void __cpuidle_driver_init(struct cpuidle_driver *drv)
 {
 	drv->refcnt = 0;
-
-	if (!drv->power_specified)
-		set_power_states(drv);
 }
 
 static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu)
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 20ea33a..fe343a0 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -312,7 +312,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 {
 	struct menu_device *data = &__get_cpu_var(menu_devices);
 	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
-	int power_usage = INT_MAX;
 	int i;
 	int multiplier;
 	struct timespec t;
@@ -383,11 +382,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		if (s->exit_latency * multiplier > data->predicted_us)
 			continue;
 
-		if (s->power_usage < power_usage) {
-			power_usage = s->power_usage;
-			data->last_state_idx = i;
-			data->exit_us = s->exit_latency;
-		}
+		data->last_state_idx = i;
+		data->exit_us = s->exit_latency;
 	}
 
 	/* not deepest C-state chosen for low predicted residency */
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 3711b34..24cd103 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -126,9 +126,9 @@ struct cpuidle_driver {
 	struct module 		*owner;
 	int                     refcnt;
 
-	unsigned int		power_specified:1;
 	/* set to 1 to use the core cpuidle time keeping (for all states). */
 	unsigned int		en_core_tk_irqen:1;
+	/* states array must be ordered in decreasing power consumption */
 	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
 	int			state_count;
 	int			safe_state_index;
-- 
1.7.9.5


[-- Attachment #3: 0002-cpuidle-optimize-the-select-function-for-the-menu-go.patch --]
[-- Type: application/octet-stream, Size: 1902 bytes --]

From 5a52395af9f30aadd5a81813fd9a69dd3ab2df8f Mon Sep 17 00:00:00 2001
From: Daniel Lezcano <daniel.lezcano@linaro.org>
Date: Fri, 14 Dec 2012 13:57:35 +0000
Subject: [PATCH 2/2] cpuidle - optimize the select function for the 'menu'
 governor

As the power is backward sorted in the states array and we are looking for
the state consuming the little power as possible, instead of looking from
the beginning of the array, we look from the end. That should save us some
iterations in the loop each time we select a state at idle time.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governors/menu.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index fe343a0..05b8998 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -367,24 +367,24 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	 * Find the idle state with the lowest power while satisfying
 	 * our constraints.
 	 */
-	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
+	for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 
 		if (s->disabled || su->disable)
 			continue;
-		if (s->target_residency > data->predicted_us) {
-			low_predicted = 1;
-			continue;
-		}
 		if (s->exit_latency > latency_req)
 			continue;
+		if (s->target_residency > data->predicted_us)
+			continue;
 		if (s->exit_latency * multiplier > data->predicted_us)
 			continue;
 
+		low_predicted = i - CPUIDLE_DRIVER_STATE_START;
 		data->last_state_idx = i;
 		data->exit_us = s->exit_latency;
-	}
+		break;
+       }
 
 	/* not deepest C-state chosen for low predicted residency */
 	if (low_predicted) {
-- 
1.7.9.5

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Daniel Lezcano]

On 01/13/2013 09:36 PM, Sedat Dilek wrote:
> 0001: Refreshed 1-2 as v3 against Linux v3.8-rc3.
> 0002: v2 of 2-2 applied cleanly after 1-2 was refreshed!

Hi Sedat,

for the moment, you should use only the 1/2 because 2/2 (which is an
optimization) is wrong.

Regards.

  -- Daniel

> Have fun!
> 
> - Sedat -
> 
> On Sun, Jan 13, 2013 at 8:30 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>> Hi Thomas,
>>
>> I read your original LKML posting in [1].
>>
>> [ QUOTE ]
>> 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...
>> [ /QUOTE ]
>>
>> You can find the patches at LKML ([2] and [3]) or patchwork ([4] and [5]).
>>
>> As a Linux-Next "customer" I know about Daniel's work for-next [5] but
>> I did not found any other GIT repository of him.
>> Striving through Daniel's other GIT repos I found idlestat [6] and
>> powerdebug [7] tools which I want to test.
>> ( So, thank you for your email! )
>>
>> Have fun!
>>
>> Regards,
>> - Sedat -
>>
>> [0] http://marc.info/?l=linux-acpi&m=135808048905350&w=2
>> [1] https://lkml.org/lkml/2012/12/14/153
>> [2] https://lkml.org/lkml/2012/12/14/149
>> [3] https://patchwork.kernel.org/patch/1878691/
>> [4] https://patchwork.kernel.org/patch/1878701/
>> [5] http://git.linaro.org/gitweb?p=people/dlezcano/cpuidle-next.git
>> [6] http://git.linaro.org/gitweb?p=people/dlezcano/idlestat.git;a=summary
>> [7] http://git.linaro.org/gitweb?p=people/dlezcano/powerdebug.git;a=summary


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

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Sedat Dilek]

On Sun, Jan 13, 2013 at 9:44 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 01/13/2013 09:36 PM, Sedat Dilek wrote:
>> 0001: Refreshed 1-2 as v3 against Linux v3.8-rc3.
>> 0002: v2 of 2-2 applied cleanly after 1-2 was refreshed!
>
> Hi Sedat,
>
> for the moment, you should use only the 1/2 because 2/2 (which is an
> optimization) is wrong.
>

Thanks, I wanted to test this cpuidle-fixes right now.

Just FYI:
Your original 1-2 (v2) patch does ***NOT*** apply against Linux v3.8-rc3!
I have sent a v3 already to the MLs.

- Sedat -

> Regards.
>
>   -- Daniel
>
>> Have fun!
>>
>> - Sedat -
>>
>> On Sun, Jan 13, 2013 at 8:30 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>> Hi Thomas,
>>>
>>> I read your original LKML posting in [1].
>>>
>>> [ QUOTE ]
>>> 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...
>>> [ /QUOTE ]
>>>
>>> You can find the patches at LKML ([2] and [3]) or patchwork ([4] and [5]).
>>>
>>> As a Linux-Next "customer" I know about Daniel's work for-next [5] but
>>> I did not found any other GIT repository of him.
>>> Striving through Daniel's other GIT repos I found idlestat [6] and
>>> powerdebug [7] tools which I want to test.
>>> ( So, thank you for your email! )
>>>
>>> Have fun!
>>>
>>> Regards,
>>> - Sedat -
>>>
>>> [0] http://marc.info/?l=linux-acpi&m=135808048905350&w=2
>>> [1] https://lkml.org/lkml/2012/12/14/153
>>> [2] https://lkml.org/lkml/2012/12/14/149
>>> [3] https://patchwork.kernel.org/patch/1878691/
>>> [4] https://patchwork.kernel.org/patch/1878701/
>>> [5] http://git.linaro.org/gitweb?p=people/dlezcano/cpuidle-next.git
>>> [6] http://git.linaro.org/gitweb?p=people/dlezcano/idlestat.git;a=summary
>>> [7] http://git.linaro.org/gitweb?p=people/dlezcano/powerdebug.git;a=summary
>
>
> --
>  <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
>

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Thomas Schlichter]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

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:
> On Sunday, January 13, 2013 03:41:34 PM Daniel Lezcano wrote:
> > On 01/13/2013 01:34 PM, Thomas Schlichter wrote:
> > > 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!
> > 
> > Rafael, is possible to apply the patch [1/2] I previously sent ?
> > 
> > https://patchwork.kernel.org/patch/1878691/
> 
> I need to talk about this with Len.  That should happen tomorrow if
> everything goes well.
> 
> > So we get this bug fixed.
> > 
> > I will resend the patch [2/2] as soon as possible.
> 
> OK
> 
> Thanks,
> Rafael

[-- Attachment #2: 0001-Get-power-info-before-updating-the-C-states.patch --]
[-- Type: text/x-patch, Size: 1013 bytes --]

>From fc300f4fefff59fe1029bf7852ea32d957145821 Mon Sep 17 00:00:00 2001
From: Thomas Schlichter <thomas.schlichter@web.de>
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.

Signed-off-by: Thomas Schlichter <thomas.schlichter@web.de>
---
 drivers/acpi/processor_idle.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index f1a5da4..17ed60b 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -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

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Thomas Schlichter]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

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:
> 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:
> > On Sunday, January 13, 2013 03:41:34 PM Daniel Lezcano wrote:
> > > On 01/13/2013 01:34 PM, Thomas Schlichter wrote:
> > > > 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!
> > > 
> > > Rafael, is possible to apply the patch [1/2] I previously sent ?
> > > 
> > > https://patchwork.kernel.org/patch/1878691/
> > 
> > I need to talk about this with Len.  That should happen tomorrow if
> > everything goes well.
> > 
> > > So we get this bug fixed.
> > > 
> > > I will resend the patch [2/2] as soon as possible.
> > 
> > OK
> > 
> > Thanks,
> > Rafael

[-- Attachment #2: cpuidle_3.8-rc4.txt --]
[-- Type: text/plain, Size: 1545 bytes --]

/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

[-- Attachment #3: cpuidle_3.8-rc4_patched.txt --]
[-- Type: text/plain, Size: 1558 bytes --]

/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

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Thomas Schlichter]

Am Sonntag, 13. Januar 2013, 21:44:41 schrieb Daniel Lezcano:
> On 01/13/2013 09:36 PM, Sedat Dilek wrote:
> > 0001: Refreshed 1-2 as v3 against Linux v3.8-rc3.
> > 0002: v2 of 2-2 applied cleanly after 1-2 was refreshed!
> 
> Hi Sedat,
> 
> for the moment, you should use only the 1/2 because 2/2 (which is an
> optimization) is wrong.

Hi Daniel,

thanks again for this patch, this together with my patch finally fix the bug.
Now I recognized that only my patch was also sent to stable (thanks Rafael), 
yours not. So the bug is not completely fixed in 3.4 and 3.7.

Is there a reason for not sending this to stable, too?

Kind regards,
  Thomas

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Julius Werner]

>> Rafael, is possible to apply the patch [1/2] I previously sent ?
>>
>> https://patchwork.kernel.org/patch/1878691/
>
> I need to talk about this with Len.  That should happen tomorrow if everything
> goes well.

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.

Re: [PATCH] cpuidle: fix new C-states not functional after AC disconnect

[Rafael J. Wysocki]

On Wednesday, January 30, 2013 07:52:30 PM Julius Werner wrote:
> >> Rafael, is possible to apply the patch [1/2] I previously sent ?
> >>
> >> https://patchwork.kernel.org/patch/1878691/
> >
> > I need to talk about this with Len.  That should happen tomorrow if everything
> > goes well.
> 
> 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.