Re: [PATCH 6/7] cpufreq:amd-pstate: add suspend and resume callback for passive mode

[Mario Limonciello <mario.limonciello@amd.com>]

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 */