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

Dhananjay Ugwekar
2025-02-24T09:21:26Z

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);