diff mbox series

[V7,2/4] cpufreq: amd-pstate: Add test module for amd-pstate driver

Message ID 20220522115423.1147282-3-li.meng@amd.com (mailing list archive)
State Changes Requested, archived
Delegated to: Shuah Khan
Headers show
Series Add unit test module for AMD P-State driver | expand

Commit Message

Meng, Li (Jassmine) May 22, 2022, 11:54 a.m. UTC
Add amd-pstate-ut module, which is conceptually out-of-tree module
and provides ways for selftests/amd-pstate driver to test various
kernel module-related functionality. This module will be expected by
some of selftests to be present and loaded.

Signed-off-by: Meng Li <li.meng@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
---
 drivers/cpufreq/Kconfig.x86     |   7 +
 drivers/cpufreq/Makefile        |   1 +
 drivers/cpufreq/amd-pstate-ut.c | 293 ++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/cpufreq/amd-pstate-ut.c

Comments

Viresh Kumar May 23, 2022, 7:32 a.m. UTC | #1
On 22-05-22, 19:54, Meng Li wrote:
> Add amd-pstate-ut module, which is conceptually out-of-tree module
> and provides ways for selftests/amd-pstate driver to test various
> kernel module-related functionality. This module will be expected by
> some of selftests to be present and loaded.
> 
> Signed-off-by: Meng Li <li.meng@amd.com>
> Acked-by: Huang Rui <ray.huang@amd.com>
> ---
>  drivers/cpufreq/Kconfig.x86     |   7 +
>  drivers/cpufreq/Makefile        |   1 +
>  drivers/cpufreq/amd-pstate-ut.c | 293 ++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+)
>  create mode 100644 drivers/cpufreq/amd-pstate-ut.c

I wonder if this should be moved to selftest directories instead ?
Meng, Li (Jassmine) May 23, 2022, 8:25 a.m. UTC | #2
[AMD Official Use Only - General]



> -----Original Message-----
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Sent: Monday, May 23, 2022 3:32 PM
> To: Meng, Li (Jassmine) <Li.Meng@amd.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>; Huang, Ray
> <Ray.Huang@amd.com>; linux-pm@vger.kernel.org; Rafael J . Wysocki
> <rafael.j.wysocki@intel.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Sharma, Deepak
> <Deepak.Sharma@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>;
> Yuan, Perry <Perry.Yuan@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>;
> Borislav Petkov <bp@alien8.de>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V7 2/4] cpufreq: amd-pstate: Add test module for amd-
> pstate driver
> 
> [CAUTION: External Email]
> 
> On 22-05-22, 19:54, Meng Li wrote:
> > Add amd-pstate-ut module, which is conceptually out-of-tree module and
> > provides ways for selftests/amd-pstate driver to test various kernel
> > module-related functionality. This module will be expected by some of
> > selftests to be present and loaded.
> >
> > Signed-off-by: Meng Li <li.meng@amd.com>
> > Acked-by: Huang Rui <ray.huang@amd.com>
> > ---
> >  drivers/cpufreq/Kconfig.x86     |   7 +
> >  drivers/cpufreq/Makefile        |   1 +
> >  drivers/cpufreq/amd-pstate-ut.c | 293
> > ++++++++++++++++++++++++++++++++
> >  3 files changed, 301 insertions(+)
> >  create mode 100644 drivers/cpufreq/amd-pstate-ut.c
> 
> I wonder if this should be moved to selftest directories instead ?
> 
> --
> Viresh

Thanks.
We took Shuah's comments and put test driver under drivers/cpufreq.

[Meng, Li (Jassmine)]
Shuah Khan May 25, 2022, 4:05 p.m. UTC | #3
On 5/22/22 5:54 AM, Meng Li wrote:
> Add amd-pstate-ut module, which is conceptually out-of-tree module

I didn't notice this before. Let's get rid of "out-of-tree" workding.

> and provides ways for selftests/amd-pstate driver to test various
> kernel module-related functionality. This module will be expected by
> some of selftests to be present and loaded.
> 

What does "various kernel module-related functionality" mean. Can we
rephrase this to say "this module is used by kselftest to unit test
amd-pstate functionality" ?

> Signed-off-by: Meng Li <li.meng@amd.com>
> Acked-by: Huang Rui <ray.huang@amd.com>
> ---

[snip]

> +
> +module_init(amd_pstate_ut_init);
> +module_exit(amd_pstate_ut_exit);
> +
> +MODULE_AUTHOR("Meng Li <li.meng@amd.com>");
> +MODULE_DESCRIPTION("Kselftest for AMD P-state driver");

Change this to "AMD P-state driver Test module"

Having Kselftest in the name is confusing. It is a test driver
that kselftest amd_pstate_test depends on.

> +MODULE_LICENSE("GPL");
> 

With these above changes:

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index 55516043b656..fdd819069d72 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -51,6 +51,13 @@  config X86_AMD_PSTATE
 
 	  If in doubt, say N.
 
+config X86_AMD_PSTATE_UT
+	tristate "selftest for AMD Processor P-State driver"
+	depends on X86 && ACPI_PROCESSOR
+	default n
+	help
+	  This kernel module is used for testing. It's safe to say M here.
+
 config X86_ACPI_CPUFREQ
 	tristate "ACPI Processor P-States driver"
 	depends on ACPI_PROCESSOR
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 285de70af877..49b98c62c5af 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -30,6 +30,7 @@  amd_pstate-y				:= amd-pstate.o amd-pstate-trace.o
 
 obj-$(CONFIG_X86_ACPI_CPUFREQ)		+= acpi-cpufreq.o
 obj-$(CONFIG_X86_AMD_PSTATE)		+= amd_pstate.o
+obj-$(CONFIG_X86_AMD_PSTATE_UT)		+= amd-pstate-ut.o
 obj-$(CONFIG_X86_POWERNOW_K8)		+= powernow-k8.o
 obj-$(CONFIG_X86_PCC_CPUFREQ)		+= pcc-cpufreq.o
 obj-$(CONFIG_X86_POWERNOW_K6)		+= powernow-k6.o
diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
new file mode 100644
index 000000000000..a8d2783d98b5
--- /dev/null
+++ b/drivers/cpufreq/amd-pstate-ut.c
@@ -0,0 +1,293 @@ 
+// SPDX-License-Identifier: GPL-1.0-or-later
+/*
+ * AMD Processor P-state Frequency Driver Unit Test
+ *
+ * Copyright (C) 2022 Advanced Micro Devices, Inc. All Rights Reserved.
+ *
+ * Author: Meng Li <li.meng@amd.com>
+ *
+ * The AMD P-State Unit Test is a test module for testing the amd-pstate
+ * driver. 1) It can help all users to verify their processor support
+ * (SBIOS/Firmware or Hardware). 2) Kernel can have a basic function
+ * test to avoid the kernel regression during the update. 3) We can
+ * introduce more functional or performance tests to align the result
+ * together, it will benefit power and performance scale optimization.
+ *
+ * This driver implements basic framework with plans to enhance it with
+ * additional test cases to improve the depth and coverage of the test.
+ *
+ * See Documentation/admin-guide/pm/amd-pstate.rst Unit Tests for
+ * amd-pstate to get more detail.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/fs.h>
+#include <linux/amd-pstate.h>
+
+#include <acpi/cppc_acpi.h>
+
+/*
+ * Abbreviations:
+ * amd_pstate_ut: used as a shortform for AMD P-State unit test.
+ * It helps to keep variable names smaller, simpler
+ */
+enum amd_pstate_ut_result {
+	AMD_PSTATE_UT_RESULT_PASS,
+	AMD_PSTATE_UT_RESULT_FAIL,
+};
+
+struct amd_pstate_ut_struct {
+	const char *name;
+	void (*func)(u32 index);
+	enum amd_pstate_ut_result result;
+};
+
+/*
+ * Kernel module for testing the AMD P-State unit test
+ */
+static void amd_pstate_ut_acpi_cpc_valid(u32 index);
+static void amd_pstate_ut_check_enabled(u32 index);
+static void amd_pstate_ut_check_perf(u32 index);
+static void amd_pstate_ut_check_freq(u32 index);
+
+static struct amd_pstate_ut_struct amd_pstate_ut_cases[] = {
+	{"amd_pstate_ut_acpi_cpc_valid",   amd_pstate_ut_acpi_cpc_valid   },
+	{"amd_pstate_ut_check_enabled",    amd_pstate_ut_check_enabled    },
+	{"amd_pstate_ut_check_perf",       amd_pstate_ut_check_perf       },
+	{"amd_pstate_ut_check_freq",       amd_pstate_ut_check_freq       }
+};
+
+static bool get_shared_mem(void)
+{
+	bool result = false;
+	char path[] = "/sys/module/amd_pstate/parameters/shared_mem";
+	char buf[5] = {0};
+	struct file *filp = NULL;
+	loff_t pos = 0;
+	ssize_t ret;
+
+	if (!boot_cpu_has(X86_FEATURE_CPPC)) {
+		filp = filp_open(path, FMODE_PREAD, 0);
+		if (IS_ERR(filp))
+			pr_err("%s unable to open %s file!\n", __func__, path);
+		else {
+			ret = kernel_read(filp, &buf, sizeof(buf), &pos);
+			if (ret < 0)
+				pr_err("%s read %s file fail ret=%ld!\n",
+					__func__, path, (long)ret);
+			filp_close(filp, NULL);
+		}
+
+		if ('Y' == *buf)
+			result = true;
+	}
+
+	return result;
+}
+
+/*
+ * check the _CPC object is present in SBIOS.
+ */
+static void amd_pstate_ut_acpi_cpc_valid(u32 index)
+{
+	if (acpi_cpc_valid())
+		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
+	else {
+		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+		pr_err("%s the _CPC object is not present in SBIOS!\n", __func__);
+	}
+}
+
+static void amd_pstate_ut_pstate_enable(u32 index)
+{
+	int ret = 0;
+	u64 cppc_enable = 0;
+
+	ret = rdmsrl_safe(MSR_AMD_CPPC_ENABLE, &cppc_enable);
+	if (ret) {
+		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+		pr_err("%s rdmsrl_safe MSR_AMD_CPPC_ENABLE ret=%d error!\n", __func__, ret);
+		return;
+	}
+	if (cppc_enable)
+		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
+	else {
+		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+		pr_err("%s amd pstate must be enabled!\n", __func__);
+	}
+}
+
+/*
+ * check if amd pstate is enabled
+ */
+static void amd_pstate_ut_check_enabled(u32 index)
+{
+	if (get_shared_mem())
+		amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
+	else
+		amd_pstate_ut_pstate_enable(index);
+}
+
+/*
+ * check if performance values are reasonable.
+ * highest_perf >= nominal_perf > lowest_nonlinear_perf > lowest_perf > 0
+ */
+static void amd_pstate_ut_check_perf(u32 index)
+{
+	int cpu = 0, ret = 0;
+	u32 highest_perf = 0, nominal_perf = 0, lowest_nonlinear_perf = 0, lowest_perf = 0;
+	u64 cap1 = 0;
+	struct cppc_perf_caps cppc_perf;
+	struct cpufreq_policy *policy = NULL;
+	struct amd_cpudata *cpudata = NULL;
+
+	highest_perf = amd_get_highest_perf();
+
+	for_each_possible_cpu(cpu) {
+		policy = cpufreq_cpu_get(cpu);
+		if (!policy)
+			break;
+		cpudata = policy->driver_data;
+
+		if (get_shared_mem()) {
+			ret = cppc_get_perf_caps(cpu, &cppc_perf);
+			if (ret) {
+				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+				pr_err("%s cppc_get_perf_caps ret=%d error!\n", __func__, ret);
+				return;
+			}
+
+			nominal_perf = cppc_perf.nominal_perf;
+			lowest_nonlinear_perf = cppc_perf.lowest_nonlinear_perf;
+			lowest_perf = cppc_perf.lowest_perf;
+		} else {
+			ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &cap1);
+			if (ret) {
+				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+				pr_err("%s read CPPC_CAP1 ret=%d error!\n", __func__, ret);
+				return;
+			}
+
+			nominal_perf = AMD_CPPC_NOMINAL_PERF(cap1);
+			lowest_nonlinear_perf = AMD_CPPC_LOWNONLIN_PERF(cap1);
+			lowest_perf = AMD_CPPC_LOWEST_PERF(cap1);
+		}
+
+		if ((highest_perf != READ_ONCE(cpudata->highest_perf)) ||
+			(nominal_perf != READ_ONCE(cpudata->nominal_perf)) ||
+			(lowest_nonlinear_perf != READ_ONCE(cpudata->lowest_nonlinear_perf)) ||
+			(lowest_perf != READ_ONCE(cpudata->lowest_perf))) {
+			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+			pr_err("%s cpu%d highest=%d %d nominal=%d %d lowest_nonlinear=%d %d lowest=%d %d, they should be equal!\n",
+				__func__, cpu, highest_perf, cpudata->highest_perf,
+				nominal_perf, cpudata->nominal_perf,
+				lowest_nonlinear_perf, cpudata->lowest_nonlinear_perf,
+				lowest_perf, cpudata->lowest_perf);
+			return;
+		}
+
+		if (!((highest_perf >= nominal_perf) &&
+			(nominal_perf > lowest_nonlinear_perf) &&
+			(lowest_nonlinear_perf > lowest_perf) &&
+			(lowest_perf > 0))) {
+			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+			pr_err("%s cpu%d highest=%d >= nominal=%d > lowest_nonlinear=%d > lowest=%d > 0, the formula is incorrect!\n",
+				__func__, cpu, highest_perf, nominal_perf,
+				lowest_nonlinear_perf, lowest_perf);
+			return;
+		}
+	}
+
+	amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
+}
+
+/*
+ * Check if frequency values are reasonable.
+ * max_freq >= nominal_freq > lowest_nonlinear_freq > min_freq > 0
+ * check max freq when set support boost mode.
+ */
+static void amd_pstate_ut_check_freq(u32 index)
+{
+	int cpu = 0;
+	struct cpufreq_policy *policy = NULL;
+	struct amd_cpudata *cpudata = NULL;
+
+	for_each_possible_cpu(cpu) {
+		policy = cpufreq_cpu_get(cpu);
+		if (!policy)
+			break;
+		cpudata = policy->driver_data;
+
+		if (!((cpudata->max_freq >= cpudata->nominal_freq) &&
+			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
+			(cpudata->lowest_nonlinear_freq > cpudata->min_freq) &&
+			(cpudata->min_freq > 0))) {
+			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
+				__func__, cpu, cpudata->max_freq, cpudata->nominal_freq,
+				cpudata->lowest_nonlinear_freq, cpudata->min_freq);
+			return;
+		}
+
+		if (cpudata->min_freq != policy->min) {
+			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+			pr_err("%s cpu%d cpudata_min_freq=%d policy_min=%d, they should be equal!\n",
+				__func__, cpu, cpudata->min_freq, policy->min);
+			return;
+		}
+
+		if (cpudata->boost_supported) {
+			if ((policy->max == cpudata->max_freq) ||
+					(policy->max == cpudata->nominal_freq))
+				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
+			else {
+				amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+				pr_err("%s cpu%d policy_max=%d should be equal cpu_max=%d or cpu_nominal=%d !\n",
+					__func__, cpu, policy->max, cpudata->max_freq,
+					cpudata->nominal_freq);
+				return;
+			}
+		} else {
+			amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_FAIL;
+			pr_err("%s cpu%d must support boost!\n", __func__, cpu);
+			return;
+		}
+	}
+
+	amd_pstate_ut_cases[index].result = AMD_PSTATE_UT_RESULT_PASS;
+}
+
+static int __init amd_pstate_ut_init(void)
+{
+	u32 i = 0, arr_size = ARRAY_SIZE(amd_pstate_ut_cases);
+
+	for (i = 0; i < arr_size; i++) {
+		amd_pstate_ut_cases[i].func(i);
+		switch (amd_pstate_ut_cases[i].result) {
+		case AMD_PSTATE_UT_RESULT_PASS:
+			pr_info("%-4d %-20s\t success!\n", i+1, amd_pstate_ut_cases[i].name);
+			break;
+		case AMD_PSTATE_UT_RESULT_FAIL:
+		default:
+			pr_info("%-4d %-20s\t fail!\n", i+1, amd_pstate_ut_cases[i].name);
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static void __exit amd_pstate_ut_exit(void)
+{
+}
+
+module_init(amd_pstate_ut_init);
+module_exit(amd_pstate_ut_exit);
+
+MODULE_AUTHOR("Meng Li <li.meng@amd.com>");
+MODULE_DESCRIPTION("Kselftest for AMD P-state driver");
+MODULE_LICENSE("GPL");