diff mbox

[v2] cpufreq: Add test cases to validate the frequency and pstate table

Message ID 1462194216-19638-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com
State Rejected
Headers show

Commit Message

Shilpasri G Bhat May 2, 2016, 1:03 p.m. UTC 
Add test case to parse CPU frequency and pstate table to check for
invalid entries that are beyond the specified minimum and maximum
limits.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
Changes from v1:
- Use libfdt to read device tree properties from fw->fdt instead of reading
them from sysfs files as suggested by Jeremy Kerr.

 src/cpu/cpufreq/cpufreq.c         | 124 ++++++++++++++++++++++++++++++++++++++
 src/lib/include/fwts_devicetree.h |  22 +++++++
 src/lib/src/fwts_devicetree.c     |  34 +++++++++++
 3 files changed, 180 insertions(+)

Comments

Jeremy Kerr May 3, 2016, 7:36 a.m. UTC | #1 
Hi Shilparsi,

This patch is looking good - but still a couple of minor changes that
I'd suggest:

> +static int cpufreq_test_frequency_limits(fwts_framework *fw)

Just curious: do we fail both of those tests on the recent turbo-mode
OPAL bug? Does Linux still parse the invalid range of pstates from the
DT and export them to /sys/?

> +#if FWTS_HAS_DEVICETREE
> +static int cpufreq_test_pstate_limits(fwts_framework *fw)

It's a close call on whether we put this in the cpufreq tests, or the
device tree tests. The latter would mean that we don't start adding
conditional compilation into the cpufreq code, so I think I'd prefer
that (but that's just my opinion, the FWTS folks may disagree!)

> +{
> +	const char *power_mgt_path = "/ibm,opal/power-mgt/";
> +	int pstate_min, pstate_max, pstates[MAX_FREQS], nr_pstates;
> +	bool ok = true;
> +	int  offset, len, ret, i;
> +
> +	if (!fw->fdt) {
> +		fwts_skipped(fw, "Device tree not found");
> +		return FWTS_SKIP;
> +	}
> +
> +	offset = fdt_path_offset(fw->fdt, power_mgt_path);
> +	if (offset < 0) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
> +			    "power management node is missing");
> +		return FWTS_ERROR;
> +	}

This isn't necessarily an error on all device tree platforms - consider
non-OPAL POWER machines.

You might want to skip this test on the condition:

	if (fwts_firmware_detect() != FWTS_FIRMWARE_OPAL)

[Also, do this at the beginning of the test, so that non-OPAL machines
don't print out messages about no device tree.]

Looks good though - I like the new device-tree helpers too.

Cheers,


Jeremy
Shilpasri G Bhat May 3, 2016, 10:25 a.m. UTC | #2 
Hi Jeremy,

On 05/03/2016 01:06 PM, Jeremy Kerr wrote:
> Hi Shilparsi,
> 
> This patch is looking good - but still a couple of minor changes that
> I'd suggest:

Thanks for the review.

> 
>> +static int cpufreq_test_frequency_limits(fwts_framework *fw)
> 
> Just curious: do we fail both of those tests on the recent turbo-mode
> OPAL bug? Does Linux still parse the invalid range of pstates from the
> DT and export them to /sys/?

We will fail the second test 'cpufreq_test_pstate_limits' in the turbo-mode OPAL
bug. The first test 'cpufreq_test_frequency_limits' will pass as min and max
frequency are computed by cpufreq core from the frequency table which is passed
by the driver (by parsing ibm,pstate-frequencies-mhz property).

/sys/devices/system/cpu/cpuX/cpufreq/cpuinfo_max_freq and
/sys/devices/system/cpu/cpuX/cpufreq/cpuinfo_min_freq will point to min and max
entries in 'ibm,pstate-frequencies-mhz' property.

Yes Linux directly uses the pstates from DT and exports to /sys/. Currently we
are clipping out the invalid range of pstates in OPAL.

The second test will fail in turbo-mode OPAL bug as the min and max pstate are
same as what is provided by OCC. (which is exported as DT properties in
ibm,pstate-max and ibm,pstate-min)

> 
>> +#if FWTS_HAS_DEVICETREE
>> +static int cpufreq_test_pstate_limits(fwts_framework *fw)
> 
> It's a close call on whether we put this in the cpufreq tests, or the
> device tree tests. The latter would mean that we don't start adding
> conditional compilation into the cpufreq code, so I think I'd prefer
> that (but that's just my opinion, the FWTS folks may disagree!)
> 

Okay.

>> +{
>> +	const char *power_mgt_path = "/ibm,opal/power-mgt/";
>> +	int pstate_min, pstate_max, pstates[MAX_FREQS], nr_pstates;
>> +	bool ok = true;
>> +	int  offset, len, ret, i;
>> +
>> +	if (!fw->fdt) {
>> +		fwts_skipped(fw, "Device tree not found");
>> +		return FWTS_SKIP;
>> +	}
>> +
>> +	offset = fdt_path_offset(fw->fdt, power_mgt_path);
>> +	if (offset < 0) {
>> +		fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
>> +			    "power management node is missing");
>> +		return FWTS_ERROR;
>> +	}
> 
> This isn't necessarily an error on all device tree platforms - consider
> non-OPAL POWER machines.
> 
> You might want to skip this test on the condition:
> 
> 	if (fwts_firmware_detect() != FWTS_FIRMWARE_OPAL)
> 
> [Also, do this at the beginning of the test, so that non-OPAL machines
> don't print out messages about no device tree.]

Yes agree. Will post the next version with this change.
> 
> Looks good though - I like the new device-tree helpers too.
> 

Thanks and Regards,
Shilpa
diff mbox

Patch

diff --git a/src/cpu/cpufreq/cpufreq.c b/src/cpu/cpufreq/cpufreq.c
index bdcadca..fe178c7 100644
--- a/src/cpu/cpufreq/cpufreq.c
+++ b/src/cpu/cpufreq/cpufreq.c
@@ -40,6 +40,10 @@ 
 #include <ctype.h>
 #include <inttypes.h>
 
+#if FWTS_HAS_DEVICETREE
+#include <libfdt.h>
+#endif
+
 #define FWTS_CPU_PATH	"/sys/devices/system/cpu"
 
 #define MAX_FREQS	256
@@ -393,6 +397,122 @@  static int cpufreq_test_cpu_performance(fwts_framework *fw)
 	return FWTS_OK;
 }
 
+static int cpufreq_test_frequency_limits(fwts_framework *fw)
+{
+	char path[PATH_MAX];
+	int min, max, i;
+	bool ok = true;
+
+	cpu_mkpath(path, sizeof(path), &cpus[0], "cpuinfo_min_freq");
+	if (fwts_get_int(path, &min) != FWTS_OK) {
+		fwts_log_warning(fw, "Failed to read the minimum CPU frequency %s",
+				 path);
+		return FWTS_ERROR;
+	}
+
+	cpu_mkpath(path, sizeof(path), &cpus[0], "cpuinfo_max_freq");
+	if (fwts_get_int(path, &max) != FWTS_OK) {
+		fwts_log_warning(fw, "Failed to read the maximum CPU frequency %s",
+				 path);
+		return FWTS_ERROR;
+	}
+
+	for (i = 0; i < cpus[0].n_freqs; i++) {
+		if (cpus[0].freqs[i].Hz > (uint64_t)max) {
+			fwts_log_warning(fw, "%s is greater than max frequency %s",
+					 hz_to_human(cpus[0].freqs[i].Hz),
+					 hz_to_human(max));
+			ok = false;
+		}
+		if (cpus[0].freqs[i].Hz < (uint64_t)min) {
+			fwts_log_warning(fw, "%s is lesser than min frequency %s",
+					 hz_to_human(cpus[0].freqs[i].Hz),
+					 hz_to_human(min));
+			ok = false;
+		}
+	}
+	if (ok)
+		fwts_passed(fw, "CPU frequencies are within the min and max limits");
+	else
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "CPUFreqLimitsTest",
+			     "Contains frequencies beyond max and min limits");
+	return FWTS_OK;
+}
+
+#if FWTS_HAS_DEVICETREE
+static int cpufreq_test_pstate_limits(fwts_framework *fw)
+{
+	const char *power_mgt_path = "/ibm,opal/power-mgt/";
+	int pstate_min, pstate_max, pstates[MAX_FREQS], nr_pstates;
+	bool ok = true;
+	int  offset, len, ret, i;
+
+	if (!fw->fdt) {
+		fwts_skipped(fw, "Device tree not found");
+		return FWTS_SKIP;
+	}
+
+	offset = fdt_path_offset(fw->fdt, power_mgt_path);
+	if (offset < 0) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTNodeMissing",
+			    "power management node is missing");
+		return FWTS_ERROR;
+	}
+
+	ret = fwts_dt_property_read_u32(fw->fdt, offset, "ibm,pstate-min",
+					&pstate_min);
+	if (ret != FWTS_OK) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
+			    "Failed to read property ibm,pstate-min %s",
+			    fdt_strerror(pstate_min));
+		return FWTS_ERROR;
+	}
+
+	ret = fwts_dt_property_read_u32(fw->fdt, offset, "ibm,pstate-max",
+					&pstate_max);
+	if (ret != FWTS_OK) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
+			    "Failed to read property ibm,pstate-max %s",
+			    fdt_strerror(pstate_max));
+		return FWTS_ERROR;
+	}
+
+	ret = fwts_dt_property_read_u32_arr(fw->fdt, offset, "ibm,pstate-ids",
+					    pstates, &len);
+	if (ret != FWTS_OK) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "DTPropertyReadError",
+			    "Failed to read property ibm,pstate-ids %s",
+			    fdt_strerror(len));
+		return FWTS_ERROR;
+	}
+
+	nr_pstates = pstate_max - pstate_min;
+	if (len != nr_pstates)
+		fwts_log_warning(fw, "Wrong number of pstates. Expected %d pstates, but found %d pstates",
+				 nr_pstates, len);
+
+	for (i = 0; i < nr_pstates; i++) {
+		if (pstates[i] > pstate_max) {
+			fwts_log_warning(fw, "Invalid Pstate id %d greater than max pstate %d",
+					 pstates[i], pstate_max);
+			ok = false;
+		}
+		if (pstates[i] < pstate_min) {
+			fwts_log_warning(fw, "Invalid Pstate id %d lesser than min pstate %d",
+					 pstates[i], pstate_min);
+			ok = false;
+		}
+	}
+
+	if (ok)
+		fwts_passed(fw, "CPU pstates are within the min and max limits");
+	else
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "CPUPstateLimitsTest",
+			    "Contains pstates beyond max and min limits");
+	return FWTS_OK;
+}
+#endif
+
 static int sw_tests_possible(fwts_framework *fw)
 {
 	int i, online_cpus = 0;
@@ -861,6 +981,10 @@  static fwts_framework_minor_test cpufreq_tests[] = {
 	{ cpufreq_test_sw_any,		"CPU frequency SW_ANY control" },
 	{ cpufreq_test_sw_all,		"CPU frequency SW_ALL control" },
 	{ cpufreq_test_cpu_performance,	"CPU frequency performance tests." },
+	{ cpufreq_test_frequency_limits, "CPU frequency limits test" },
+#if FWTS_HAS_DEVICETREE
+	{ cpufreq_test_pstate_limits,	"CPU Pstate limits test (device_tree)" },
+#endif
 	{ NULL, NULL }
 };
 
diff --git a/src/lib/include/fwts_devicetree.h b/src/lib/include/fwts_devicetree.h
index 7c67dc5..0299b64 100644
--- a/src/lib/include/fwts_devicetree.h
+++ b/src/lib/include/fwts_devicetree.h
@@ -31,6 +31,10 @@ 
 #if FWTS_HAS_DEVICETREE
 
 int fwts_devicetree_read(fwts_framework *fwts);
+int fwts_dt_property_read_u32(void *fdt, int offset, const char *pname,
+			      int *value);
+int fwts_dt_property_read_u32_arr(void *fdt, int offset, const char *pname,
+				  int *value, int *len);
 
 #else /* !FWTS_HAS_DEVICETREE */
 static inline int fwts_devicetree_read(fwts_framework *fwts
@@ -38,6 +42,24 @@  static inline int fwts_devicetree_read(fwts_framework *fwts
 {
 	return FWTS_OK;
 }
+
+static inline int fwts_dt_property_read_u32(void *fdt __attribute__((unused)),
+			int offset __attribute__((unused)),
+			const char *pname __attribute__((unused)),
+			int *value __attribute__((unused)))
+{
+	return FWTS_OK;
+}
+
+static inline int fwts_dt_property_read_u32_arr(void *fdt
+			__attribute__((unused)),
+			int offset __attribute__((unused)),
+			const char *pname __attribute__((unused)),
+			int *value __attribute__((unused)),
+			int *len __attribute__((unused)))
+{
+	return FWTS_OK;
+}
 #endif
 
 #endif
diff --git a/src/lib/src/fwts_devicetree.c b/src/lib/src/fwts_devicetree.c
index baa4e6b..b8ef55f 100644
--- a/src/lib/src/fwts_devicetree.c
+++ b/src/lib/src/fwts_devicetree.c
@@ -20,6 +20,7 @@ 
 #define _GNU_SOURCE
 
 #include <stdio.h>
+#include <libfdt.h>
 
 #include "fwts.h"
 
@@ -64,3 +65,36 @@  int fwts_devicetree_read(fwts_framework *fwts)
 	return FWTS_OK;
 }
 
+int fwts_dt_property_read_u32(void *fdt, int offset, const char *pname,
+			      int *value)
+{
+	int len;
+	const int *buf;
+
+	buf = fdt_getprop(fdt, offset, pname, &len);
+	if (buf == NULL) {
+		*value = len;
+		return FWTS_ERROR;
+	}
+	*value = be32toh(*buf);
+	return FWTS_OK;
+}
+
+int fwts_dt_property_read_u32_arr(void *fdt, int offset, const char *pname,
+				  int *value, int *len)
+{
+	int i;
+	const int *buf;
+
+	buf = fdt_getprop(fdt, offset, pname, len);
+	if (buf == NULL) {
+		*value = *len;
+		return FWTS_ERROR;
+	}
+
+	*len = *len / sizeof(int);
+	*len = *len - 1;
+	for (i = 0; i < *len; i++)
+		value[i] = be32toh(buf[i]);
+	return FWTS_OK;
+}