2015-04-01 18:04:55

by Thomas Schlichter

[permalink] [raw]
Subject: [PATCH] fix sysfs showing the correct C-states after AC->DC and DC->AC transitions

Hello,

I do have a Samsung NC20 netbook which provides the C-states C1 and C2 to the
OS when connected to AC, and additionally provides the C3 C-state when
disconnected from AC. With the current kernels I have these two problems
(regressions):

1. The number of C-states shown in sysfs is fixed to the number of C-states
present at boot.
If I boot with AC connected, I always only see the C-states up to C2 even
if I disconnect AC.

The reason is commit 130a5f692425e6237229598a8624da0a247f33d5
"ACPI / cpuidle: remove dev->state_count setting". It removes the update of
dev->state_count, but sysfs uses exactly this variable to show the
C-states.

The fix is to use drv->state_count in sysfs. As this is currently the last
user of dev->state_count, this variable can be completely removed. And this
is exactly what Bartlomiej Zolnierkiewicz's not yet applied patch "[PATCH
v2 9/9] cpuidle: remove state_count field from struct cpuidle_device"
http://marc.info/?m=138756533317836 does.

Long story short: That patch fixes problem 1 for me.

2. After a AC->DC or DC->AC transition, the name and description of the
POLLING C-state become "<null>".

Here, the reason is commit d7c7f103262bc2248548ed0e113e916e843c4eeb
"cpuidle: don't call poll_idle_init() for every cpu". It only calls
poll_idle_init() during cpuidle_register_driver() instead of
cpuidle_enable_device() and thus does not re-initialize the fields of
drv->states[0] after acpi_processor_setup_cpuidle_states() cleared them.

Here, the fix is to _not_ clear drv->states[0] in
acpi_processor_setup_cpuidle_states(). For this purpose I created a small
patch.

For your convenience, both of the fixing patches are attached to this mail.
The first one is necessary for all kernels since 3.14, the second one for all
kernels since 3.13. So I'd propose to push both of them also to the necessary
stable kernels.

Kind regards,
Thomas


Attachments:
0001-cpuidle-remove-state_count-field-from-struct-cpuidle.patch (2.30 kB)
0002-ACPI-cpuidle-do-not-overwrite-name-and-description-o.patch (1.26 kB)
Download all attachments
Subject: Re: [PATCH] fix sysfs showing the correct C-states after AC->DC and DC->AC transitions


On Wednesday, April 01, 2015 08:04:34 PM Thomas Schlichter wrote:
> Hello,

Hi,

> I do have a Samsung NC20 netbook which provides the C-states C1 and C2 to the
> OS when connected to AC, and additionally provides the C3 C-state when
> disconnected from AC. With the current kernels I have these two problems
> (regressions):
>
> 1. The number of C-states shown in sysfs is fixed to the number of C-states
> present at boot.
> If I boot with AC connected, I always only see the C-states up to C2 even
> if I disconnect AC.
>
> The reason is commit 130a5f692425e6237229598a8624da0a247f33d5
> "ACPI / cpuidle: remove dev->state_count setting". It removes the update of
> dev->state_count, but sysfs uses exactly this variable to show the
> C-states.
>
> The fix is to use drv->state_count in sysfs. As this is currently the last
> user of dev->state_count, this variable can be completely removed. And this
> is exactly what Bartlomiej Zolnierkiewicz's not yet applied patch "[PATCH
> v2 9/9] cpuidle: remove state_count field from struct cpuidle_device"
> http://marc.info/?m=138756533317836 does.
>
> Long story short: That patch fixes problem 1 for me.

Rafael, please apply that patch (as Thomas has noticed it is required
to fix issue with dynamic dev/drv->state_count updates, sorry for not
catching it earlier).

[ Originally the whole series was dropped when patch #8 ("[PATCH v2 8/9]
intel_idle: use the common cpuidle_[un]register() routines") turned out
to introduce some problems. Patches #1-7 were later re-merged but not
patch #9 (which is independent of patch #8). ]

> 2. After a AC->DC or DC->AC transition, the name and description of the
> POLLING C-state become "<null>".
>
> Here, the reason is commit d7c7f103262bc2248548ed0e113e916e843c4eeb
> "cpuidle: don't call poll_idle_init() for every cpu". It only calls
> poll_idle_init() during cpuidle_register_driver() instead of
> cpuidle_enable_device() and thus does not re-initialize the fields of
> drv->states[0] after acpi_processor_setup_cpuidle_states() cleared them.
>
> Here, the fix is to _not_ clear drv->states[0] in
> acpi_processor_setup_cpuidle_states(). For this purpose I created a small
> patch.

This patch looks fine to me. FWIW:

Reviewed-by: Bartlomiej Zolnierkiewicz <[email protected]>

> For your convenience, both of the fixing patches are attached to this mail.
> The first one is necessary for all kernels since 3.14, the second one for all
> kernels since 3.13. So I'd propose to push both of them also to the necessary
> stable kernels.

Sounds good to me. Thank you for working on this.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics