[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 9 Sep 2022 14:00:53 -0500
From: "Limonciello, Mario" <mario.limonciello@....com>
To: Perry Yuan <Perry.Yuan@....com>, rafael.j.wysocki@...el.com,
ray.huang@....com, viresh.kumar@...aro.org
Cc: Deepak.Sharma@....com, Nathan.Fontenot@....com,
Alexander.Deucher@....com, Jinzhou.Su@....com,
Shimmer.Huang@....com, Xiaojian.Du@....com, Li.Meng@....com,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] cpufreq: amd-pstate: implement suspend and resume
callbacks
On 9/9/2022 11:45, Perry Yuan wrote:
> add suspend and resume support for the AMD processors by amd_pstate_epp
> driver instance.
>
> When the CPPC is suspended, EPP driver will set EPP profile to 'power'
> profile and set max/min perf to lowest perf value.
> When resume happens, it will restore the MSR registers with
> previous cached value.
>
> Signed-off-by: Perry Yuan <Perry.Yuan@....com>
> ---
> drivers/cpufreq/amd-pstate.c | 39 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index e63fed39f90c..749083d28b05 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1476,6 +1476,43 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
> return amd_pstate_cpu_offline(policy);
> }
>
> +static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
> + int ret;
I don't see an explicit guard in here to only run this code for epp
mode. If you want it to be running both for EPP and non-EPP then you
should update the commit message. If you only want it running for EPP,
I would think you need a:
if (!epp_enabled)
return 0;
> +
> + pr_debug("AMD CPU Core %d suspending\n", cpudata->cpu);
This debug statement seems needlessly noisy to me (even for dyndbg).
Unless they're for debugging synchronization problems, I would think
that you can get a similar result using ftrace for function names.
> +
> + cpudata->suspended = true;
> +
> + /* disable CPPC in lowlevel firmware */
> + ret = amd_pstate_enable(false);
> + if (ret)
> + pr_err("failed to disable amd pstate during suspend, return %d\n", ret);
amd-pstate uses pr_fmt. You don't need to mention so much in your error
message. Something like this would suffice:
pr_err("failed to suspend: %d\n, ret);
> +
> + return 0;
Shouldn't you be returning ret here?
> +}
> +
> +static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
> +{
> + struct amd_cpudata *cpudata = all_cpu_data[policy->cpu];
> + > + pr_debug("AMD CPU Core %d resuming\n", cpudata->cpu);
Ditto on above comments.
> +
> + if (cpudata->suspended && epp_enabled) {
If you end up adopting the suggestion above for checking epp_enabled in
suspend, I don't belivee you also need to check in resume. Your check
for cpudata->suspended will make sure this only runs if you did
something for suspend.
> + mutex_lock(&amd_pstate_limits_lock);
> +
> + /* enable amd pstate from suspend state*/
> + amd_pstate_epp_reenable(cpudata);
> +
> + mutex_unlock(&amd_pstate_limits_lock);
> + }
> +
> + cpudata->suspended = false;
You can move this into the if statement.
> +
> + return 0;
> +}
> +
> static void amd_pstate_verify_cpu_policy(struct amd_cpudata *cpudata,
> struct cpufreq_policy_data *policy)
> {
> @@ -1512,6 +1549,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
> .update_limits = amd_pstate_epp_update_limits,
> .offline = amd_pstate_epp_cpu_offline,
> .online = amd_pstate_epp_cpu_online,
> + .suspend = amd_pstate_epp_suspend,
> + .resume = amd_pstate_epp_resume,
> .name = "amd_pstate_epp",
> .attr = amd_pstate_epp_attr,
> };
Powered by blists - more mailing lists