[6/7] cpufreq:amd-pstate: add suspend and resume callback for passive mode
Commit Message
From: Perry Yuan <Perry.Yuan@amd.com>
Add suspend and resume support for `passive` mode driver which can save
the previous CPU Pstate values and restore them while resuming, on some
old platforms, the highest perf needs to be restored from driver side,
otherwise the highest frequency could be changed during suspend.
Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
---
drivers/cpufreq/amd-pstate.c | 48 ++++++++++++++++++++++++++++++++----
1 file changed, 43 insertions(+), 5 deletions(-)
Comments
On 1/26/2024 02:08, Perry Yuan wrote:
> From: Perry Yuan <Perry.Yuan@amd.com>
>
> Add suspend and resume support for `passive` mode driver which can save
> the previous CPU Pstate values and restore them while resuming, on some
> old platforms, the highest perf needs to be restored from driver side,
> otherwise the highest frequency could be changed during suspend.
So this sounds like a BIOS bug, right? Do you know how far back this
problem exists? Should it be a quirked behavior to only run on the
broken platforms so we don't need to run the callback on modern ones
without it?
>
> Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> ---
> drivers/cpufreq/amd-pstate.c | 48 ++++++++++++++++++++++++++++++++----
> 1 file changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 5cbbc2999d9a..bba7640d46e0 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -785,23 +785,61 @@ static int amd_pstate_cpu_exit(struct cpufreq_policy *policy)
>
> static int amd_pstate_cpu_resume(struct cpufreq_policy *policy)
> {
> + struct cppc_perf_ctrls perf_ctrls;
> + struct amd_cpudata *cpudata = policy->driver_data;
> + u64 value, max_perf;
> int ret;
>
> - ret = amd_pstate_enable(true);
> - if (ret)
> - pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
> + if (cpudata->suspended) {
> + mutex_lock(&amd_pstate_driver_lock);
>
> - return ret;
> + ret = amd_pstate_enable(true);
> + if (ret) {
> + pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
> + mutex_unlock(&amd_pstate_driver_lock);
> + return ret;
> + }
This /looks/ like an unintended logic change to me. Previously
amd_pstate_enable(true) would be called in all modes, but now it will
only be called in passive mode.
Is that right?
> +
> + value = READ_ONCE(cpudata->cppc_req_cached);
> + max_perf = READ_ONCE(cpudata->highest_perf);
> +
> + if (boot_cpu_has(X86_FEATURE_CPPC)) {
> + wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> + } else {
> + perf_ctrls.max_perf = max_perf;
> + cppc_set_perf(cpudata->cpu, &perf_ctrls);
> + }
> +
> + cpudata->suspended = false;
> + mutex_unlock(&amd_pstate_driver_lock);
> + }
> +
> + return 0;
> }
>
> static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
> {
> + struct amd_cpudata *cpudata = policy->driver_data;
> int ret;
>
> + /* avoid suspending when EPP is not enabled */
The logic seems right, but shouldn't the comment be:
/* only run suspend callbacks for passive mode */
Because this stuff won't run in guided mode or disable mode either
> + if (cppc_state != AMD_PSTATE_PASSIVE)
> + return 0;
> +
> + mutex_lock(&amd_pstate_driver_lock);
> +
> + /* set this flag to avoid calling core offline function
> + * when system is suspending, use this flag to skip offline function
> + * called
> + */
> + cpudata->suspended = true;
> +
> ret = amd_pstate_enable(false);
> if (ret)
> pr_err("failed to disable amd-pstate during suspend, return %d\n", ret);
>
> + mutex_unlock(&amd_pstate_driver_lock);
> +
> return ret;
> }
>
> @@ -1460,7 +1498,7 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
> if (cppc_state != AMD_PSTATE_ACTIVE)
> return 0;
>
> - /* set this flag to avoid setting core offline*/
> + /* set this flag to avoid setting core offline */
> cpudata->suspended = true;
>
> /* disable CPPC in lowlevel firmware */
@@ -785,23 +785,61 @@ static int amd_pstate_cpu_exit(struct cpufreq_policy *policy)
static int amd_pstate_cpu_resume(struct cpufreq_policy *policy)
{
+ struct cppc_perf_ctrls perf_ctrls;
+ struct amd_cpudata *cpudata = policy->driver_data;
+ u64 value, max_perf;
int ret;
- ret = amd_pstate_enable(true);
- if (ret)
- pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
+ if (cpudata->suspended) {
+ mutex_lock(&amd_pstate_driver_lock);
- return ret;
+ ret = amd_pstate_enable(true);
+ if (ret) {
+ pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
+ mutex_unlock(&amd_pstate_driver_lock);
+ return ret;
+ }
+
+ value = READ_ONCE(cpudata->cppc_req_cached);
+ max_perf = READ_ONCE(cpudata->highest_perf);
+
+ if (boot_cpu_has(X86_FEATURE_CPPC)) {
+ wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
+ } else {
+ perf_ctrls.max_perf = max_perf;
+ cppc_set_perf(cpudata->cpu, &perf_ctrls);
+ }
+
+ cpudata->suspended = false;
+ mutex_unlock(&amd_pstate_driver_lock);
+ }
+
+ return 0;
}
static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
{
+ struct amd_cpudata *cpudata = policy->driver_data;
int ret;
+ /* avoid suspending when EPP is not enabled */
+ if (cppc_state != AMD_PSTATE_PASSIVE)
+ return 0;
+
+ mutex_lock(&amd_pstate_driver_lock);
+
+ /* set this flag to avoid calling core offline function
+ * when system is suspending, use this flag to skip offline function
+ * called
+ */
+ cpudata->suspended = true;
+
ret = amd_pstate_enable(false);
if (ret)
pr_err("failed to disable amd-pstate during suspend, return %d\n", ret);
+ mutex_unlock(&amd_pstate_driver_lock);
+
return ret;
}
@@ -1460,7 +1498,7 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
if (cppc_state != AMD_PSTATE_ACTIVE)
return 0;
- /* set this flag to avoid setting core offline*/
+ /* set this flag to avoid setting core offline */
cpudata->suspended = true;
/* disable CPPC in lowlevel firmware */