Re: [PATCH v3 17/18] cpufreq/amd-pstate: Rework CPPC enabling

[Gautham R. Shenoy]

On Mon, Feb 17, 2025 at 04:07:06PM -0600, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> 
> The CPPC enable register is configured as "write once".  That is
> any future writes don't actually do anything.
> 
> Because of this, all the cleanup paths that currently exist for
> CPPC disable are non-effective.
> 
> Rework CPPC enable to only enable after all the CAP registers have
> been read to avoid enabling CPPC on CPUs with invalid _CPC or
> unpopulated MSRs.
> 
> As the register is write once, remove all cleanup paths as well.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v3:
>  * Fixup for suspend/resume issue
> ---
[..snip..]

>  
> -static int shmem_cppc_enable(bool enable)
> +static int shmem_cppc_enable(struct cpufreq_policy *policy)
>  {
> -	int cpu, ret = 0;
> +	struct amd_cpudata *cpudata = policy->driver_data;
>  	struct cppc_perf_ctrls perf_ctrls;
> +	int ret;
>  
> -	if (enable == cppc_enabled)
> -		return 0;
> +	ret = cppc_set_enable(cpudata->cpu, 1);
> +	if (ret)
> +		return ret;
>  
> -	for_each_present_cpu(cpu) {
> -		ret = cppc_set_enable(cpu, enable);
> +	/* Enable autonomous mode for EPP */
> +	if (cppc_state == AMD_PSTATE_ACTIVE) {
> +		/* Set desired perf as zero to allow EPP firmware control */
> +		perf_ctrls.desired_perf = 0;
> +		ret = cppc_set_perf(cpudata->cpu, &perf_ctrls);
>  		if (ret)
>  			return ret;

We don't need the if condition here. There is nothing following this
inside the if block and the function return "ret" soon after coming
out of this if block.


> -
> -		/* Enable autonomous mode for EPP */
> -		if (cppc_state == AMD_PSTATE_ACTIVE) {
> -			/* Set desired perf as zero to allow EPP firmware control */
> -			perf_ctrls.desired_perf = 0;
> -			ret = cppc_set_perf(cpu, &perf_ctrls);
> -			if (ret)
> -				return ret;
> -		}
>  	}
>  
> -	cppc_enabled = enable;
>  	return ret;
>  }
>  
>  DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
>  
> -static inline int amd_pstate_cppc_enable(bool enable)
[..snip..]

>  
> -static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
> +static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  	union perf_cached perf = READ_ONCE(cpudata->perf);
>  	int ret;
>  
> -	ret = amd_pstate_cppc_enable(true);
> -	if (ret)
> -		pr_err("failed to enable amd pstate during resume, return %d\n", ret);
> -
> -
> -	return amd_pstate_epp_update_limit(policy);
> -}
> +	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
>  
> -static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
> -{
> -	struct amd_cpudata *cpudata = policy->driver_data;
> -	int ret;
> +	ret = amd_pstate_cppc_enable(policy);
> +	if (ret)
> +		return ret;
>  
> -	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
>  
> -	ret = amd_pstate_epp_reenable(policy);
> +	ret = amd_pstate_update_perf(policy, 0, 0, perf.highest_perf, cpudata->epp_cached, false);

Previously, when a CPU came online, the callpath would be
amd_pstate_epp_cpu_online(policy)
--> amd_pstate_epp_reenable(policy)
     --> amd_pstate_epp_update_limit(policy)
          --> amd_pstate_epp_update_limit(policy)

which reevaluates the min_perf_limit and max_perf_limit based on
policy->min and policy->max and then calls

      amd_pstate_update_perf(policy, min_limit_perf, 0, max_limit_perf, epp, false)

With this patch, we call

      amd_pstate_update_perf(policy, 0, 0, perf.highest_perf, cpudata->epp_cached, false);

which would set CPPC.min_perf to 0.

I guess this should be ok since cpufreq_online() would eventually call
amd_pstate_verify() and amd_pstate_epp_set_policy() which should
re-initialize the the min_limit_perf and max_limit_perf. Though I
haven't verified if the behaviour changes with this patch when the CPU
is offlined and brought back online.

Rest of the patch looks good to me.

-- 
Thanks and Regards
gautham.