Re: [PATCH v4 17/19] cpufreq/amd-pstate: Rework CPPC enabling

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

On 2/20/2025 2:33 AM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> 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@amd.com>
> ---
> v4:
>  * Remove unnecessary amd_pstate_update_perf() call during online
>  * Remove unnecessary if (ret) ret.
>  * Drop amd_pstate_cpu_resume()
>  * Drop unnecessary derefs
> v3:
>  * Fixup for suspend/resume issue
> ---
>  drivers/cpufreq/amd-pstate.c | 187 +++++++++++------------------------
>  1 file changed, 55 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index fa9c581c68a39..f152636cecbeb 100644
[Snip]
> -static inline int msr_cppc_enable(bool enable)
> +static int shmem_cppc_enable(struct cpufreq_policy *policy)
>  {
> -	int ret, cpu;
> -	unsigned long logical_proc_id_mask = 0;
> -
> -       /*
> -        * MSR_AMD_CPPC_ENABLE is write-once, once set it cannot be cleared.
> -        */
> -	if (!enable)
> -		return 0;
> -
> -	if (enable == cppc_enabled)
> -		return 0;
> -
> -	for_each_present_cpu(cpu) {
> -		unsigned long logical_id = topology_logical_package_id(cpu);
> -
> -		if (test_bit(logical_id, &logical_proc_id_mask))
> -			continue;
> -
> -		set_bit(logical_id, &logical_proc_id_mask);
> -
> -		ret = wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE,
> -				enable);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	cppc_enabled = enable;
> -	return 0;
> -}
> -
> -static int shmem_cppc_enable(bool enable)
> -{
> -	int cpu, ret = 0;
>  	struct cppc_perf_ctrls perf_ctrls;
> +	int ret;
>  
> -	if (enable == cppc_enabled)
> -		return 0;
> -
> -	for_each_present_cpu(cpu) {
> -		ret = cppc_set_enable(cpu, enable);
> -		if (ret)
> -			return ret;
> +	ret = cppc_set_enable(policy->cpu, 1);
> +	if (ret)
> +		return ret;
>  
> -		/* 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;
> -		}
> +	/* 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(policy->cpu, &perf_ctrls);

I'm thinking do we need this "setting of desired_perf" as a part of shmem_cppc_enable, 
one thing is we're not doing it in the "msr_" counterpart
also, I guess this would be taken care as part of amd_pstate_epp_set_policy()->amd_pstate_epp_update_limit()->amd_pstate_update_perf()

>  	}
>  
> -	cppc_enabled = enable;
>  	return ret;
>  }
>  
>  DEFINE_STATIC_CALL(amd_pstate_cppc_enable, msr_cppc_enable);
>  
> -static inline int amd_pstate_cppc_enable(bool enable)
> +static inline int amd_pstate_cppc_enable(struct cpufreq_policy *policy)
>  {
> -	return static_call(amd_pstate_cppc_enable)(enable);
> +	return static_call(amd_pstate_cppc_enable)(policy);
>  }
>  
>  static int msr_init_perf(struct amd_cpudata *cpudata)
> @@ -1115,28 +1060,6 @@ static void amd_pstate_cpu_exit(struct cpufreq_policy *policy)
>  	kfree(cpudata);
>  }
>  
> -static int amd_pstate_cpu_resume(struct cpufreq_policy *policy)
> -{
> -	int ret;
> -
> -	ret = amd_pstate_cppc_enable(true);
> -	if (ret)
> -		pr_err("failed to enable amd-pstate during resume, return %d\n", ret);
> -
> -	return ret;
> -}
> -
> -static int amd_pstate_cpu_suspend(struct cpufreq_policy *policy)
> -{
> -	int ret;
> -
> -	ret = amd_pstate_cppc_enable(false);
> -	if (ret)
> -		pr_err("failed to disable amd-pstate during suspend, return %d\n", ret);
> -
> -	return ret;
> -}
> -
>  /* Sysfs attributes */
>  
>  /*
> @@ -1228,8 +1151,10 @@ static ssize_t show_energy_performance_available_preferences(
>  static ssize_t store_energy_performance_preference(
>  		struct cpufreq_policy *policy, const char *buf, size_t count)
>  {
> +	struct amd_cpudata *cpudata = policy->driver_data;
>  	char str_preference[21];
>  	ssize_t ret;
> +	u8 epp;
>  
>  	ret = sscanf(buf, "%20s", str_preference);
>  	if (ret != 1)
> @@ -1239,7 +1164,29 @@ static ssize_t store_energy_performance_preference(
>  	if (ret < 0)
>  		return -EINVAL;
>  
> -	ret = amd_pstate_set_energy_pref_index(policy, ret);
> +	if (!ret)
> +		epp = cpudata->epp_default;
> +	else
> +		epp = epp_values[ret];
> +
> +	if (epp > 0 && policy->policy == CPUFREQ_POLICY_PERFORMANCE) {
> +		pr_debug("EPP cannot be set under performance policy\n");
> +		return -EBUSY;
> +	}
> +
> +	if (trace_amd_pstate_epp_perf_enabled()) {
> +		union perf_cached perf = cpudata->perf;
> +
> +		trace_amd_pstate_epp_perf(cpudata->cpu, perf.highest_perf,
> +					  epp,
> +					  FIELD_GET(AMD_CPPC_MIN_PERF_MASK, cpudata->cppc_req_cached),
> +					  FIELD_GET(AMD_CPPC_MAX_PERF_MASK, cpudata->cppc_req_cached),
> +					  policy->boost_enabled,
> +					  FIELD_GET(AMD_CPPC_EPP_PERF_MASK,
> +						    cpudata->cppc_req_cached) != epp);

We are doing the tracing in amd_pstate_set_epp() as well right?, Isnt this one redundant?

> +	}
> +
> +	ret = amd_pstate_set_epp(policy, epp);
>  
>  	return ret ? ret : count;
>  }
> @@ -1272,7 +1219,6 @@ static ssize_t show_energy_performance_preference(
>  
>  static void amd_pstate_driver_cleanup(void)
>  {
> -	amd_pstate_cppc_enable(false);
>  	cppc_state = AMD_PSTATE_DISABLE;
>  	current_pstate_driver = NULL;
>  }
> @@ -1306,14 +1252,6 @@ static int amd_pstate_register_driver(int mode)
>  
>  	cppc_state = mode;
>  
> -	ret = amd_pstate_cppc_enable(true);
> -	if (ret) {
> -		pr_err("failed to enable cppc during amd-pstate driver registration, return %d\n",
> -		       ret);
> -		amd_pstate_driver_cleanup();
> -		return ret;
> -	}
> -
>  	/* at least one CPU supports CPB */
>  	current_pstate_driver->boost_enabled = cpu_feature_enabled(X86_FEATURE_CPB);
>  
> @@ -1553,11 +1491,15 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	policy->cpuinfo.max_freq = policy->max = perf_to_freq(perf,
>  							      cpudata->nominal_freq,
>  							      perf.highest_perf);
> +	policy->driver_data = cpudata;
> +
> +	ret = amd_pstate_cppc_enable(policy);

I think we missed cppc_enable in "amd_pstate_cpu_init". Confirmed this by booting with "amd_pstate=passive" 

Also, one weird behavior I saw while testing this part, if we boot with "amd_pstate=passive" 
initially, the MSR_AMD_CPPC_ENABLE register is 0. But after I run the amd-pstate-ut (which fails 
the check_amd_pstate_enabled() test the first time) the MSR_AMD_CPPC_ENABLE gets set to 1. But I 
didnt see any code in amd-pstate-ut that sets it. We can ignore this quirk for now, just 
mentioned to see if you have any idea about this.

> +	if (ret)
> +		goto free_cpudata1;
>  
>  	/* It will be updated by governor */
>  	policy->cur = policy->cpuinfo.min_freq;
>  
> -	policy->driver_data = cpudata;
>  
>  	policy->boost_enabled = READ_ONCE(cpudata->boost_supported);
>  
> @@ -1649,31 +1591,21 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>  	return 0;
>  }
>  
> -static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
> -{
> -	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);
> -}
> -
>  static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  	int ret;
>  
> -	pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
> +	pr_debug("AMD CPU Core %d going online\n", policy->cpu);
>  
> -	ret = amd_pstate_epp_reenable(policy);
> +	ret = amd_pstate_cppc_enable(policy);
>  	if (ret)
>  		return ret;
> +
>  	cpudata->suspended = false;

Do we need this here?, shouldn't only resume() have this statement?

>  
>  	return 0;
> +
>  }
>  
>  static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
> @@ -1691,11 +1623,6 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>  static int amd_pstate_epp_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_ACTIVE)
> -		return 0;
>  
>  	/* invalidate to ensure it's rewritten during resume */
>  	cpudata->cppc_req_cached = 0;
> @@ -1703,11 +1630,6 @@ static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
>  	/* set this flag to avoid setting core offline*/
>  	cpudata->suspended = true;
>  
> -	/* disable CPPC in lowlevel firmware */
> -	ret = amd_pstate_cppc_enable(false);
> -	if (ret)
> -		pr_err("failed to suspend, return %d\n", ret);
> -
>  	return 0;
>  }
>  
> @@ -1716,8 +1638,12 @@ static int amd_pstate_epp_resume(struct cpufreq_policy *policy)
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  
>  	if (cpudata->suspended) {
> +		int ret;
> +
>  		/* enable amd pstate from suspend state*/
> -		amd_pstate_epp_reenable(policy);
> +		ret = amd_pstate_epp_update_limit(policy);
> +		if (ret)
> +			return ret;
>  
>  		cpudata->suspended = false;
>  	}
> @@ -1732,8 +1658,6 @@ static struct cpufreq_driver amd_pstate_driver = {
>  	.fast_switch    = amd_pstate_fast_switch,
>  	.init		= amd_pstate_cpu_init,
>  	.exit		= amd_pstate_cpu_exit,
> -	.suspend	= amd_pstate_cpu_suspend,
> -	.resume		= amd_pstate_cpu_resume,
>  	.set_boost	= amd_pstate_set_boost,
>  	.update_limits	= amd_pstate_update_limits,
>  	.name		= "amd-pstate",
> @@ -1748,8 +1672,8 @@ static struct cpufreq_driver amd_pstate_epp_driver = {
>  	.exit		= amd_pstate_epp_cpu_exit,
>  	.offline	= amd_pstate_epp_cpu_offline,
>  	.online		= amd_pstate_epp_cpu_online,
> -	.suspend	= amd_pstate_epp_suspend,
> -	.resume		= amd_pstate_epp_resume,
> +	.suspend        = amd_pstate_epp_suspend,
> +	.resume         = amd_pstate_epp_resume,

Spurious whitespace change?

>  	.update_limits	= amd_pstate_update_limits,
>  	.set_boost	= amd_pstate_set_boost,
>  	.name		= "amd-pstate-epp",
> @@ -1900,7 +1824,6 @@ static int __init amd_pstate_init(void)
>  
>  global_attr_free:
>  	cpufreq_unregister_driver(current_pstate_driver);
> -	amd_pstate_cppc_enable(false);
>  	return ret;
>  }
>  device_initcall(amd_pstate_init);