Skip to contentSkip to conversation

Properly fix sysfs_get_idlestate_count()

PATCH In review Prarit Bhargava 3 participants 5 messages Dec 14, 2014 – Dec 17, 2014 CPU power monitoring

lore
Share & tools
lore
#0 lore raw

cover Properly fix sysfs_get_idlestate_count()

My previous commit 16b7c275c055 ("tools: cpupower: fix return checks for sysfs_get_idlestate_count()") was not correct. After looking
at the changelog for cpupower I noticed that Thomas had changed the return of sysfs_get_idlestate_count() to an unsigned int to simplify the code. The problem is really that both he (in his original change) and I (in my new change) missed the obvious that sysfs_get_idlestate_count()
can't return -ENODEV. It should just return 0 for "no c-states".

Patch 1 reverts my recent change and patch 2 fixes the problem correctly.

Sorry 'bout that Thomas. I should have caught that the the first time.

Cc Thomas Renninger <[email protected]>
Cc Rafael J. Wysocki <[email protected]>

Prarit Bhargava (2):
Revert "tools: cpupower: fix return checks for
sysfs_get_idlestate_count()"
Fix no idle state information return value

 tools/power/cpupower/utils/cpuidle-info.c  |    8 ++++----
 tools/power/cpupower/utils/helpers/sysfs.c |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)
Signature · 1 line
1.7.9.3
#1 lore raw

PATCH 1/2 Revert "tools: cpupower: fix return checks for sysfs_get_idlestate_count()"

This reverts commit 16b7c275c055cc36218404b5d147be7f76575087.

My previous commit 16b7c275c055 ("tools: cpupower: fix return checks for sysfs_get_idlestate_count()") was not correct. After looking
at the changelog for cpupower I noticed that Thomas had changed the return of sysfs_get_idlestate_count() to an unsigned int to simplify the code. The problem is really that both he (in his original change) and I (in my new change) missed the obvious that sysfs_get_idlestate_count()
can't return -ENODEV. It should just return 0 for "no c-states".

Cc Thomas Renninger <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
---
 tools/power/cpupower/utils/cpuidle-info.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
tools/power/cpupower/utils/cpuidle-info.c +4 -4
index 458d69b..75e66de 100644
@@ -22,13 +22,13 @@
 
 static void cpuidle_cpu_output(unsigned int cpu, int verbose)
 {
-	int idlestates, idlestate;
+	unsigned int idlestates, idlestate;
 	char *tmp;
 
 	printf(_ ("Analyzing CPU %d:\n"), cpu);
 
 	idlestates = sysfs_get_idlestate_count(cpu);
-	if (idlestates < 1) {
+	if (idlestates == 0) {
 		printf(_("CPU %u: No idle states\n"), cpu);
 		return;
 	}
@@ -100,10 +100,10 @@ static void cpuidle_general_output(void)
 static void proc_cpuidle_cpu_output(unsigned int cpu)
 {
 	long max_allowed_cstate = 2000000000;
-	int cstate, cstates;
+	unsigned int cstate, cstates;
 
 	cstates = sysfs_get_idlestate_count(cpu);
-	if (cstates < 1) {
+	if (cstates == 0) {
 		printf(_("CPU %u: No C-states info\n"), cpu);
 		return;
 	}
Signature · 1 line
1.7.9.3
#2 lore raw

PATCH 2/2 Fix no idle state information return value

In reply to Prarit Bhargava

sysfs_get_idlestate_count() returns an unsigned int. Returning -ENODEV is not the right thing to do here, and in any case is handled the same way as if there are no states found.

Cc Thomas Renninger <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
---
 tools/power/cpupower/utils/helpers/sysfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/power/cpupower/utils/helpers/sysfs.c +1 -1
index 09afe5d..4e8fe2c 100644
@@ -361,7 +361,7 @@ unsigned int sysfs_get_idlestate_count(unsigned int cpu)
 
 	snprintf(file, SYSFS_PATH_MAX, PATH_TO_CPU "cpuidle");
 	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
-		return -ENODEV;
+		return 0;
 
 	snprintf(file, SYSFS_PATH_MAX, PATH_TO_CPU "cpu%u/cpuidle/state0", cpu);
 	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
Signature · 1 line
1.7.9.3
Acked-by on v1 2/2
#3 lore raw

Re: [PATCH 2/2] Fix no idle state information return value

Quoted 3 lines · Prarit Bhargava

sysfs_get_idlestate_count() returns an unsigned int. Returning -ENODEV is not the right thing to do here, and in any case is handled the same way as if there are no states found.

Yep, returning zero states instead of an error code makes a lot sense here.

Quoted 4 lines
Cc Thomas Renninger <[email protected]>
Cc Rafael J. Wysocki <[email protected]>

Thanks!

Thomas

Quoted 9 lines
---
 tools/power/cpupower/utils/helpers/sysfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0

b/tools/power/cpupower/utils/helpers/sysfs.c index 09afe5d..4e8fe2c 100644

tools/power/cpupower/utils/helpers/sysfs.c unsigned int sysfs_get_idlestate_count(unsigned int cpu) · line 366

Quoted 7 lines
tools/power/cpupower/utils/helpers/sysfs.c +1 -1
@@ -361,7 +361,7 @@ unsigned int sysfs_get_idlestate_count(unsigned int cpu)
 
 	snprintf(file, SYSFS_PATH_MAX, PATH_TO_CPU "cpuidle");
 	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
-		return -ENODEV;
+		return 0;
 
 	snprintf(file, SYSFS_PATH_MAX, PATH_TO_CPU "cpu%u/cpuidle/state0", 

cpu);

Quoted 1 line
tools/power/cpupower/utils/helpers/sysfs.c +0 -0
 	if (stat(file, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
#4 lore raw
In reply to Prarit Bhargava
Quoted 16 lines · Prarit Bhargava

My previous commit 16b7c275c055 ("tools: cpupower: fix return checks for sysfs_get_idlestate_count()") was not correct. After looking
at the changelog for cpupower I noticed that Thomas had changed the return of sysfs_get_idlestate_count() to an unsigned int to simplify the code. The problem is really that both he (in his original change) and I (in my new change) missed the obvious that sysfs_get_idlestate_count()
can't return -ENODEV. It should just return 0 for "no c-states".

Patch 1 reverts my recent change and patch 2 fixes the problem correctly.

Sorry 'bout that Thomas. I should have caught that the the first time.

Cc Thomas Renninger <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Quoted 8 lines

Prarit Bhargava (2):
Revert "tools: cpupower: fix return checks for
sysfs_get_idlestate_count()"
Fix no idle state information return value

 tools/power/cpupower/utils/cpuidle-info.c  |    8 ++++----
 tools/power/cpupower/utils/helpers/sysfs.c |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

I'll queue up the revert, but I need an ACK from Thomas for the second patch.

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.