Re: [PATCH v5 5/5] cpufreq/amd-pstate-ut: Add a unit test for raw EPP

["Gautham R. Shenoy" <gautham.shenoy@amd.com>]

Hello Mario,

On Mon, Jan 05, 2026 at 11:14:41PM -0600, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Ensure that all supported raw EPP values work properly.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate-ut.c | 58 +++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index 447b9aa5ce40b..d51233b753db6 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -26,6 +26,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/mm.h>
>  #include <linux/fs.h>
>  #include <linux/cleanup.h>
>  
> @@ -35,6 +36,7 @@
>  
>  #include "amd-pstate.h"
>  
> +DEFINE_FREE(cleanup_page, void *, if (_T) free_page((unsigned long)_T))
>  
>  struct amd_pstate_ut_struct {
>  	const char *name;
> @@ -48,6 +50,7 @@ static int amd_pstate_ut_acpi_cpc_valid(u32 index);
>  static int amd_pstate_ut_check_enabled(u32 index);
>  static int amd_pstate_ut_check_perf(u32 index);
>  static int amd_pstate_ut_check_freq(u32 index);
> +static int amd_pstate_ut_epp(u32 index);
>  static int amd_pstate_ut_check_driver(u32 index);
>  
>  static struct amd_pstate_ut_struct amd_pstate_ut_cases[] = {
> @@ -55,6 +58,7 @@ static struct amd_pstate_ut_struct amd_pstate_ut_cases[] = {
>  	{"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_epp",              amd_pstate_ut_epp              },
>  	{"amd_pstate_ut_check_driver",	   amd_pstate_ut_check_driver     }
>  };
>  
> @@ -241,6 +245,60 @@ static int amd_pstate_set_mode(enum amd_pstate_mode mode)
>  	return amd_pstate_update_status(mode_str, strlen(mode_str));
>  }
>  
> +static int amd_pstate_ut_epp(u32 index)
> +{
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
> +	void *buf __free(cleanup_page) = NULL;
> +	struct amd_cpudata *cpudata;
> +	int ret, cpu = 0;
> +	u16 epp;
> +
> +	policy = cpufreq_cpu_get(cpu);
> +	if (!policy)
> +		return -ENODEV;
> +
> +	cpudata = policy->driver_data;
> +
> +	/* disable dynamic EPP before running test */
> +	if (cpudata->dynamic_epp) {
> +		pr_debug("Dynamic EPP is enabled, disabling it\n");
> +		amd_pstate_clear_dynamic_epp(policy);
> +	}
> +
> +	buf = (void *)__get_free_page(GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = amd_pstate_set_mode(AMD_PSTATE_ACTIVE);
> +	if (ret)
> +		return ret;
> +
> +	for (epp = 0; epp <= U8_MAX; epp++) {
> +		u8 val;
> +
> +		/* write all EPP values */
> +		memset(buf, 0, sizeof(*buf));
> +		snprintf(buf, PAGE_SIZE, "%d", epp);
> +		ret = store_energy_performance_preference(policy, buf, sizeof(*buf));
> +		if (ret < 0)
> +			return ret;
> +
> +		/* check if the EPP value reads back correctly for raw numbers */
> +		memset(buf, 0, sizeof(*buf));
> +		ret = show_energy_performance_preference(policy, buf);
> +		if (ret < 0)
> +			return ret;
> +		strreplace(buf, '\n', '\0');
> +		ret = kstrtou8(buf, 0, &val);
> +		if (!ret && epp != val) {
> +			pr_err("Raw EPP value mismatch: %d != %d\n", epp, val);
> +			return -EINVAL;
> +		}

The loop validates that writing a raw integer reads back as an
integer, but the commit message for Patch 4 ("Add support for raw
EPP writes") promises:

    "If the last value written was an integer then an integer will
     be returned.  If the last value written was a string then a
     string will be returned."

The string-to-string direction is not tested.  After the raw integer
loop, raw_epp is left as true.  Writing "performance" should flip
raw_epp to false and reading back should return the string
"performance", not the integer "0".  The same applies to
"balance_performance", "balance_power", and "power".

This would also exercise the transition from raw_epp=true to
raw_epp=false, which is where a regression would most likely appear.

> +	}

Also, the test forces AMD_PSTATE_ACTIVE mode here but never saves or
restores the original mode.  It also disables dynamic EPP above
without re-enabling it.

Shouldn't amd_pstate_ut_epp() save the mode and dynamic_epp state at
the start and restore both before returning like the other tests, for
e.g., amd_pstate_ut_check_driver() do?

> +
> +	return 0;
> +}
> +
>  static int amd_pstate_ut_check_driver(u32 index)
>  {
>  	enum amd_pstate_mode mode1, mode2 = AMD_PSTATE_DISABLE;
> 
> -- 
> 2.43.0
> 

-- 
Thanks and Regards
gautham.