| Message ID | 1340390327-2836-1-git-send-email-rajagopal.venkat@linaro.org |
|---|---|
| State | New |
| Headers | show |
Can someone consider this patch for merge? 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 > >
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. 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 > >
> 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? 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 >
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<http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-ux500/cpuidle.c;h=b54884bd254987f105d5f8ab0701b458ece1396e;hb=HEAD> 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 > > > >
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;
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(-)