Re: [PATCH 1/3] x86/cpufreq: Clean up powernow registration

[Andrew Cooper <amc96@xxxxxxxx>]

On 15/11/2021 09:27, Roger Pau Monné wrote:
> On Fri, Nov 12, 2021 at 06:28:16PM +0000, Andrew Cooper wrote:
>> powernow_register_driver() is currently written with a K&R type definition;
>> I'm surprised that compilers don't object to a mismatch with its declaration,
>> which is written in an ANSI-C compatible way.
>>
>> Furthermore, its sole caller is cpufreq_driver_init() which is a pre-smp
>> initcall.  There are no other online CPUs, and even if there were, checking
>> the BSP's CPUID data $N times is pointless.  Simplify registration to only
>> look at the BSP.
> I guess an extra safety would be to add some check for the cpuid bit
> in the AP boot path if the cpufreq driver is enabled.

We already have a number of cases where we expect the system to be
reasonably homogeneous, microcode most notably.

Given that we don't currently check this, I don't think it is worth
changing things.
>> While at it, drop obviously unused includes.  Also rewrite the expression in
>> cpufreq_driver_init() for clarity.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> ---
>>  xen/arch/x86/acpi/cpufreq/cpufreq.c  | 20 +++++++++++++-------
>>  xen/arch/x86/acpi/cpufreq/powernow.c | 28 ++++++----------------------
>>  2 files changed, 19 insertions(+), 29 deletions(-)
>>
>> diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c 
>> b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> index f1f3c6923fb3..2251c87f9e42 100644
>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> @@ -640,13 +640,19 @@ static int __init cpufreq_driver_init(void)
>>  {
>>      int ret = 0;
>>  
>> -    if ((cpufreq_controller == FREQCTL_xen) &&
>> -        (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL))
>> -        ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>> -    else if ((cpufreq_controller == FREQCTL_xen) &&
>> -        (boot_cpu_data.x86_vendor &
>> -         (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
>> -        ret = powernow_register_driver();
>> +    if ( cpufreq_controller == FREQCTL_xen )
>> +    {
>> +        switch ( boot_cpu_data.x86_vendor )
>> +        {
>> +        case X86_VENDOR_INTEL:
>> +            ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>> +            break;
>> +
>> +        case X86_VENDOR_AMD | X86_VENDOR_HYGON:
> This should be two separate case statements.
>
> With this fixed:
>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.  I'd actually already found and fixed that bug - clearly I sent
out an old version of the patch.

~Andrew