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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d52622c3-e750-4147-a03d-fa19c397d347@amd.com>
Date: Tue, 25 Feb 2025 10:20:24 +0530
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
To: Mario Limonciello <superm1@...nel.org>,
 "Gautham R . Shenoy" <gautham.shenoy@....com>,
 Perry Yuan <perry.yuan@....com>
Cc: "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
 <linux-kernel@...r.kernel.org>,
 "open list:CPU FREQUENCY SCALING FRAMEWORK" <linux-pm@...r.kernel.org>,
 Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v4 17/19] cpufreq/amd-pstate: Rework CPPC enabling

On 2/25/2025 5:29 AM, Mario Limonciello wrote:
>>> +    /* 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()
> 
> Great point, agreed will drop it.
> 
>>
>>>       }
>>>   -    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)
[Snip]
>>>   @@ -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?
> 
> The reason I had in mind for it was this sequence:
> * Suspend
> * CPU goes offline
> * CPU goes online
> * Resume
> 
> But I don't think that's realistic even with parallel boot.  I will drop this.

Also I have one doubt, why do we need to keep track if the system is suspended ?

Won't the idle subsystem have safeguards to prevent CPU offline while the system 
is being suspended ? Haven't gone through that code, just checking if you have an 
idea about it.