[PATCH 0/4] tools/power/cpupower latest fixes

[PATCH 0/4] tools/power/cpupower latest fixes

[Thomas Renninger]

These are fixes for the cpupower utility I received lately.

Rafael: Would be great if you can queue them in your pm tree.

Thanks,

         Thomas

Himangi Saraogi (1):
  cpupower: mperf monitor: Correct use of ! and &

Peter Senna Tschudin (1):
  cpupower: Remove redundant error check

Rickard Strandqvist (1):
  tools: power: cpupower: bench: parse.c: Fix several minor errors

Thomas Renninger (1):
  cpupower: Adjust MAINTAINERS file

 MAINTAINERS                                        |    2 +-
 tools/power/cpupower/bench/parse.c                 |   39 +++++++++++---------
 tools/power/cpupower/utils/cpufreq-set.c           |   11 +++---
 .../cpupower/utils/idle_monitor/mperf_monitor.c    |    2 +-
 4 files changed, 28 insertions(+), 26 deletions(-)

-- 
1.7.6.1

[PATCH 1/4] cpupower: mperf monitor: Correct use of ! and &

[Thomas Renninger]

From: Himangi Saraogi <himangi774@gmail.com>

In commit ae91d60ba88ef0bdb1b5e9b2363bd52fc45d2af7, a bug was fixed that
involved converting !x & y to !(x & y).  The code below shows the same
pattern, and thus should perhaps be fixed in the same way.

The Coccinelle semantic patch that makes this change is as follows:

// <smpl>
@@ expression E1,E2; @@
(
  !E1 & !E2
|
- !E1 & E2
+ !(E1 & E2)
)
// </smpl>

Signed-off-by: Himangi Saraogi <himangi774@gmail.com>
---
 .../cpupower/utils/idle_monitor/mperf_monitor.c    |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
index 5650ab5..90a8c4f 100644
--- a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
+++ b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
@@ -237,7 +237,7 @@ static int init_maxfreq_mode(void)
 	unsigned long long hwcr;
 	unsigned long min;
 
-	if (!cpupower_cpu_info.caps & CPUPOWER_CAP_INV_TSC)
+	if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_INV_TSC))
 		goto use_sysfs;
 
 	if (cpupower_cpu_info.vendor == X86_VENDOR_AMD) {
-- 
1.7.6.1

[PATCH 2/4] tools: power: cpupower: bench: parse.c: Fix several minor errors

[Thomas Renninger]

From: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>

Resolved several minor errors in prepare_config() and made some additional improvements.
Earlier, the risk of file stream that was not closed. Misuse of strncpy, and the use of strncmp with strlen that makes it pointless.
I also check that sscanf has been successful, otherwise continue to the next line. And minimized the use of magic numbers.

This was found using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 tools/power/cpupower/bench/parse.c |   39 +++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
index 543bba1..f503fb5 100644
--- a/tools/power/cpupower/bench/parse.c
+++ b/tools/power/cpupower/bench/parse.c
@@ -158,14 +158,15 @@ struct config *prepare_default_config()
 int prepare_config(const char *path, struct config *config)
 {
 	size_t len = 0;
-	char *opt, *val, *line = NULL;
-	FILE *configfile = fopen(path, "r");
+	char opt[16], val[32], *line = NULL;
+	FILE *configfile;
 
 	if (config == NULL) {
 		fprintf(stderr, "error: config is NULL\n");
 		return 1;
 	}
 
+	configfile = fopen(path, "r");
 	if (configfile == NULL) {
 		perror("fopen");
 		fprintf(stderr, "error: unable to read configfile\n");
@@ -174,52 +175,54 @@ int prepare_config(const char *path, struct config *config)
 	}
 
 	while (getline(&line, &len, configfile) != -1) {
-		if (line[0] == '#' || line[0] == ' ')
+		if (line[0] == '#' || line[0] == ' ' || line[0] == '\n')
 			continue;
 
-		sscanf(line, "%as = %as", &opt, &val);
+		if (sscanf(line, "%14s = %30s", opt, val) < 2)
+			continue;
 
 		dprintf("parsing: %s -> %s\n", opt, val);
 
-		if (strncmp("sleep", opt, strlen(opt)) == 0)
+		if (strcmp("sleep", opt) == 0)
 			sscanf(val, "%li", &config->sleep);
 
-		else if (strncmp("load", opt, strlen(opt)) == 0)
+		else if (strcmp("load", opt) == 0)
 			sscanf(val, "%li", &config->load);
 
-		else if (strncmp("load_step", opt, strlen(opt)) == 0)
+		else if (strcmp("load_step", opt) == 0)
 			sscanf(val, "%li", &config->load_step);
 
-		else if (strncmp("sleep_step", opt, strlen(opt)) == 0)
+		else if (strcmp("sleep_step", opt) == 0)
 			sscanf(val, "%li", &config->sleep_step);
 
-		else if (strncmp("cycles", opt, strlen(opt)) == 0)
+		else if (strcmp("cycles", opt) == 0)
 			sscanf(val, "%u", &config->cycles);
 
-		else if (strncmp("rounds", opt, strlen(opt)) == 0)
+		else if (strcmp("rounds", opt) == 0)
 			sscanf(val, "%u", &config->rounds);
 
-		else if (strncmp("verbose", opt, strlen(opt)) == 0)
+		else if (strcmp("verbose", opt) == 0)
 			sscanf(val, "%u", &config->verbose);
 
-		else if (strncmp("output", opt, strlen(opt)) == 0)
+		else if (strcmp("output", opt) == 0)
 			config->output = prepare_output(val); 
 
-		else if (strncmp("cpu", opt, strlen(opt)) == 0)
+		else if (strcmp("cpu", opt) == 0)
 			sscanf(val, "%u", &config->cpu);
 
-		else if (strncmp("governor", opt, 14) == 0)
-			strncpy(config->governor, val, 14);
+		else if (strcmp("governor", opt) == 0) {
+			strncpy(config->governor, val,
+					sizeof(config->governor));
+			config->governor[sizeof(config->governor) - 1] = '\0';
+		}
 
-		else if (strncmp("priority", opt, strlen(opt)) == 0) {
+		else if (strcmp("priority", opt) == 0) {
 			if (string_to_prio(val) != SCHED_ERR)
 				config->prio = string_to_prio(val);
 		}
 	}
 
 	free(line);
-	free(opt);
-	free(val);
 
 	return 0;
 }
-- 
1.7.6.1

[PATCH 3/4] cpupower: Remove redundant error check

[Thomas Renninger]

From: Peter Senna Tschudin <peter.senna@gmail.com>

Remove double checks, and move the call to print_error to the
first check. Replace break by return, and return 0 on success.
The simplified version of the coccinelle semantic patch that
fixes this issue is as follows:

// <smpl>
@@
expression E; identifier pr; expression list es;
@@
for(...;...;...){
...
-	if (E) break;
+	if (E){
+		pr(es);
+		break;
+	}
...
}
- if(E) pr(es);
// </smpl>

Untested.

Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
 tools/power/cpupower/utils/cpufreq-set.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
index a416de8..f656e58 100644
--- a/tools/power/cpupower/utils/cpufreq-set.c
+++ b/tools/power/cpupower/utils/cpufreq-set.c
@@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv)
 
 		printf(_("Setting cpu: %d\n"), cpu);
 		ret = do_one_cpu(cpu, &new_pol, freq, policychange);
-		if (ret)
-			break;
+		if (ret) {
+			print_error();
+			return ret;
+		}
 	}
 
-	if (ret)
-		print_error();
-
-	return ret;
+	return 0;
 }
-- 
1.7.6.1

[PATCH 4/4] cpupower: Adjust MAINTAINERS file

[Thomas Renninger]

Dominik seem to not respond to emails anymore since he finished studies.

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 MAINTAINERS |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index d76e077..c8266ca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2521,8 +2521,8 @@ F:	arch/x86/kernel/cpuid.c
 F:	arch/x86/kernel/msr.c
 
 CPU POWER MONITORING SUBSYSTEM
-M:	Dominik Brodowski <linux@dominikbrodowski.net>
 M:	Thomas Renninger <trenn@suse.de>
+L:	linux-pm@vger.kernel.org
 S:	Maintained
 F:	tools/power/cpupower/
 
-- 
1.7.6.1

Re: [PATCH 0/4] tools/power/cpupower latest fixes

[Rafael J. Wysocki]

On Tuesday, July 22, 2014 04:05:57 PM Thomas Renninger wrote:
> These are fixes for the cpupower utility I received lately.
> 
> Rafael: Would be great if you can queue them in your pm tree.

Applied [4/4] and please sign off the other ones so that I can
apply them.

Please always sign off patches from other people you send to me
so that it is clear that I've received them from you.

Rafael

[PATCH 1/3] cpupower: mperf monitor: Correct use of ! and &

[Thomas Renninger]

From: Himangi Saraogi <himangi774@gmail.com>

In commit ae91d60ba88ef0bdb1b5e9b2363bd52fc45d2af7, a bug was fixed that
involved converting !x & y to !(x & y).  The code below shows the same
pattern, and thus should perhaps be fixed in the same way.

The Coccinelle semantic patch that makes this change is as follows:

// <smpl>
@@ expression E1,E2; @@
(
  !E1 & !E2
|
- !E1 & E2
+ !(E1 & E2)
)
// </smpl>

Signed-off-by: Himangi Saraogi <himangi774@gmail.com>
Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 .../cpupower/utils/idle_monitor/mperf_monitor.c    |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
index 5650ab5..90a8c4f 100644
--- a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
+++ b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
@@ -237,7 +237,7 @@ static int init_maxfreq_mode(void)
 	unsigned long long hwcr;
 	unsigned long min;
 
-	if (!cpupower_cpu_info.caps & CPUPOWER_CAP_INV_TSC)
+	if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_INV_TSC))
 		goto use_sysfs;
 
 	if (cpupower_cpu_info.vendor == X86_VENDOR_AMD) {
-- 
1.7.6.1

[PATCH 2/3] tools: power: cpupower: bench: parse.c: Fix several minor errors

[Thomas Renninger]

From: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>

Resolved several minor errors in prepare_config() and made some additional improvements.
Earlier, the risk of file stream that was not closed. Misuse of strncpy, and the use of strncmp with strlen that makes it pointless.
I also check that sscanf has been successful, otherwise continue to the next line. And minimized the use of magic numbers.

This was found using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 tools/power/cpupower/bench/parse.c |   39 +++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c
index 543bba1..f503fb5 100644
--- a/tools/power/cpupower/bench/parse.c
+++ b/tools/power/cpupower/bench/parse.c
@@ -158,14 +158,15 @@ struct config *prepare_default_config()
 int prepare_config(const char *path, struct config *config)
 {
 	size_t len = 0;
-	char *opt, *val, *line = NULL;
-	FILE *configfile = fopen(path, "r");
+	char opt[16], val[32], *line = NULL;
+	FILE *configfile;
 
 	if (config == NULL) {
 		fprintf(stderr, "error: config is NULL\n");
 		return 1;
 	}
 
+	configfile = fopen(path, "r");
 	if (configfile == NULL) {
 		perror("fopen");
 		fprintf(stderr, "error: unable to read configfile\n");
@@ -174,52 +175,54 @@ int prepare_config(const char *path, struct config *config)
 	}
 
 	while (getline(&line, &len, configfile) != -1) {
-		if (line[0] == '#' || line[0] == ' ')
+		if (line[0] == '#' || line[0] == ' ' || line[0] == '\n')
 			continue;
 
-		sscanf(line, "%as = %as", &opt, &val);
+		if (sscanf(line, "%14s = %30s", opt, val) < 2)
+			continue;
 
 		dprintf("parsing: %s -> %s\n", opt, val);
 
-		if (strncmp("sleep", opt, strlen(opt)) == 0)
+		if (strcmp("sleep", opt) == 0)
 			sscanf(val, "%li", &config->sleep);
 
-		else if (strncmp("load", opt, strlen(opt)) == 0)
+		else if (strcmp("load", opt) == 0)
 			sscanf(val, "%li", &config->load);
 
-		else if (strncmp("load_step", opt, strlen(opt)) == 0)
+		else if (strcmp("load_step", opt) == 0)
 			sscanf(val, "%li", &config->load_step);
 
-		else if (strncmp("sleep_step", opt, strlen(opt)) == 0)
+		else if (strcmp("sleep_step", opt) == 0)
 			sscanf(val, "%li", &config->sleep_step);
 
-		else if (strncmp("cycles", opt, strlen(opt)) == 0)
+		else if (strcmp("cycles", opt) == 0)
 			sscanf(val, "%u", &config->cycles);
 
-		else if (strncmp("rounds", opt, strlen(opt)) == 0)
+		else if (strcmp("rounds", opt) == 0)
 			sscanf(val, "%u", &config->rounds);
 
-		else if (strncmp("verbose", opt, strlen(opt)) == 0)
+		else if (strcmp("verbose", opt) == 0)
 			sscanf(val, "%u", &config->verbose);
 
-		else if (strncmp("output", opt, strlen(opt)) == 0)
+		else if (strcmp("output", opt) == 0)
 			config->output = prepare_output(val); 
 
-		else if (strncmp("cpu", opt, strlen(opt)) == 0)
+		else if (strcmp("cpu", opt) == 0)
 			sscanf(val, "%u", &config->cpu);
 
-		else if (strncmp("governor", opt, 14) == 0)
-			strncpy(config->governor, val, 14);
+		else if (strcmp("governor", opt) == 0) {
+			strncpy(config->governor, val,
+					sizeof(config->governor));
+			config->governor[sizeof(config->governor) - 1] = '\0';
+		}
 
-		else if (strncmp("priority", opt, strlen(opt)) == 0) {
+		else if (strcmp("priority", opt) == 0) {
 			if (string_to_prio(val) != SCHED_ERR)
 				config->prio = string_to_prio(val);
 		}
 	}
 
 	free(line);
-	free(opt);
-	free(val);
 
 	return 0;
 }
-- 
1.7.6.1

[PATCH 3/3] cpupower: Remove redundant error check

[Thomas Renninger]

From: Peter Senna Tschudin <peter.senna@gmail.com>

Remove double checks, and move the call to print_error to the
first check. Replace break by return, and return 0 on success.
The simplified version of the coccinelle semantic patch that
fixes this issue is as follows:

// <smpl>
@@
expression E; identifier pr; expression list es;
@@
for(...;...;...){
...
-	if (E) break;
+	if (E){
+		pr(es);
+		break;
+	}
...
}
- if(E) pr(es);
// </smpl>

Untested.

Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 tools/power/cpupower/utils/cpufreq-set.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
index a416de8..f656e58 100644
--- a/tools/power/cpupower/utils/cpufreq-set.c
+++ b/tools/power/cpupower/utils/cpufreq-set.c
@@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv)
 
 		printf(_("Setting cpu: %d\n"), cpu);
 		ret = do_one_cpu(cpu, &new_pol, freq, policychange);
-		if (ret)
-			break;
+		if (ret) {
+			print_error();
+			return ret;
+		}
 	}
 
-	if (ret)
-		print_error();
-
-	return ret;
+	return 0;
 }
-- 
1.7.6.1

Re: [PATCH 1/3] cpupower: mperf monitor: Correct use of ! and &

[Rafael J. Wysocki]

On Tuesday, July 29, 2014 06:12:18 PM Thomas Renninger wrote:
> From: Himangi Saraogi <himangi774@gmail.com>
> 
> In commit ae91d60ba88ef0bdb1b5e9b2363bd52fc45d2af7, a bug was fixed that
> involved converting !x & y to !(x & y).  The code below shows the same
> pattern, and thus should perhaps be fixed in the same way.
> 
> The Coccinelle semantic patch that makes this change is as follows:
> 
> // <smpl>
> @@ expression E1,E2; @@
> (
>   !E1 & !E2
> |
> - !E1 & E2
> + !(E1 & E2)
> )
> // </smpl>
> 
> Signed-off-by: Himangi Saraogi <himangi774@gmail.com>
> Signed-off-by: Thomas Renninger <trenn@suse.de>

All three queued up for 3.17, thanks!

> ---
>  .../cpupower/utils/idle_monitor/mperf_monitor.c    |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
> index 5650ab5..90a8c4f 100644
> --- a/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
> +++ b/tools/power/cpupower/utils/idle_monitor/mperf_monitor.c
> @@ -237,7 +237,7 @@ static int init_maxfreq_mode(void)
>  	unsigned long long hwcr;
>  	unsigned long min;
>  
> -	if (!cpupower_cpu_info.caps & CPUPOWER_CAP_INV_TSC)
> +	if (!(cpupower_cpu_info.caps & CPUPOWER_CAP_INV_TSC))
>  		goto use_sysfs;
>  
>  	if (cpupower_cpu_info.vendor == X86_VENDOR_AMD) {
> 

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