linux-kernel - Re: [Regression] 6.11.0-rc1: AMD CPU boot with error when CPPC feature disabled by BIOS

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2c2569aa.47a.191062d0d40.Coremail.00107082@163.com>
Date: Wed, 31 Jul 2024 08:25:01 +0800 (CST)
From: "David Wang" <00107082@....com>
To: "Xiaojian Du" <xiaojidu@....com>
Cc: perry.yuan@....com, Alexander.Deucher@....com, Li.Meng@....com, 
	Mario.Limonciello@....com, Xiaojian.Du@....com, Xinmei.Huang@....com, 
	gautham.shenoy@....com, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, rafael.j.wysocki@...el.com, 
	viresh.kumar@...aro.org
Subject: Re: [Regression] 6.11.0-rc1: AMD CPU boot with error when CPPC
 feature disabled by BIOS

Hi Du, 

Thanks for the quick response

At 2024-07-31 01:43:47, "Xiaojian Du" <xiaojidu@....com> wrote:
>Hi  David,
>
>CPPC feature is enabled by default in BIOS for new ZENX arch CPU, and 
>amd-pstate driver is enabled in new linux kernel.
>
>For your system ,why CPPC is disabled, is it for debug or some special case?

 I do not have any special purpose, it is just the default BIOS setup for my old system.

>
>If you want to use legacy acpi cpu driver, you can disable amd-pstate 
>driver module in your linux kernel config file and compile a new kernel.
>
>> Those warning message was introduced by commit:
>>   bff7d13c190ad98cf4f877189b022c75df4cb383 ("cpufreq: amd-pstate: add debug message while CPPC is supported and disabled by SBIOS)
>> , which make sense.
>>
>> Those error message was introduced by commit:
>>   8f8b42c1fcc939a73b547b172a9ffcb65ef4bf47 ("cpufreq: amd-pstate: optimize the initial frequency values verification")
>> , when CPPC is disabled by BIOS, this error message does not make sense, and the error return-code would abort the driver registeration,
>> but this behavior could be handled earlier when detecting CPPC feature.
>>
>> I feel following changes would make a clean fix: do not register amd_pstate driver when CPPC disabled by BIOS.
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 68c616b572f2..b06faea58fd4 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -1837,8 +1837,6 @@ static bool amd_cppc_supported(void)
>>           * If the CPPC feature is disabled in the BIOS for processors that support MSR-based CPPC,
>>           * the AMD Pstate driver may not function correctly.
>>           * Check the CPPC flag and display a warning message if the platform supports CPPC.
>> -        * Note: below checking code will not abort the driver registeration process because of
>> -        * the code is added for debugging purposes.
>
>As you see, it is for debug purpose, in some corner case, if CPPC 
>feature is disabled, this debug info will help to guide user to 
>*re-enable* it.
>
>Target system, including CPU+baseboard+BIOS, is supposed to enable and 
>use CPPC feature for better Performance per Watt.

I agree that the warning guide makes total sense,  but  the  *errors* does not, why bother trying and report error when the feature is disabled by BIOS.


>
>>           */
>>          if (!cpu_feature_enabled(X86_FEATURE_CPPC)) {
>>                  if (cpu_feature_enabled(X86_FEATURE_ZEN1) || cpu_feature_enabled(X86_FEATURE_ZEN2)) {
>> @@ -1856,6 +1854,7 @@ static bool amd_cppc_supported(void)
>>          if (warn)
>>                  pr_warn_once("The CPPC feature is supported but currently disabled by the BIOS.\n"
>>                                          "Please enable it if your BIOS has the CPPC option.\n");
>> +               return false;
>Maybe need a pair of curly brace after this "if(warn)" for your change.
>>          return true;
>>   }
>>
>>
>>
>> Thanks
>> David
>>