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

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

On 3/21/2025 7:58 AM, 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 e671bc7d15508..d0c5c0aa3cc94 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>
>  
> @@ -33,6 +34,7 @@
>  
>  #include "amd-pstate.h"
>  
> +DEFINE_FREE(free_page, void *, if (_T) free_page((unsigned long)_T))
>  
>  struct amd_pstate_ut_struct {
>  	const char *name;
> @@ -46,6 +48,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[] = {
> @@ -53,6 +56,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     }
>  };
>  
> @@ -239,6 +243,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(free_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);

This causes a deadlock, as we are holding cpufreq_policy reference and calling set_mode(), which 
needs to unregister the cpufreq subsystem (i.e. needs all cpufreq_policy references freed). 

We might need to drop and retake the reference around set_mode(). Also make cpudata variable NULL 
to avoid anyone using a stale cpudata pointer later on by mistake.
OR 
Directly call set_epp() instead of store_energy_performance_preference() and eliminate the need 
of policy ref after set_mode().

> +	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;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int amd_pstate_ut_check_driver(u32 index)
>  {
>  	enum amd_pstate_mode mode1, mode2 = AMD_PSTATE_DISABLE;