[PATCH 0/2] amd-pstate: Add unit tests for state machine

[PATCH 0/2] amd-pstate: Add unit tests for state machine

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

The state machine for amd-pstate allows changing from any supported
mode to another.  This is a relatively large matrix of possible moves
that doesn't have test coverage.

Walk through the matrix as part of amd-pstate-ut tests to make sure
that all transitions work.

Mario Limonciello (2):
  cpufreq/amd-pstate: Export symbols for changing modes
  cpufreq/amd-pstate-ut: Add test case for mode switches

 drivers/cpufreq/amd-pstate-ut.c | 41 ++++++++++++++++++++++++++++++++-
 drivers/cpufreq/amd-pstate.c    | 23 ++++++++----------
 drivers/cpufreq/amd-pstate.h    | 14 +++++++++++
 3 files changed, 64 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH 1/2] cpufreq/amd-pstate: Export symbols for changing modes

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

In order to effectively test all mode switch combinations export
everything necessarily for amd-pstate-ut to trigger a mode switch.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 23 ++++++++++-------------
 drivers/cpufreq/amd-pstate.h | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 073ca9caf52ac..93adde45bebce 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -60,18 +60,6 @@
 #define AMD_CPPC_EPP_BALANCE_POWERSAVE		0xBF
 #define AMD_CPPC_EPP_POWERSAVE			0xFF
 
-/*
- * enum amd_pstate_mode - driver working mode of amd pstate
- */
-enum amd_pstate_mode {
-	AMD_PSTATE_UNDEFINED = 0,
-	AMD_PSTATE_DISABLE,
-	AMD_PSTATE_PASSIVE,
-	AMD_PSTATE_ACTIVE,
-	AMD_PSTATE_GUIDED,
-	AMD_PSTATE_MAX,
-};
-
 static const char * const amd_pstate_mode_string[] = {
 	[AMD_PSTATE_UNDEFINED]   = "undefined",
 	[AMD_PSTATE_DISABLE]     = "disable",
@@ -81,6 +69,14 @@ static const char * const amd_pstate_mode_string[] = {
 	NULL,
 };
 
+const char *amd_pstate_get_mode_string(enum amd_pstate_mode mode)
+{
+	if (mode < 0 || mode >= AMD_PSTATE_MAX)
+		return NULL;
+	return amd_pstate_mode_string[mode];
+}
+EXPORT_SYMBOL_GPL(amd_pstate_get_mode_string);
+
 struct quirk_entry {
 	u32 nominal_freq;
 	u32 lowest_freq;
@@ -1352,7 +1348,7 @@ static ssize_t amd_pstate_show_status(char *buf)
 	return sysfs_emit(buf, "%s\n", amd_pstate_mode_string[cppc_state]);
 }
 
-static int amd_pstate_update_status(const char *buf, size_t size)
+int amd_pstate_update_status(const char *buf, size_t size)
 {
 	int mode_idx;
 
@@ -1369,6 +1365,7 @@ static int amd_pstate_update_status(const char *buf, size_t size)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(amd_pstate_update_status);
 
 static ssize_t status_show(struct device *dev,
 			   struct device_attribute *attr, char *buf)
diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
index cc8bb2bc325aa..cd573bc6b6db8 100644
--- a/drivers/cpufreq/amd-pstate.h
+++ b/drivers/cpufreq/amd-pstate.h
@@ -103,4 +103,18 @@ struct amd_cpudata {
 	bool	boost_state;
 };
 
+/*
+ * enum amd_pstate_mode - driver working mode of amd pstate
+ */
+enum amd_pstate_mode {
+	AMD_PSTATE_UNDEFINED = 0,
+	AMD_PSTATE_DISABLE,
+	AMD_PSTATE_PASSIVE,
+	AMD_PSTATE_ACTIVE,
+	AMD_PSTATE_GUIDED,
+	AMD_PSTATE_MAX,
+};
+const char *amd_pstate_get_mode_string(enum amd_pstate_mode mode);
+int amd_pstate_update_status(const char *buf, size_t size);
+
 #endif /* _LINUX_AMD_PSTATE_H */
-- 
2.43.0

[PATCH 2/2] cpufreq/amd-pstate-ut: Add test case for mode switches

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

There is a state machine in the amd-pstate driver utilized for
switches for all modes. To make sure that cleanup and setup works
properly for each mode add a unit test case that tries all
combinations.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate-ut.c | 41 ++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
index b7318669485e4..c291b3dbec381 100644
--- a/drivers/cpufreq/amd-pstate-ut.c
+++ b/drivers/cpufreq/amd-pstate-ut.c
@@ -54,12 +54,14 @@ 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 void amd_pstate_ut_check_driver(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       }
+	{"amd_pstate_ut_check_freq",       amd_pstate_ut_check_freq       },
+	{"amd_pstate_ut_check_driver",	   amd_pstate_ut_check_driver     }
 };
 
 static bool get_shared_mem(void)
@@ -257,6 +259,43 @@ static void amd_pstate_ut_check_freq(u32 index)
 	cpufreq_cpu_put(policy);
 }
 
+static int amd_pstate_set_mode(enum amd_pstate_mode mode)
+{
+	const char *mode_str = amd_pstate_get_mode_string(mode);
+
+	pr_debug("->setting mode to %s\n", mode_str);
+
+	return amd_pstate_update_status(mode_str, strlen(mode_str));
+}
+
+static void amd_pstate_ut_check_driver(u32 index)
+{
+	enum amd_pstate_mode mode1, mode2;
+	int ret;
+
+	for (mode1 = AMD_PSTATE_DISABLE; mode1 < AMD_PSTATE_MAX; mode1++) {
+		ret = amd_pstate_set_mode(mode1);
+		if (ret)
+			goto out;
+		for (mode2 = AMD_PSTATE_DISABLE; mode2 < AMD_PSTATE_MAX; mode2++) {
+			if (mode1 == mode2)
+				continue;
+			ret = amd_pstate_set_mode(mode2);
+			if (ret)
+				goto out;
+		}
+	}
+out:
+	if (ret)
+		pr_warn("%s: failed to update status for %s->%s: %d\n", __func__,
+			amd_pstate_get_mode_string(mode1),
+			amd_pstate_get_mode_string(mode2), ret);
+
+	amd_pstate_ut_cases[index].result = ret ?
+					    AMD_PSTATE_UT_RESULT_FAIL :
+					    AMD_PSTATE_UT_RESULT_PASS;
+}
+
 static int __init amd_pstate_ut_init(void)
 {
 	u32 i = 0, arr_size = ARRAY_SIZE(amd_pstate_ut_cases);
-- 
2.43.0

RE: [PATCH 1/2] cpufreq/amd-pstate: Export symbols for changing modes

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Mario,

> -----Original Message-----
> From: Mario Limonciello <superm1@kernel.org>
> Sent: Sunday, September 1, 2024 10:49 AM
> To: Meng, Li (Jassmine) <Li.Meng@amd.com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Cc: open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <linux-
> kernel@vger.kernel.org>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@vger.kernel.org>; ptr1337@cachyos.org; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH 1/2] cpufreq/amd-pstate: Export symbols for changing
> modes
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> In order to effectively test all mode switch combinations export everything
> necessarily for amd-pstate-ut to trigger a mode switch.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 23 ++++++++++-------------
> drivers/cpufreq/amd-pstate.h | 14 ++++++++++++++
>  2 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 073ca9caf52ac..93adde45bebce 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -60,18 +60,6 @@
>  #define AMD_CPPC_EPP_BALANCE_POWERSAVE               0xBF
>  #define AMD_CPPC_EPP_POWERSAVE                       0xFF
>
> -/*
> - * enum amd_pstate_mode - driver working mode of amd pstate
> - */
> -enum amd_pstate_mode {
> -     AMD_PSTATE_UNDEFINED = 0,
> -     AMD_PSTATE_DISABLE,
> -     AMD_PSTATE_PASSIVE,
> -     AMD_PSTATE_ACTIVE,
> -     AMD_PSTATE_GUIDED,
> -     AMD_PSTATE_MAX,
> -};
> -
>  static const char * const amd_pstate_mode_string[] = {
>       [AMD_PSTATE_UNDEFINED]   = "undefined",
>       [AMD_PSTATE_DISABLE]     = "disable",
> @@ -81,6 +69,14 @@ static const char * const amd_pstate_mode_string[] =
> {
>       NULL,
>  };
>
> +const char *amd_pstate_get_mode_string(enum amd_pstate_mode mode)
> {
> +     if (mode < 0 || mode >= AMD_PSTATE_MAX)
> +             return NULL;
> +     return amd_pstate_mode_string[mode];
> +}
> +EXPORT_SYMBOL_GPL(amd_pstate_get_mode_string);
> +
>  struct quirk_entry {
>       u32 nominal_freq;
>       u32 lowest_freq;
> @@ -1352,7 +1348,7 @@ static ssize_t amd_pstate_show_status(char *buf)
>       return sysfs_emit(buf, "%s\n",
> amd_pstate_mode_string[cppc_state]);
>  }
>
> -static int amd_pstate_update_status(const char *buf, size_t size)
> +int amd_pstate_update_status(const char *buf, size_t size)
>  {
>       int mode_idx;
>
> @@ -1369,6 +1365,7 @@ static int amd_pstate_update_status(const char
> *buf, size_t size)
>
>       return 0;
>  }
> +EXPORT_SYMBOL_GPL(amd_pstate_update_status);
>
>  static ssize_t status_show(struct device *dev,
>                          struct device_attribute *attr, char *buf) diff --git
> a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h index
> cc8bb2bc325aa..cd573bc6b6db8 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -103,4 +103,18 @@ struct amd_cpudata {
>       bool    boost_state;
>  };
>
> +/*
> + * enum amd_pstate_mode - driver working mode of amd pstate  */ enum
> +amd_pstate_mode {
> +     AMD_PSTATE_UNDEFINED = 0,
> +     AMD_PSTATE_DISABLE,
> +     AMD_PSTATE_PASSIVE,
> +     AMD_PSTATE_ACTIVE,
> +     AMD_PSTATE_GUIDED,
> +     AMD_PSTATE_MAX,
> +};
> +const char *amd_pstate_get_mode_string(enum amd_pstate_mode mode);
> int
> +amd_pstate_update_status(const char *buf, size_t size);
> +
>  #endif /* _LINUX_AMD_PSTATE_H */
> --
> 2.43.0

LGTM,  move the amd_pstate_mode to header file that will be used by UT test .
Thanks for the improvement.

Reviewed-by: Perry Yuan <Perry.Yuan@amd.com>

RE: [PATCH 2/2] cpufreq/amd-pstate-ut: Add test case for mode switches

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Mario Limonciello <superm1@kernel.org>
> Sent: Sunday, September 1, 2024 10:49 AM
> To: Meng, Li (Jassmine) <Li.Meng@amd.com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Cc: open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <linux-
> kernel@vger.kernel.org>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@vger.kernel.org>; ptr1337@cachyos.org; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH 2/2] cpufreq/amd-pstate-ut: Add test case for mode
> switches
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> There is a state machine in the amd-pstate driver utilized for switches for all
> modes. To make sure that cleanup and setup works properly for each mode
> add a unit test case that tries all combinations.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate-ut.c | 41
> ++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-
> ut.c index b7318669485e4..c291b3dbec381 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -54,12 +54,14 @@ 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 void amd_pstate_ut_check_driver(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       }
> +     {"amd_pstate_ut_check_freq",       amd_pstate_ut_check_freq       },
> +     {"amd_pstate_ut_check_driver",
> amd_pstate_ut_check_driver     }
>  };
>
>  static bool get_shared_mem(void)
> @@ -257,6 +259,43 @@ static void amd_pstate_ut_check_freq(u32 index)
>       cpufreq_cpu_put(policy);
>  }
>
> +static int amd_pstate_set_mode(enum amd_pstate_mode mode) {
> +     const char *mode_str = amd_pstate_get_mode_string(mode);
> +
> +     pr_debug("->setting mode to %s\n", mode_str);
> +
> +     return amd_pstate_update_status(mode_str, strlen(mode_str)); }
> +
> +static void amd_pstate_ut_check_driver(u32 index) {
> +     enum amd_pstate_mode mode1, mode2;
> +     int ret;
> +
> +     for (mode1 = AMD_PSTATE_DISABLE; mode1 < AMD_PSTATE_MAX;
> mode1++) {
> +             ret = amd_pstate_set_mode(mode1);
> +             if (ret)
> +                     goto out;
> +             for (mode2 = AMD_PSTATE_DISABLE; mode2 <
> AMD_PSTATE_MAX; mode2++) {
> +                     if (mode1 == mode2)
> +                             continue;
> +                     ret = amd_pstate_set_mode(mode2);
> +                     if (ret)
> +                             goto out;
> +             }
> +     }

Dose the mode switching test need to add some delay between the previous and new mode ?
If lowlevel power firmware failed to handle the modes frequently,  you can consider adding delay in future.
Besides that, the patch looks good to me.

Reviewed-by: Perry Yuan <perry.yuan@amd.com>

> +out:
> +     if (ret)
> +             pr_warn("%s: failed to update status for %s->%s: %d\n",
> __func__,
> +                     amd_pstate_get_mode_string(mode1),
> +                     amd_pstate_get_mode_string(mode2), ret);
> +
> +     amd_pstate_ut_cases[index].result = ret ?
> +                                         AMD_PSTATE_UT_RESULT_FAIL :
> +                                         AMD_PSTATE_UT_RESULT_PASS;
> +}
> +
>  static int __init amd_pstate_ut_init(void)  {
>       u32 i = 0, arr_size = ARRAY_SIZE(amd_pstate_ut_cases);
> --
> 2.43.0