linux-kernel - Re: [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant amd_pstate_set

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: <f208714a-ed23-4f7e-a93d-6b6dad193f28@gmail.com>
Date: Sun, 27 Oct 2024 12:41:29 +0100
From: Klara Modin <klarasmodin@...il.com>
To: Mario Limonciello <superm1@...il.com>,
 Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>, gautham.shenoy@....com,
 mario.limonciello@....com, perry.yuan@....com, rafael@...nel.org,
 viresh.kumar@...aro.org
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, klara@...m.eu
Subject: Re: [PATCH 4/4] cpufreq/amd-pstate: Remove the redundant
 amd_pstate_set_driver() call

On 2024-10-27 05:27, Mario Limonciello wrote:
> 
> 
> On 10/26/24 06:58, Klara Modin wrote:
>> Hi,
>>
>> On 2024-10-17 12:05, Dhananjay Ugwekar wrote:
>>> amd_pstate_set_driver() is called twice, once in amd_pstate_init() 
>>> and once
>>> as part of amd_pstate_register_driver(). Move around code and eliminate
>>> the redundancy.
>>>
>>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
>>> ---
>>>   drivers/cpufreq/amd-pstate.c | 12 ++++--------
>>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>>> index 13ee5cac901d..6f6d961879cc 100644
>>> --- a/drivers/cpufreq/amd-pstate.c
>>> +++ b/drivers/cpufreq/amd-pstate.c
>>> @@ -1848,9 +1848,11 @@ static int __init amd_pstate_init(void)
>>>           return -ENODEV;
>>>       }
>>> -    ret = amd_pstate_set_driver(cppc_state);
>>> -    if (ret)
>>> +    ret = amd_pstate_register_driver(cppc_state);
>>> +    if (ret) {
>>> +        pr_err("failed to register with return %d\n", ret);
>>>           return ret;
>>> +    }
>>>       /* capability check */
>>>       if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>>> @@ -1870,12 +1872,6 @@ static int __init amd_pstate_init(void)
>>>               return ret;
>>>       }
>>> -    ret = amd_pstate_register_driver(cppc_state);
>>> -    if (ret) {
>>> -        pr_err("failed to register with return %d\n", ret);
>>> -        return ret;
>>> -    }
>>> -
>>>       dev_root = bus_get_dev_root(&cpu_subsys);
>>>       if (dev_root) {
>>>           ret = sysfs_create_group(&dev_root->kobj, 
>>> &amd_pstate_global_attr_group);
>>
>>
>> This seems to break boot on my Zen 2 desktop (my Zen 4 laptop is fine, 
>> however). I don't see any messages on the console and nothing shows up 
>> in the pstore either.
>>
>> I'll attach the kernel log from a normal boot (with the patch 
>> reverted) in case it helps.
>>
>> Please let me know if there's anything else you need.
>>
>> Regards,
>> Klara Modin
> 
> Hi,
> 
> Thanks for reporting.  I think this might be a regression on shared 
> memory designs specifically because static calls weren't updated yet.
> 
> Can you try this below diff?
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 206725219d8c9..2f0e29b05963d 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1857,12 +1857,6 @@ static int __init amd_pstate_init(void)
>                  return -ENODEV;
>          }
> 
> -       ret = amd_pstate_register_driver(cppc_state);
> -       if (ret) {
> -               pr_err("failed to register with return %d\n", ret);
> -               return ret;
> -       }
> -
>          /* capability check */
>          if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>                  pr_debug("AMD CPPC MSR based functionality is 
> supported\n");
> @@ -1881,6 +1875,12 @@ static int __init amd_pstate_init(void)
>                          return ret;
>          }
> 
> +       ret = amd_pstate_register_driver(cppc_state);
> +       if (ret) {
> +               pr_err("failed to register with return %d\n", ret);
> +               return ret;
> +       }
> +
>          dev_root = bus_get_dev_root(&cpu_subsys);
>          if (dev_root) {
>                  ret = sysfs_create_group(&dev_root->kobj, 
> &amd_pstate_global_attr_group);
> 

Moving the it afterwards does fix the issue, thanks!

Tested-by: Klara Modin <klarasmodin@...il.com>