On 1 August 2012 03:34, Chris Ferron <chris.e.ferron@linux.intel.com> wrote:

> This patch fixes powertop to display cpuidle states on platforms
> where cpuidle stateX directory name field does not contain
> "CX" string. On some platforms, the name field contains meaningful
> strings like WFI, Sleep, DeepSleep.
>
> Please review the patch.
>

Im not sure this is doing what you think it is, and if it is doing
something; then i fear its an weird artifact of something else.

I assume when you say "some platforms" you mean "ARM"?
Can you detail out with data what the bug is you are having, along with
expected results?

This patch is not specific to ARM but for any platform on which cpuidle
state name filed doesn't follow "CX" string pattern. For instance the
snowball cpuidle states are defined as follows[1]
{
        .enter                  = arm_cpuidle_simple_enter,
        ......
        .name                   = "WFI",
        .desc                   = "ARM WFI",
},
{
        .enter       = ux500_enter_idle,
        ......
        .name       = "ApIdle",
        .desc        = "ARM Retention",
},

Now, while parsing for C states, powertop is using .name field(human_name)
for detecting supported cpuidle state levels. This is done with an assumption
that, name field value contains "CX" substring, which is not true in above case.
Infact, I could find good number of cpuidle drivers of this kind in mainline kernel.

None of above cpuidle states should get added in insert_cstate() function.
Fortunately, the deepest idle state is added as C0 state because of
memset(state, 0, sizeof(*state));. Hence powertop displays only one
idle state.

Parsing idle states based on directory name stateX(linux_name) is safe.
This patch does that.

[1] arch/arm/mach-ux500/cpuidle.c

What is your test platform, kernel version, etc.
By your comment of what fields contents, I am betting there is more work
to be done here
then just what you sent regardless.
Im not going to take this patch at this time, until I understand the full
details.

Please be verbose, I have no ARM platforms to experiment or test with.

-Chris

> On 23 June 2012 00:08, Rajagopal Venkat <rajagopal.venkat@linaro.org>
> wrote:
>
>> parse cpuidle C state based on sysfs file entry(stateX)
>> instead of state name/description
>>
>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
>> ---
>> src/cpu/abstract_cpu.cpp | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/cpu/abstract_cpu.cpp b/src/cpu/abstract_cpu.cpp
>> index cd4eba0..72969fc 100644
>> --- a/src/cpu/abstract_cpu.cpp
>> +++ b/src/cpu/abstract_cpu.cpp
>> @@ -147,7 +147,7 @@ void abstract_cpu::insert_cstate(const char
>> *linux_name, const char *human_name,
>> strcpy(state->linux_name, linux_name);
>> strcpy(state->human_name, human_name);
>>
>> - c = human_name;
>> + c = linux_name;
>> while (*c) {
>> if (strcmp(linux_name, "active")==0) {
>> state->line_level = LEVEL_C0;
>> --
>> 1.7.9.5
>>
>>

> _______________________________________________
> PowerTop mailing list
> PowerTop@lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>