[PATCH 0/6] cpufreq/amd-pstate: Dynamic EPP fixes

[PATCH 0/6] cpufreq/amd-pstate: Dynamic EPP fixes

[K Prateek Nayak]

Few rare race conditions were found when testing the Dynamic EPP feature
on EPYC platforms:

o Toggling dynamic EPP is essentially a driver switch into the same mode
  and can race with an admin-initiated driver switch which can cause the
  system to hang.

o If a profile change / power supply status change races with the driver
  unloading, it can overwrite the Floor Perf restored in the CPPC_REQ
  register that was meant to be left untouched until the next driver
  load.

o When dynamic mode is disabled, the default EPP hint is set based on
  the power supply mode at the time of driver load which can cause the
  battery to drain faster when starting off from AC supply and then
  switching to battery.

When fixing and testing the above, a couple of incorrect error codes
were noted which is also fixed in the series along with the unit test.

Series is based on mainline at v7.1-rc2 release tag. Thanks to Mario for
initial review. Feedback is greatly appreciated.
---
K Prateek Nayak (6):
  cpufreq/amd-pstate: Grab "amd_pstate_driver_lock" when toggling
    dynamic_epp
  cpufreq/amd-pstate: Return -ENOMEM on failure to allocate profile_name
  cpufreq/amd-pstate: Allow writes to dynamic_epp when state isn't
    modified
  cpufreq/amd-pstate: Reorder notifier unregistration and floor perf
    reset
  cpufreq/amd-pstate: Use "epp_default_dc" as default when dynamic_epp
    is disabled
  cpufreq/amd-pstate-ut: Drop policy reference before driver switch

 drivers/cpufreq/amd-pstate-ut.c | 36 ++++++++++++++++++++++++++-------
 drivers/cpufreq/amd-pstate.c    | 23 ++++++++++++++++-----
 2 files changed, 47 insertions(+), 12 deletions(-)


base-commit: 7fd2df204f342fc17d1a0bfcd474b24232fb0f32
-- 
2.34.1

[PATCH 1/6] cpufreq/amd-pstate: Grab "amd_pstate_driver_lock" when toggling dynamic_epp

[K Prateek Nayak]

Concurrently changing driver mode and dynamic_epp with:

    echo passive > /sys/devices/system/cpu/amd_pstate/status&
    echo disable > /sys/devices/system/cpu/amd_pstate/dynamic_epp&

hits the WARN_ON_ONCE() in static_key_disable_cpuslocked() and hangs the
system since both sysfs writes are trying to do
amd_pstate_change_driver_mode() without any synchronization.

Grab the "amd_pstate_driver_lock" mutex when modifying "dynamic_epp" to
prevent the two paths from racing with each other. Add a lockdep
assertion for "amd_pstate_driver_lock" in
amd_pstate_change_driver_mode() to formalize the dependency.

Since "cppc_mode" is stable under "amd_pstate_driver_lock", only reload
the driver when in "AMD_PSTATE_ACTIVE" mode and reject all writes when
in passive or guided mode, or if the driver is not loaded, since only
active mode operates on EPP.

Fixes: e30ca6dd5345 ("cpufreq/amd-pstate: Add dynamic energy performance preference")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 453084c67327f..5e2d28c73294d 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1707,6 +1707,8 @@ static int amd_pstate_change_driver_mode(int mode)
 {
 	int ret;
 
+	lockdep_assert_held(&amd_pstate_driver_lock);
+
 	ret = amd_pstate_unregister_driver(0);
 	if (ret)
 		return ret;
@@ -1821,6 +1823,13 @@ static ssize_t dynamic_epp_store(struct device *a, struct device_attribute *b,
 	if (ret)
 		return ret;
 
+	guard(mutex)(&amd_pstate_driver_lock);
+
+	if (cppc_state != AMD_PSTATE_ACTIVE) {
+		pr_debug("dynamic_epp can only be toggled in active mode\n");
+		return -EINVAL;
+	}
+
 	if (dynamic_epp == enabled)
 		return -EINVAL;
 
-- 
2.34.1

[PATCH 2/6] cpufreq/amd-pstate: Return -ENOMEM on failure to allocate profile_name

[K Prateek Nayak]

Failure to allocate profile name will return -EINVAL from
platform_profile_register() while in fact, it is a failure to allocate
memory for the profile_name string.

Return -ENOMEM when kasprintf() fails to allocate profile_name string.

Fixes: e30ca6dd5345 ("cpufreq/amd-pstate: Add dynamic energy performance preference")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 5e2d28c73294d..72514be2f30fb 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1291,6 +1291,8 @@ static int amd_pstate_set_dynamic_epp(struct cpufreq_policy *policy)
 		return ret;
 
 	cpudata->profile_name = kasprintf(GFP_KERNEL, "amd-pstate-epp-cpu%d", cpudata->cpu);
+	if (!cpudata->profile_name)
+		return -ENOMEM;
 
 	cpudata->ppdev = platform_profile_register(get_cpu_device(policy->cpu),
 						   cpudata->profile_name,
-- 
2.34.1

[PATCH 3/6] cpufreq/amd-pstate: Allow writes to dynamic_epp when state isn't modified

[K Prateek Nayak]

Writing the current "dynamic_epp" state to sysfs fails with -EINVAL even
though the desired result was achieved. Allow writes to "dynamic_epp"
that does not modify the state.

Fixes: e30ca6dd5345 ("cpufreq/amd-pstate: Add dynamic energy performance preference")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 72514be2f30fb..462ddad7bc79e 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1832,8 +1832,9 @@ static ssize_t dynamic_epp_store(struct device *a, struct device_attribute *b,
 		return -EINVAL;
 	}
 
+	/* Nothing to do */
 	if (dynamic_epp == enabled)
-		return -EINVAL;
+		return count;
 
 	/* reinitialize with desired dynamic EPP value */
 	dynamic_epp = enabled;
-- 
2.34.1

[PATCH 4/6] cpufreq/amd-pstate: Reorder notifier unregistration and floor perf reset

[K Prateek Nayak]

An active power supply notifier can race with amd_pstate_epp_cpu_exit()
trying to reset the floor perf and can overwrite the floor perf set in
MSR_AMD_CPPC_REQ.

Unregister the notifier before setting the floor perf to prevent the
rare race.

Fixes: e30ca6dd5345 ("cpufreq/amd-pstate: Add dynamic energy performance preference")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 462ddad7bc79e..175925762a93c 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1982,12 +1982,13 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
 	if (cpudata) {
 		union perf_cached perf = READ_ONCE(cpudata->perf);
 
+		if (cpudata->dynamic_epp)
+			amd_pstate_clear_dynamic_epp(policy);
+
 		/* Reset CPPC_REQ MSR to the BIOS value */
 		amd_pstate_update_perf(policy, perf.bios_min_perf, 0U, 0U, 0U, false);
 		amd_pstate_set_floor_perf(policy, cpudata->bios_floor_perf);
 
-		if (cpudata->dynamic_epp)
-			amd_pstate_clear_dynamic_epp(policy);
 		kfree(cpudata);
 		policy->driver_data = NULL;
 	}
-- 
2.34.1

[PATCH 5/6] cpufreq/amd-pstate: Use "epp_default_dc" as default when dynamic_epp is disabled

[K Prateek Nayak]

If "dynamic_epp" is disabled, the driver initialization and the default
EPP selection from sysfs currently sets the EPP based on the power
supply state of the system at that time but there is no power supply
callbacks registered to toggle it when the power supply state changes.

This can lead to faster battery drain on platforms that start off while
being plugged to the wall but later move to battery power since the EPP
stays at AMD_CPPC_EPP_PERFORMANCE.

Use "epp_default_dc" as the default EPP selection when dynamic_epp is
disabled, restoring older behavior. On servers, this defaults to
AMD_CPPC_EPP_PERFORMANCE and on other platforms, it defaults to
AMD_CPPC_EPP_BALANCE_PERFORMANCE.

Fixes: e30ca6dd5345 ("cpufreq/amd-pstate: Add dynamic energy performance preference")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 175925762a93c..9eb9c3f4e809a 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1429,7 +1429,7 @@ ssize_t store_energy_performance_preference(struct cpufreq_policy *policy,
 		if (ret)
 			epp = epp_values[ret];
 		else
-			epp = amd_pstate_get_balanced_epp(policy);
+			epp = cpudata->epp_default_dc;
 	}
 
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE) {
@@ -1954,7 +1954,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	if (dynamic_epp)
 		ret = amd_pstate_set_dynamic_epp(policy);
 	else
-		ret = amd_pstate_set_epp(policy, amd_pstate_get_balanced_epp(policy));
+		ret = amd_pstate_set_epp(policy, cpudata->epp_default_dc);
 	if (ret)
 		goto free_cpudata1;
 
-- 
2.34.1

[PATCH 6/6] cpufreq/amd-pstate-ut: Drop policy reference before driver switch

[K Prateek Nayak]

Recent changes to the EPP unit test tries to perform a driver switch
with a cpufreq_policy reference held when the driver is loaded into
anything but the active mode which leads to a circular dependency and
the unit test hanging indefinitely.

Drop the reference before driver switch and grab it back once the driver
mode is stabilized for the test.

The EPP writes are only possible with CPUFREQ_POLICY_POWERSAVE policy.
Temporarily switch the cpudata->policy (while holding the write end of
the policy->rwsem) to CPUFREQ_POLICY_POWERSAVE and restore the original
policy once tests are done. To ensure the final EPP is correct in case
the driver started with CPUFREQ_POLICY_PERFORMANCE, EPP performance is
tested last.

The __free() based cleanup for cpufreq_policy is lost in the process.

Reported-by: Kalpana Shetty <kalpana.shetty@amd.com>
Fixes: 7e173bc310d2b ("cpufreq/amd-pstate-ut: Add a unit test for raw EPP")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 drivers/cpufreq/amd-pstate-ut.c | 36 ++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate-ut.c b/drivers/cpufreq/amd-pstate-ut.c
index aa8a464fab47a..13a23dac477d1 100644
--- a/drivers/cpufreq/amd-pstate-ut.c
+++ b/drivers/cpufreq/amd-pstate-ut.c
@@ -274,20 +274,21 @@ static int amd_pstate_set_mode(enum amd_pstate_mode mode)
 
 static int amd_pstate_ut_epp(u32 index)
 {
-	struct cpufreq_policy *policy __free(put_cpufreq_policy) = NULL;
-	char *buf __free(cleanup_page) = NULL;
 	static const char * const epp_strings[] = {
-		"performance",
-		"balance_performance",
-		"balance_power",
 		"power",
+		"balance_power",
+		"balance_performance",
+		"performance",
 	};
-	struct amd_cpudata *cpudata;
+	char *buf __free(cleanup_page) = NULL;
+	struct cpufreq_policy *policy = NULL;
 	enum amd_pstate_mode orig_mode;
+	struct amd_cpudata *cpudata;
+	unsigned long orig_policy;
 	bool orig_dynamic_epp;
 	int ret, cpu = 0;
-	int i;
 	u16 epp;
+	int i;
 
 	policy = cpufreq_cpu_get(cpu);
 	if (!policy)
@@ -297,6 +298,10 @@ static int amd_pstate_ut_epp(u32 index)
 	orig_mode = amd_pstate_get_status();
 	orig_dynamic_epp = cpudata->dynamic_epp;
 
+	/* Drop reference before potential driver change. */
+	cpufreq_cpu_put(policy);
+	policy = NULL;
+
 	/* disable dynamic EPP before running test */
 	if (cpudata->dynamic_epp) {
 		pr_debug("Dynamic EPP is enabled, disabling it\n");
@@ -311,6 +316,17 @@ static int amd_pstate_ut_epp(u32 index)
 	if (ret)
 		goto out;
 
+	policy = cpufreq_cpu_get(cpu);
+	if (!policy) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	down_write(&policy->rwsem);
+	cpudata = policy->driver_data;
+	orig_policy = cpudata->policy;
+	cpudata->policy = CPUFREQ_POLICY_POWERSAVE;
+
 	for (epp = 0; epp <= U8_MAX; epp++) {
 		u8 val;
 
@@ -358,6 +374,12 @@ static int amd_pstate_ut_epp(u32 index)
 	ret = 0;
 
 out:
+	if (policy) {
+		cpudata->policy = orig_policy;
+		up_write(&policy->rwsem);
+		cpufreq_cpu_put(policy);
+	}
+
 	if (orig_dynamic_epp) {
 		int ret2;
 
-- 
2.34.1

Re: [PATCH 0/6] cpufreq/amd-pstate: Dynamic EPP fixes

[Mario Limonciello]

On 5/8/26 00:17, K Prateek Nayak wrote:
> Few rare race conditions were found when testing the Dynamic EPP feature
> on EPYC platforms:
> 
> o Toggling dynamic EPP is essentially a driver switch into the same mode
>    and can race with an admin-initiated driver switch which can cause the
>    system to hang.
> 
> o If a profile change / power supply status change races with the driver
>    unloading, it can overwrite the Floor Perf restored in the CPPC_REQ
>    register that was meant to be left untouched until the next driver
>    load.
> 
> o When dynamic mode is disabled, the default EPP hint is set based on
>    the power supply mode at the time of driver load which can cause the
>    battery to drain faster when starting off from AC supply and then
>    switching to battery.
> 
> When fixing and testing the above, a couple of incorrect error codes
> were noted which is also fixed in the series along with the unit test.
> 
> Series is based on mainline at v7.1-rc2 release tag. Thanks to Mario for
> initial review. Feedback is greatly appreciated.
> ---
> K Prateek Nayak (6):
>    cpufreq/amd-pstate: Grab "amd_pstate_driver_lock" when toggling
>      dynamic_epp
>    cpufreq/amd-pstate: Return -ENOMEM on failure to allocate profile_name
>    cpufreq/amd-pstate: Allow writes to dynamic_epp when state isn't
>      modified
>    cpufreq/amd-pstate: Reorder notifier unregistration and floor perf
>      reset
>    cpufreq/amd-pstate: Use "epp_default_dc" as default when dynamic_epp
>      is disabled
>    cpufreq/amd-pstate-ut: Drop policy reference before driver switch
> 
>   drivers/cpufreq/amd-pstate-ut.c | 36 ++++++++++++++++++++++++++-------
>   drivers/cpufreq/amd-pstate.c    | 23 ++++++++++++++++-----
>   2 files changed, 47 insertions(+), 12 deletions(-)
> 
> 
> base-commit: 7fd2df204f342fc17d1a0bfcd474b24232fb0f32

Much appreciated.  I've staged these in amd-pstate-fixes and will send 
out the PR next week after more testing.