Re: [PATCH 3/4] cpufreq: amd-pstate-ut: Tolerate aliased EPP preferences in the EPP test

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/1/26 11:52, K Prateek Nayak wrote:
> Hello Mario,
> 
> On 9/22/2026 12:32 AM, Mario Limonciello wrote:
>> @@ -385,12 +388,27 @@ static int amd_pstate_ut_epp(u32 index)
>>   		strreplace(buf, '\n', '\0');
>>   		/*
>>   		 * "dynamic" mode reports the EPP as "dynamic(profile:X)"
>> -		 * Trim at "(" and just compare tie the epp string.
>> +		 * Trim at "(" and just keep the preference name.
>>   		 */
>>   		strreplace(buf, '(', '\0');
>>   
>> -		if (strcmp(buf, epp_strings[i])) {
>> -			pr_err("String EPP value mismatch: %s != %s\n", buf, epp_strings[i]);
>> +		/*
>> +		 * The preference read back may legitimately differ from the one
>> +		 * written: some platforms (e.g. Zen6) program the same raw EPP
>> +		 * value for more than one named preference, so show() reports the
>> +		 * first name that matches that value. Rather than compare the
>> +		 * strings, re-write whatever name was reported and confirm it
>> +		 * programs the same raw EPP value. This verifies the show()/store()
>> +		 * mapping stays consistent while tolerating such aliasing.
>> +		 */
>> +		ret = store_energy_performance_preference(policy, buf, strlen(buf));
>> +		if (ret < 0)
>> +			goto out;
>> +		got_epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
>> +
>> +		if (want_epp != got_epp) {
> 
> 
> So this feels a bit hacky since it may be possible an epp
> may be programmed incorrectly in hardware and as a result
> it aliases with a different string.
> 
> Can we use amd_pstate_cpu_epp_values() as a source of truth
> to double verify?

That does sound like a better approach.  I'll test the below.

> 
> Something like below:
> 
>    (Only build tested on bleeding-edge with this patch reverted)
> 
> diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
> index be10ab90b820..506d7bb2e660 100644
> --- a/drivers/cpufreq/amd-pstate-ut.c
> +++ b/drivers/cpufreq/amd-pstate-ut.c
> @@ -304,12 +304,12 @@ static int amd_pstate_set_mode(enum amd_pstate_mode mode)
>   
>   static int amd_pstate_ut_epp(u32 index)
>   {
> -	static const char * const epp_strings[] = {
> -		"dynamic",
> -		"power",
> -		"balance_power",
> -		"balance_performance",
> -		"performance",
> +	enum energy_perf_value_index epp_modes[] = {
> +		EPP_INDEX_DYNAMIC,
> +		EPP_INDEX_POWERSAVE,
> +		EPP_INDEX_BALANCE_POWERSAVE,
> +		EPP_INDEX_BALANCE_PERFORMANCE,
> +		EPP_INDEX_PERFORMANCE,
>   	};
>   	char *buf __free(cleanup_page) = NULL;
>   	struct cpufreq_policy *policy = NULL;
> @@ -378,9 +378,9 @@ static int amd_pstate_ut_epp(u32 index)
>   		}
>   	}
>   
> -	for (i = 0; i < ARRAY_SIZE(epp_strings); i++) {
> +	for (i = 0; i < ARRAY_SIZE(epp_modes); i++) {
>   		memset(buf, 0, PAGE_SIZE);
> -		snprintf(buf, PAGE_SIZE, "%s", epp_strings[i]);
> +		snprintf(buf, PAGE_SIZE, "%s", energy_perf_strings[epp_modes[i]]);
>   		ret = store_energy_performance_preference(policy, buf, strlen(buf));
>   		if (ret < 0)
>   			goto out;
> @@ -396,8 +396,24 @@ static int amd_pstate_ut_epp(u32 index)
>   		 */
>   		strreplace(buf, '(', '\0');
>   
> -		if (strcmp(buf, epp_strings[i])) {
> -			pr_err("String EPP value mismatch: %s != %s\n", buf, epp_strings[i]);
> +		if (strcmp(buf, energy_perf_strings[epp_modes[i]])) {
> +			u8 epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
> +
> +			/*
> +			 * The preference read back may legitimately differ from the one
> +			 * written: some platforms (e.g. Zen6) program the same raw EPP
> +			 * value for more than one named preference, so show() reports the
> +			 * first name that matches that value.
> +			 *
> +			 * If strings mismatch, confirm the CPU is programmed with the correct
> +			 * EPP value for the mode. This verifies hardware programming while
> +			 * tolerating such aliasing in driver.
> +			 */
> +			if (epp == amd_pstate_cpu_epp_values(cpudata->cpu_type)[epp_modes[i]])
> +				continue;
> +
> +			pr_err("String EPP value mismatch: %s != %s\n",
> +			       buf, energy_perf_strings[epp_modes[i]]);
>   			ret = -EINVAL;
>   			goto out;
>   		}
> @@ -413,11 +429,12 @@ static int amd_pstate_ut_epp(u32 index)
>   		 * restore it here before dropping policy reference.
>   		 */
>   		if (orig_dynamic_epp) {
> +			const char * const dynamic_string = energy_perf_strings[epp_modes[0]];
>   			int ret2;
>   
>   			ret2 = store_energy_performance_preference(policy,
> -								  epp_strings[0],
> -								  strlen(epp_strings[0]));
> +								  dynamic_string,
> +								  strlen(dynamic_string));
>   			if (!ret && (ret2 < 0))
>   				ret = ret2;
>   		}
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index f27676e8e9ef..89f911d378ae 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -44,7 +44,6 @@
>   #include <acpi/cppc_acpi.h>
>   
>   #include <asm/msr.h>
> -#include <asm/processor.h>
>   #include <asm/cpufeature.h>
>   #include <asm/cpu_device_id.h>
>   
> @@ -89,46 +88,6 @@ static int cppc_state = AMD_PSTATE_UNDEFINED;
>   static bool amd_pstate_prefcore = true;
>   static struct quirk_entry *quirks;
>   
> -/*
> - * AMD Energy Preference Performance (EPP)
> - * The EPP is used in the CCLK DPM controller to drive
> - * the frequency that a core is going to operate during
> - * short periods of activity. EPP values will be utilized for
> - * different OS profiles (balanced, performance, power savings)
> - * display strings corresponding to EPP index in the
> - * energy_perf_strings[]
> - *	index		String
> - *-------------------------------------
> - *	0		default
> - *	1		performance
> - *	2		balance_performance
> - *	3		balance_power
> - *	4		power
> - *	5		custom (for raw EPP values)
> - *	6		dynamic (platform profile driven selection)
> - */
> -enum energy_perf_value_index {
> -	EPP_INDEX_DEFAULT = 0,
> -	EPP_INDEX_PERFORMANCE,
> -	EPP_INDEX_BALANCE_PERFORMANCE,
> -	EPP_INDEX_BALANCE_POWERSAVE,
> -	EPP_INDEX_POWERSAVE,
> -	EPP_INDEX_CUSTOM,
> -	EPP_INDEX_DYNAMIC,
> -	EPP_INDEX_MAX,
> -};
> -
> -static const char * const energy_perf_strings[] = {
> -	[EPP_INDEX_DEFAULT] = "default",
> -	[EPP_INDEX_PERFORMANCE] = "performance",
> -	[EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> -	[EPP_INDEX_BALANCE_POWERSAVE] = "balance_power",
> -	[EPP_INDEX_POWERSAVE] = "power",
> -	[EPP_INDEX_CUSTOM] = "custom",
> -	[EPP_INDEX_DYNAMIC] = "dynamic",
> -};
> -static_assert(ARRAY_SIZE(energy_perf_strings) == EPP_INDEX_MAX);
> -
>   /*
>    * The numeric EPP value programmed for each named preference. First dimension
>    * is CPU type (TOPO_CPU_TYPE_ANY for non-hybrid, TOPO_CPU_TYPE_PERFORMANCE/
> @@ -159,7 +118,7 @@ static_assert(ARRAY_SIZE(epp_values) == TOPO_CPU_TYPE_LOW_POWER + 1,
>    * Non-hybrid systems use TOPO_CPU_TYPE_ANY; hybrid systems use the CPU's
>    * actual type (PERFORMANCE/EFFICIENCY/LOW_POWER).
>    */
> -static inline u8 *amd_pstate_cpu_epp_values(enum x86_topology_cpu_type cpu_type)
> +u8 *amd_pstate_cpu_epp_values(enum x86_topology_cpu_type cpu_type)
>   {
>   	switch (cpu_type) {
>   	case TOPO_CPU_TYPE_PERFORMANCE:
> @@ -170,6 +129,7 @@ static inline u8 *amd_pstate_cpu_epp_values(enum x86_topology_cpu_type cpu_type)
>   		return epp_values[TOPO_CPU_TYPE_ANY];
>   	}
>   }
> +EXPORT_SYMBOL_FOR_PSTATE_UT(amd_pstate_cpu_epp_values);
>   
>   /**
>    * struct amd_pstate_epp_values - EPP values for the four named preferences
> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
> index c7189d177b81..422188c639b1 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -11,6 +11,8 @@
>   #include <linux/pm_qos.h>
>   #include <linux/platform_profile.h>
>   
> +#include <asm/processor.h>
> +
>   #if IS_MODULE(CONFIG_X86_AMD_PSTATE_UT)
>   #define EXPORT_SYMBOL_FOR_PSTATE_UT(symbol) \
>   	EXPORT_SYMBOL_FOR_MODULES(symbol, "amd-pstate-ut")
> @@ -162,6 +164,48 @@ enum amd_pstate_mode {
>   	AMD_PSTATE_MAX,
>   };
>   
> +/*
> + * AMD Energy Preference Performance (EPP)
> + * The EPP is used in the CCLK DPM controller to drive
> + * the frequency that a core is going to operate during
> + * short periods of activity. EPP values will be utilized for
> + * different OS profiles (balanced, performance, power savings)
> + * display strings corresponding to EPP index in the
> + * energy_perf_strings[]
> + *	index		String
> + *-------------------------------------
> + *	0		default
> + *	1		performance
> + *	2		balance_performance
> + *	3		balance_power
> + *	4		power
> + *	5		custom (for raw EPP values)
> + *	6		dynamic (platform profile driven selection)
> + */
> +enum energy_perf_value_index {
> +	EPP_INDEX_DEFAULT = 0,
> +	EPP_INDEX_PERFORMANCE,
> +	EPP_INDEX_BALANCE_PERFORMANCE,
> +	EPP_INDEX_BALANCE_POWERSAVE,
> +	EPP_INDEX_POWERSAVE,
> +	EPP_INDEX_CUSTOM,
> +	EPP_INDEX_DYNAMIC,
> +	EPP_INDEX_MAX,
> +};
> +
> +static const char * const energy_perf_strings[] = {
> +	[EPP_INDEX_DEFAULT] = "default",
> +	[EPP_INDEX_PERFORMANCE] = "performance",
> +	[EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
> +	[EPP_INDEX_BALANCE_POWERSAVE] = "balance_power",
> +	[EPP_INDEX_POWERSAVE] = "power",
> +	[EPP_INDEX_CUSTOM] = "custom",
> +	[EPP_INDEX_DYNAMIC] = "dynamic",
> +};
> +static_assert(ARRAY_SIZE(energy_perf_strings) == EPP_INDEX_MAX);
> +
> +u8 *amd_pstate_cpu_epp_values(enum x86_topology_cpu_type cpu_type);
> +
>   static inline u8 freq_to_perf(union perf_cached perf, u32 nominal_freq, unsigned int freq_val)
>   {
>   	u32 perf_val = DIV_ROUND_UP_ULL((u64)freq_val * perf.nominal_perf, nominal_freq);
> ---
> 
> Thoughts?
> 
>> +			pr_err("EPP mapping inconsistent: %s programmed %u but %s programmed %u\n",
>> +			       epp_strings[i], want_epp, buf, got_epp);
>>   			ret = -EINVAL;
>>   			goto out;
>>   		}
>