Re: [PATCH 7/7] cpufreq: amd-pstate: implement suspend and resume callbacks

[Limonciello, Mario]

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@xxxxxxx>
> ---
>   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,
>   };