diff mbox

cpu: maxfreq: report maxfreq and tidy up the test (LP: #1253658)

Message ID 1385044565-17603-1-git-send-email-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Nov. 21, 2013, 2:36 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Add some feedback on the maxfreq as it is useful to know.  Also, tidy
up the test in a few ways:

 Use bools rather than ints for true/false settings.
 Reduce the nesting as this makes the code harder to read.
 Fix the source into 80 columns.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/cpu/maxfreq/maxfreq.c | 128 ++++++++++++++++++++++++++--------------------
 1 file changed, 73 insertions(+), 55 deletions(-)

Comments

Ivan Hu Dec. 6, 2013, 1:05 p.m. UTC | #1 
On 11/21/2013 10:36 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Add some feedback on the maxfreq as it is useful to know.  Also, tidy
> up the test in a few ways:
>
>   Use bools rather than ints for true/false settings.
>   Reduce the nesting as this makes the code harder to read.
>   Fix the source into 80 columns.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/cpu/maxfreq/maxfreq.c | 128 ++++++++++++++++++++++++++--------------------
>   1 file changed, 73 insertions(+), 55 deletions(-)
>
> diff --git a/src/cpu/maxfreq/maxfreq.c b/src/cpu/maxfreq/maxfreq.c
> index 51ed1e8..be9ba43 100644
> --- a/src/cpu/maxfreq/maxfreq.c
> +++ b/src/cpu/maxfreq/maxfreq.c
> @@ -20,6 +20,8 @@
>
>   #ifdef FWTS_ARCH_INTEL
>
> +#include <stdlib.h>
> +#include <stdbool.h>
>   #include <sys/types.h>
>   #include <dirent.h>
>   #include <ctype.h>
> @@ -50,16 +52,13 @@ static double maxfreq_max(const char *str)
>
>   static int maxfreq_test1(fwts_framework *fw)
>   {
> -	DIR *dir;
> +	double *cpufreq;
> +	int cpu, cpus;
> +	bool advice = false, passed = true, cpufreqs_read = false;
>   	struct dirent *entry;
> -	int cpus;
> -	int cpufreqs_read = 0;
> -	int cpu;
> +	DIR *dir;
>   	fwts_list_link *item;
>   	fwts_list *cpuinfo;
> -	double *cpufreq;
> -	int failed = 0;
> -	int advice = 0;
>
>   	fwts_log_info(fw,
>   		"This test checks the maximum CPU frequency as detected by "
> @@ -96,7 +95,7 @@ static int maxfreq_test1(fwts_framework *fw)
>   					cpufreq[cpu] *= 1000000.0;
>   				if (strstr(str, "MHz"))
>   					cpufreq[cpu] *= 1000.0;
> -				cpufreqs_read++;
> +				cpufreqs_read = true;
>   			}
>   			else
>   				cpufreq[cpu] = -1.0;
> @@ -106,8 +105,10 @@ static int maxfreq_test1(fwts_framework *fw)
>   	}
>   	fwts_list_free(cpuinfo, free);
>
> -	if (cpufreqs_read == 0) {
> -		fwts_skipped(fw, "Cannot read CPU frequencies from %s, this generally happens on AMD CPUs, skipping test.", CPU_INFO_PATH);
> +	if (!cpufreqs_read) {
> +		fwts_skipped(fw,
> +			"Cannot read CPU frequencies from %s, this generally "
> +			"happens on AMD CPUs, skipping test.", CPU_INFO_PATH);
>   		free(cpufreq);
>   		return FWTS_SKIP;
>   	}
> @@ -122,59 +123,76 @@ static int maxfreq_test1(fwts_framework *fw)
>   	}
>
>   	do {
> +		char path[PATH_MAX];
> +		char *data;
> +		double maxfreq, maxfreq_ghz, cpufreq_ghz;
> +		int cpunum;
> +
>   		entry = readdir(dir);
> -		if (entry && strlen(entry->d_name)>2) {
> -			char path[PATH_MAX];
> -			char *data;
> -
> -			snprintf(path, sizeof(path),
> -				CPU_FREQ_PATH "/%s/cpufreq/scaling_available_frequencies",
> -				entry->d_name);
> -			if ((data = fwts_get(path)) != NULL) {
> -				double maxfreq = maxfreq_max(data);
> -				int cpunum = atoi(entry->d_name + 3);
> -				if (maxfreq < 0.0) {
> -					fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -						"CPUFreqReadFailed",
> -						"Cannot read cpu frequency from %s for CPU %s\n",
> -						CPU_FREQ_PATH, entry->d_name);
> -					failed++;
> -				} else {
> -					double maxfreq_ghz = maxfreq / 1000000.0;
> -					double cpufreq_ghz = cpufreq[cpunum] / 1000000.0;
> -
> -					if (fabs(maxfreq_ghz - cpufreq_ghz) > (maxfreq_ghz * 0.005)) {
> -						failed++;
> -						fwts_failed(fw,
> -							LOG_LEVEL_MEDIUM,
> -							"CPUFreqSpeedMismatch",
> -							"Maximum scaling frequency %f GHz do not match expected frequency %f GHz\n",
> -							maxfreq_ghz, cpufreq_ghz);
> -						if (!advice)  {
> -							advice++;
> -							fwts_advice(fw,
> -								"The maximum scaling frequency %f GHz for CPU %d configured by the BIOS in %s "
> -								"does not match the expected maximum CPU frequency %f GHz "
> -								"that the CPU can run at. This usually indicates a misconfiguration of "
> -								"the ACPI _PSS (Performance Supported States) object. This is described in "
> -								"section 8.4.4.2 of the APCI specification.",
> -								(double)maxfreq/1000000.0, cpunum, path,
> -								(double)cpufreq[cpunum]/1000000.0);
> -						}
> -						else
> -							fwts_advice(fw, "See advice for previous CPU.");
> -					}
> -				}
> +		if (!entry || entry->d_name[0] == '.' ||
> +		    strlen(entry->d_name) < 3)
> +			continue;
> +
> +		snprintf(path, sizeof(path),
> +			CPU_FREQ_PATH "/%s/cpufreq/scaling_available_frequencies",
> +			entry->d_name);
> +
> +		if ((data = fwts_get(path)) == NULL)
> +			continue;
> +		maxfreq = maxfreq_max(data);
> +		free(data);
> +
> +		cpunum = atoi(entry->d_name + 3);
> +		if (maxfreq < 0.0) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				"CPUFreqReadFailed",
> +				"Cannot read cpu frequency from %s for CPU %s.",
> +				CPU_FREQ_PATH, entry->d_name);
> +			passed = false;
> +			continue;
> +		}
> +
> +		maxfreq_ghz = maxfreq / 1000000.0;
> +		cpufreq_ghz = cpufreq[cpunum] / 1000000.0;
> +
> +		if (fabs(maxfreq_ghz - cpufreq_ghz) > (maxfreq_ghz * 0.005)) {
> +			passed = false;
> +			fwts_failed(fw,
> +				LOG_LEVEL_MEDIUM,
> +				"CPUFreqSpeedMismatch",
> +				"Maximum scaling frequency %f GHz do not match "
> +				"expected frequency %f GHz.",
> +				maxfreq_ghz, cpufreq_ghz);
> +			if (!advice)  {
> +				advice = true;
> +				fwts_advice(fw,
> +					"The maximum scaling frequency %f GHz "
> +					"for CPU %d configured by the BIOS in "
> +					"%s does not match the expected "
> +					"maximum CPU frequency %f GHz that "
> +					"the CPU can run at. This usually "
> +					"indicates a misconfiguration of the "
> +					"ACPI _PSS (Performance Supported "
> +					"States) object. This is described in "
> +					"section 8.4.4.2 of the APCI "
> +					"specification.",
> +					(double)maxfreq/1000000.0, cpunum, path,
> +					(double)cpufreq[cpunum]/1000000.0);
> +			} else {
> +				fwts_advice(fw, "See advice for previous CPU.");
>   			}
> -			free(data);
> +		} else {
> +			fwts_log_info(fw,
> +				"CPU %d maximum frequency %f GHz is sane.",
> +				cpunum, maxfreq_ghz);
>   		}
>   	} while (entry);
>
> -	if (!failed)
> +	if (passed)
>   		fwts_passed(fw, "%d CPUs passed the maximum frequency check.", cpus);
>
>   	free(cpufreq);
> -	closedir(dir);
> +	(void)closedir(dir);
>
>   	return FWTS_OK;
>   }
>

Acked-by: Ivan Hu <ivan.hu@canonical.com>
Alex Hung Dec. 9, 2013, 3:45 a.m. UTC | #2 
On 11/21/2013 10:36 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Add some feedback on the maxfreq as it is useful to know.  Also, tidy
> up the test in a few ways:
> 
>  Use bools rather than ints for true/false settings.
>  Reduce the nesting as this makes the code harder to read.
>  Fix the source into 80 columns.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/cpu/maxfreq/maxfreq.c | 128 ++++++++++++++++++++++++++--------------------
>  1 file changed, 73 insertions(+), 55 deletions(-)
> 
> diff --git a/src/cpu/maxfreq/maxfreq.c b/src/cpu/maxfreq/maxfreq.c
> index 51ed1e8..be9ba43 100644
> --- a/src/cpu/maxfreq/maxfreq.c
> +++ b/src/cpu/maxfreq/maxfreq.c
> @@ -20,6 +20,8 @@
>  
>  #ifdef FWTS_ARCH_INTEL
>  
> +#include <stdlib.h>
> +#include <stdbool.h>
>  #include <sys/types.h>
>  #include <dirent.h>
>  #include <ctype.h>
> @@ -50,16 +52,13 @@ static double maxfreq_max(const char *str)
>  
>  static int maxfreq_test1(fwts_framework *fw)
>  {
> -	DIR *dir;
> +	double *cpufreq;
> +	int cpu, cpus;
> +	bool advice = false, passed = true, cpufreqs_read = false;
>  	struct dirent *entry;
> -	int cpus;
> -	int cpufreqs_read = 0;
> -	int cpu;
> +	DIR *dir;
>  	fwts_list_link *item;
>  	fwts_list *cpuinfo;
> -	double *cpufreq;
> -	int failed = 0;
> -	int advice = 0;
>  
>  	fwts_log_info(fw,
>  		"This test checks the maximum CPU frequency as detected by "
> @@ -96,7 +95,7 @@ static int maxfreq_test1(fwts_framework *fw)
>  					cpufreq[cpu] *= 1000000.0;
>  				if (strstr(str, "MHz"))
>  					cpufreq[cpu] *= 1000.0;
> -				cpufreqs_read++;
> +				cpufreqs_read = true;
>  			}
>  			else
>  				cpufreq[cpu] = -1.0;
> @@ -106,8 +105,10 @@ static int maxfreq_test1(fwts_framework *fw)
>  	}
>  	fwts_list_free(cpuinfo, free);
>  
> -	if (cpufreqs_read == 0) {
> -		fwts_skipped(fw, "Cannot read CPU frequencies from %s, this generally happens on AMD CPUs, skipping test.", CPU_INFO_PATH);
> +	if (!cpufreqs_read) {
> +		fwts_skipped(fw,
> +			"Cannot read CPU frequencies from %s, this generally "
> +			"happens on AMD CPUs, skipping test.", CPU_INFO_PATH);
>  		free(cpufreq);
>  		return FWTS_SKIP;
>  	}
> @@ -122,59 +123,76 @@ static int maxfreq_test1(fwts_framework *fw)
>  	}
>  
>  	do {
> +		char path[PATH_MAX];
> +		char *data;
> +		double maxfreq, maxfreq_ghz, cpufreq_ghz;
> +		int cpunum;
> +
>  		entry = readdir(dir);
> -		if (entry && strlen(entry->d_name)>2) {
> -			char path[PATH_MAX];
> -			char *data;
> -
> -			snprintf(path, sizeof(path),
> -				CPU_FREQ_PATH "/%s/cpufreq/scaling_available_frequencies",
> -				entry->d_name);
> -			if ((data = fwts_get(path)) != NULL) {
> -				double maxfreq = maxfreq_max(data);
> -				int cpunum = atoi(entry->d_name + 3);
> -				if (maxfreq < 0.0) {
> -					fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -						"CPUFreqReadFailed",
> -						"Cannot read cpu frequency from %s for CPU %s\n",
> -						CPU_FREQ_PATH, entry->d_name);
> -					failed++;
> -				} else {
> -					double maxfreq_ghz = maxfreq / 1000000.0;
> -					double cpufreq_ghz = cpufreq[cpunum] / 1000000.0;
> -
> -					if (fabs(maxfreq_ghz - cpufreq_ghz) > (maxfreq_ghz * 0.005)) {
> -						failed++;
> -						fwts_failed(fw,
> -							LOG_LEVEL_MEDIUM,
> -							"CPUFreqSpeedMismatch",
> -							"Maximum scaling frequency %f GHz do not match expected frequency %f GHz\n",
> -							maxfreq_ghz, cpufreq_ghz);
> -						if (!advice)  {
> -							advice++;
> -							fwts_advice(fw,
> -								"The maximum scaling frequency %f GHz for CPU %d configured by the BIOS in %s "
> -								"does not match the expected maximum CPU frequency %f GHz "
> -								"that the CPU can run at. This usually indicates a misconfiguration of "
> -								"the ACPI _PSS (Performance Supported States) object. This is described in "
> -								"section 8.4.4.2 of the APCI specification.",
> -								(double)maxfreq/1000000.0, cpunum, path,
> -								(double)cpufreq[cpunum]/1000000.0);
> -						}
> -						else
> -							fwts_advice(fw, "See advice for previous CPU.");
> -					}
> -				}
> +		if (!entry || entry->d_name[0] == '.' ||
> +		    strlen(entry->d_name) < 3)
> +			continue;
> +
> +		snprintf(path, sizeof(path),
> +			CPU_FREQ_PATH "/%s/cpufreq/scaling_available_frequencies",
> +			entry->d_name);
> +
> +		if ((data = fwts_get(path)) == NULL)
> +			continue;
> +		maxfreq = maxfreq_max(data);
> +		free(data);
> +
> +		cpunum = atoi(entry->d_name + 3);
> +		if (maxfreq < 0.0) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +				"CPUFreqReadFailed",
> +				"Cannot read cpu frequency from %s for CPU %s.",
> +				CPU_FREQ_PATH, entry->d_name);
> +			passed = false;
> +			continue;
> +		}
> +
> +		maxfreq_ghz = maxfreq / 1000000.0;
> +		cpufreq_ghz = cpufreq[cpunum] / 1000000.0;
> +
> +		if (fabs(maxfreq_ghz - cpufreq_ghz) > (maxfreq_ghz * 0.005)) {
> +			passed = false;
> +			fwts_failed(fw,
> +				LOG_LEVEL_MEDIUM,
> +				"CPUFreqSpeedMismatch",
> +				"Maximum scaling frequency %f GHz do not match "
> +				"expected frequency %f GHz.",
> +				maxfreq_ghz, cpufreq_ghz);
> +			if (!advice)  {
> +				advice = true;
> +				fwts_advice(fw,
> +					"The maximum scaling frequency %f GHz "
> +					"for CPU %d configured by the BIOS in "
> +					"%s does not match the expected "
> +					"maximum CPU frequency %f GHz that "
> +					"the CPU can run at. This usually "
> +					"indicates a misconfiguration of the "
> +					"ACPI _PSS (Performance Supported "
> +					"States) object. This is described in "
> +					"section 8.4.4.2 of the APCI "
> +					"specification.",
> +					(double)maxfreq/1000000.0, cpunum, path,
> +					(double)cpufreq[cpunum]/1000000.0);
> +			} else {
> +				fwts_advice(fw, "See advice for previous CPU.");
>  			}
> -			free(data);
> +		} else {
> +			fwts_log_info(fw,
> +				"CPU %d maximum frequency %f GHz is sane.",
> +				cpunum, maxfreq_ghz);
>  		}
>  	} while (entry);
>  
> -	if (!failed)
> +	if (passed)
>  		fwts_passed(fw, "%d CPUs passed the maximum frequency check.", cpus);
>  
>  	free(cpufreq);
> -	closedir(dir);
> +	(void)closedir(dir);
>  
>  	return FWTS_OK;
>  }
> 

Acked-by: Alex Hung <alex.hung@canonical.com>
diff mbox

Patch

diff --git a/src/cpu/maxfreq/maxfreq.c b/src/cpu/maxfreq/maxfreq.c
index 51ed1e8..be9ba43 100644
--- a/src/cpu/maxfreq/maxfreq.c
+++ b/src/cpu/maxfreq/maxfreq.c
@@ -20,6 +20,8 @@ 
 
 #ifdef FWTS_ARCH_INTEL
 
+#include <stdlib.h>
+#include <stdbool.h>
 #include <sys/types.h>
 #include <dirent.h>
 #include <ctype.h>
@@ -50,16 +52,13 @@  static double maxfreq_max(const char *str)
 
 static int maxfreq_test1(fwts_framework *fw)
 {
-	DIR *dir;
+	double *cpufreq;
+	int cpu, cpus;
+	bool advice = false, passed = true, cpufreqs_read = false;
 	struct dirent *entry;
-	int cpus;
-	int cpufreqs_read = 0;
-	int cpu;
+	DIR *dir;
 	fwts_list_link *item;
 	fwts_list *cpuinfo;
-	double *cpufreq;
-	int failed = 0;
-	int advice = 0;
 
 	fwts_log_info(fw,
 		"This test checks the maximum CPU frequency as detected by "
@@ -96,7 +95,7 @@  static int maxfreq_test1(fwts_framework *fw)
 					cpufreq[cpu] *= 1000000.0;
 				if (strstr(str, "MHz"))
 					cpufreq[cpu] *= 1000.0;
-				cpufreqs_read++;
+				cpufreqs_read = true;
 			}
 			else
 				cpufreq[cpu] = -1.0;
@@ -106,8 +105,10 @@  static int maxfreq_test1(fwts_framework *fw)
 	}
 	fwts_list_free(cpuinfo, free);
 
-	if (cpufreqs_read == 0) {
-		fwts_skipped(fw, "Cannot read CPU frequencies from %s, this generally happens on AMD CPUs, skipping test.", CPU_INFO_PATH);
+	if (!cpufreqs_read) {
+		fwts_skipped(fw,
+			"Cannot read CPU frequencies from %s, this generally "
+			"happens on AMD CPUs, skipping test.", CPU_INFO_PATH);
 		free(cpufreq);
 		return FWTS_SKIP;
 	}
@@ -122,59 +123,76 @@  static int maxfreq_test1(fwts_framework *fw)
 	}
 
 	do {
+		char path[PATH_MAX];
+		char *data;
+		double maxfreq, maxfreq_ghz, cpufreq_ghz;
+		int cpunum;
+
 		entry = readdir(dir);
-		if (entry && strlen(entry->d_name)>2) {
-			char path[PATH_MAX];
-			char *data;
-
-			snprintf(path, sizeof(path),
-				CPU_FREQ_PATH "/%s/cpufreq/scaling_available_frequencies",
-				entry->d_name);
-			if ((data = fwts_get(path)) != NULL) {
-				double maxfreq = maxfreq_max(data);
-				int cpunum = atoi(entry->d_name + 3);
-				if (maxfreq < 0.0) {
-					fwts_failed(fw, LOG_LEVEL_MEDIUM,
-						"CPUFreqReadFailed",
-						"Cannot read cpu frequency from %s for CPU %s\n",
-						CPU_FREQ_PATH, entry->d_name);
-					failed++;
-				} else {
-					double maxfreq_ghz = maxfreq / 1000000.0;
-					double cpufreq_ghz = cpufreq[cpunum] / 1000000.0;
-
-					if (fabs(maxfreq_ghz - cpufreq_ghz) > (maxfreq_ghz * 0.005)) {
-						failed++;
-						fwts_failed(fw,
-							LOG_LEVEL_MEDIUM,
-							"CPUFreqSpeedMismatch",
-							"Maximum scaling frequency %f GHz do not match expected frequency %f GHz\n",
-							maxfreq_ghz, cpufreq_ghz);
-						if (!advice)  {
-							advice++;
-							fwts_advice(fw,
-								"The maximum scaling frequency %f GHz for CPU %d configured by the BIOS in %s "
-								"does not match the expected maximum CPU frequency %f GHz "
-								"that the CPU can run at. This usually indicates a misconfiguration of "
-								"the ACPI _PSS (Performance Supported States) object. This is described in "
-								"section 8.4.4.2 of the APCI specification.",
-								(double)maxfreq/1000000.0, cpunum, path,
-								(double)cpufreq[cpunum]/1000000.0);
-						}
-						else
-							fwts_advice(fw, "See advice for previous CPU.");
-					}
-				}
+		if (!entry || entry->d_name[0] == '.' ||
+		    strlen(entry->d_name) < 3)
+			continue;
+
+		snprintf(path, sizeof(path),
+			CPU_FREQ_PATH "/%s/cpufreq/scaling_available_frequencies",
+			entry->d_name);
+
+		if ((data = fwts_get(path)) == NULL)
+			continue;
+		maxfreq = maxfreq_max(data);
+		free(data);
+
+		cpunum = atoi(entry->d_name + 3);
+		if (maxfreq < 0.0) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM,
+				"CPUFreqReadFailed",
+				"Cannot read cpu frequency from %s for CPU %s.",
+				CPU_FREQ_PATH, entry->d_name);
+			passed = false;
+			continue;
+		}
+
+		maxfreq_ghz = maxfreq / 1000000.0;
+		cpufreq_ghz = cpufreq[cpunum] / 1000000.0;
+
+		if (fabs(maxfreq_ghz - cpufreq_ghz) > (maxfreq_ghz * 0.005)) {
+			passed = false;
+			fwts_failed(fw,
+				LOG_LEVEL_MEDIUM,
+				"CPUFreqSpeedMismatch",
+				"Maximum scaling frequency %f GHz do not match "
+				"expected frequency %f GHz.",
+				maxfreq_ghz, cpufreq_ghz);
+			if (!advice)  {
+				advice = true;
+				fwts_advice(fw,
+					"The maximum scaling frequency %f GHz "
+					"for CPU %d configured by the BIOS in "
+					"%s does not match the expected "
+					"maximum CPU frequency %f GHz that "
+					"the CPU can run at. This usually "
+					"indicates a misconfiguration of the "
+					"ACPI _PSS (Performance Supported "
+					"States) object. This is described in "
+					"section 8.4.4.2 of the APCI "
+					"specification.",
+					(double)maxfreq/1000000.0, cpunum, path,
+					(double)cpufreq[cpunum]/1000000.0);
+			} else {
+				fwts_advice(fw, "See advice for previous CPU.");
 			}
-			free(data);
+		} else {
+			fwts_log_info(fw,
+				"CPU %d maximum frequency %f GHz is sane.",
+				cpunum, maxfreq_ghz);
 		}
 	} while (entry);
 
-	if (!failed)
+	if (passed)
 		fwts_passed(fw, "%d CPUs passed the maximum frequency check.", cpus);
 
 	free(cpufreq);
-	closedir(dir);
+	(void)closedir(dir);
 
 	return FWTS_OK;
 }