[PATCH 1/4] cpufreq: amd-pstate: Restore previous mode when changing driver mode fails

[PATCH 1/4] cpufreq: amd-pstate: Restore previous mode when changing driver mode fails

[Mario Limonciello]

amd_pstate_change_driver_mode() unregisters the currently active driver
before registering the requested mode.  If amd_pstate_register_driver()
fails for the new mode, the function returns the error without restoring
anything, leaving the system with no cpufreq scaling driver at all until
a valid mode is manually re-selected.

Remember the mode that was active before the transition and, if
registering the requested mode fails, register the previous mode again so
the system keeps a working scaling driver.  The original error is still
returned to the caller so the sysfs write reports the failure.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-5b138ddb-c88b-4f82-8a2d-f39446b625d4
Fixes: 3ca7bc818d8c ("cpufreq: amd-pstate: Add guided mode control support via sysfs")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 03727d9b2ff84..f633a8e349255 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1833,6 +1833,7 @@ static int amd_pstate_change_mode_without_dvr_change(int mode)
 
 static int amd_pstate_change_driver_mode(int mode)
 {
+	int old_mode = cppc_state;
 	int ret;
 
 	lockdep_assert_held(&amd_pstate_driver_lock);
@@ -1842,10 +1843,16 @@ static int amd_pstate_change_driver_mode(int mode)
 		return ret;
 
 	ret = amd_pstate_register_driver(mode);
-	if (ret)
-		return ret;
+	if (ret) {
+		pr_err("Failed to register %s mode, restoring %s mode\n",
+		       amd_pstate_get_mode_string(mode),
+		       amd_pstate_get_mode_string(old_mode));
+		if (amd_pstate_register_driver(old_mode))
+			pr_err("Failed to restore %s mode\n",
+			       amd_pstate_get_mode_string(old_mode));
+	}
 
-	return 0;
+	return ret;
 }
 
 static cppc_mode_transition_fn mode_state_machine[AMD_PSTATE_MAX][AMD_PSTATE_MAX] = {
-- 
2.43.0

[PATCH 2/4] cpufreq: amd-pstate: Propagate cppc_set_auto_sel() errors on mode change

[Mario Limonciello]

amd_pstate_change_mode_without_dvr_change() iterates over the online CPUs
and calls cppc_set_auto_sel() to enable or disable hardware autonomous
selection, but discards its return value and unconditionally returns 0.

If the firmware rejects the request, the failure is hidden: the cpufreq
core records the mode transition as successful while the hardware stays in
its previous autonomous-selection state.  The software mode and the actual
hardware behaviour then disagree, breaking the expected frequency scaling.

Check the return value of cppc_set_auto_sel() and propagate the first
error to the caller so the sysfs write reports the failure.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-d114a94a-655b-43a6-91b1-9484889726c9
Fixes: 3ca7bc818d8c ("cpufreq: amd-pstate: Add guided mode control support via sysfs")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index f633a8e349255..7a2174b5911e4 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1818,6 +1818,7 @@ static int amd_pstate_unregister_driver(int dummy)
 static int amd_pstate_change_mode_without_dvr_change(int mode)
 {
 	int cpu = 0;
+	int ret;
 
 	cppc_state = mode;
 
@@ -1825,7 +1826,9 @@ static int amd_pstate_change_mode_without_dvr_change(int mode)
 		return 0;
 
 	for_each_online_cpu(cpu) {
-		cppc_set_auto_sel(cpu, (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1);
+		ret = cppc_set_auto_sel(cpu, (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
-- 
2.43.0

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

[Mario Limonciello]

amd_pstate_ut_epp() writes each named EPP preference and requires it to
read back as the same string.  That assumes a one-to-one mapping between
named preferences and raw EPP values, which does not hold on all
platforms.  Zen6 client, for example, programs the same raw EPP value for
more than one named preference (power and balance_power are both 64 on
performance cores), so show_energy_performance_preference() reports the
first name matching that value and the string comparison fails:

  amd_pstate_ut: String EPP value mismatch: balance_power != power
  amd_pstate_ut: 5    amd_pstate_ut_epp     fail: -22!

Instead of comparing the strings, record the raw EPP value programmed by
the written preference, then re-write whatever name show() reported and
confirm it programs the same raw EPP value.  This tolerates several
preferences aliasing to one value while still catching a genuinely
inconsistent show()/store() mapping.

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

diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
index c2c1a166b3a9e..f5888beb767a8 100644
--- a/drivers/cpufreq/amd-pstate-ut.c
+++ b/drivers/cpufreq/amd-pstate-ut.c
@@ -372,11 +372,14 @@ static int amd_pstate_ut_epp(u32 index)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(epp_strings); i++) {
+		u8 want_epp, got_epp;
+
 		memset(buf, 0, PAGE_SIZE);
 		snprintf(buf, PAGE_SIZE, "%s", epp_strings[i]);
 		ret = store_energy_performance_preference(policy, buf, strlen(buf));
 		if (ret < 0)
 			goto out;
+		want_epp = FIELD_GET(AMD_CPPC_EPP_PERF_MASK, cpudata->cppc_req_cached);
 
 		memset(buf, 0, PAGE_SIZE);
 		ret = show_energy_performance_preference(policy, buf);
@@ -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) {
+			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;
 		}
-- 
2.43.0

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

[K Prateek Nayak]

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?

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;
>  		}

-- 
Thanks and Regards,
Prateek

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

[Mario Limonciello]

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;
>>   		}
> 

[PATCH 4/4] cpufreq: amd-pstate-ut: Don't require max_freq >= nominal_freq

[Mario Limonciello]

amd_pstate_ut_check_freq() asserts the ordering

  max_freq >= nominal_freq > lowest_nonlinear_freq > min_freq > 0

but max_freq >= nominal_freq does not hold on all parts.  On
heterogeneous Zen6 client, amd_get_max_frequency() returns a hardcoded
per-CPU-type maximum frequency, and the low-power cores cap below the
shared nominal reference frequency.  On a 20-core Zen6 sample the four
low-power cores report a maximum of 2399 MHz against a nominal of
2400 MHz, so the test fails:

```
  amd_pstate_ut: amd_pstate_ut_check_freq cpu8 max=2399000 >= nominal=2400000 ... the formula is incorrect!
  amd_pstate_ut: 4    amd_pstate_ut_check_freq      fail: -22!
```

The maximum frequency is a boost ceiling and its relationship to the
nominal reference is platform dependent, so only require it to sit above
the lowest nonlinear frequency, which holds on every core.  The rest of
the ordering (nominal_freq > lowest_nonlinear_freq >= min_freq > 0) is
kept unchanged.

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

diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
index f5888beb767a8..e4e85514d0b67 100644
--- a/drivers/cpufreq/amd-pstate-ut.c
+++ b/drivers/cpufreq/amd-pstate-ut.c
@@ -216,7 +216,14 @@ static int amd_pstate_ut_check_perf(u32 index)
 
 /*
  * Check if frequency values are reasonable.
- * max_freq >= nominal_freq > lowest_nonlinear_freq > min_freq > 0
+ * nominal_freq > lowest_nonlinear_freq >= min_freq > 0
+ * max_freq >= lowest_nonlinear_freq
+ *
+ * On most parts the boost frequency is the highest, i.e.
+ * max_freq >= nominal_freq. On heterogeneous designs (e.g. Zen6) a
+ * low-power core can have a maximum frequency below the shared nominal
+ * reference frequency, so only require the boost frequency to sit above
+ * the lowest nonlinear frequency here.
  * check max freq when set support boost mode.
  */
 static int amd_pstate_ut_check_freq(u32 index)
@@ -235,11 +242,11 @@ static int amd_pstate_ut_check_freq(u32 index)
 		cpudata = policy->driver_data;
 		perf = READ_ONCE(cpudata->perf);
 
-		if (!((policy->cpuinfo.max_freq >= cpudata->nominal_freq) &&
+		if (!((policy->cpuinfo.max_freq >= cpudata->lowest_nonlinear_freq) &&
 			(cpudata->nominal_freq > cpudata->lowest_nonlinear_freq) &&
 			(cpudata->lowest_nonlinear_freq >= policy->cpuinfo.min_freq) &&
 			(policy->cpuinfo.min_freq > 0))) {
-			pr_err("%s cpu%d max=%d >= nominal=%d > lowest_nonlinear=%d > min=%d > 0, the formula is incorrect!\n",
+			pr_err("%s cpu%d max=%d, nominal=%d, lowest_nonlinear=%d, min=%d, the formula is incorrect!\n",
 				__func__, cpu, policy->cpuinfo.max_freq, cpudata->nominal_freq,
 				cpudata->lowest_nonlinear_freq, policy->cpuinfo.min_freq);
 			return -EINVAL;
-- 
2.43.0