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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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

One rant below about getting rid of powernow_register_driver.

> +            ret = powernow_register_driver();
> +            break;
> +        }
> +    }
>  
>      return ret;
>  }
> diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c 
> b/xen/arch/x86/acpi/cpufreq/powernow.c
> index f620bebc7e91..80095dfd14b4 100644
> --- a/xen/arch/x86/acpi/cpufreq/powernow.c
> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c
> @@ -24,13 +24,9 @@
>  #include <xen/types.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>
> -#include <xen/delay.h>
>  #include <xen/cpumask.h>
> -#include <xen/timer.h>
>  #include <xen/xmalloc.h>
> -#include <asm/bug.h>
>  #include <asm/msr.h>
> -#include <asm/io.h>
>  #include <asm/processor.h>
>  #include <asm/cpufeature.h>
>  #include <acpi/acpi.h>
> @@ -353,25 +349,13 @@ static const struct cpufreq_driver __initconstrel 
> powernow_cpufreq_driver = {
>      .update = powernow_cpufreq_update
>  };
>  
> -unsigned int __init powernow_register_driver()
> +unsigned int __init powernow_register_driver(void)
>  {
> -    unsigned int i, ret = 0;
> +    if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
> +        return -ENODEV;
>  
> -    for_each_online_cpu(i) {
> -        struct cpuinfo_x86 *c = &cpu_data[i];
> -        if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
> -            ret = -ENODEV;
> -        else
> -        {
> -            u32 eax, ebx, ecx, edx;
> -            cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx);
> -            if ((edx & USE_HW_PSTATE) != USE_HW_PSTATE)
> -                ret = -ENODEV;
> -        }
> -        if (ret)
> -            return ret;
> -    }
> +    if ( !(cpuid_edx(CPUID_FREQ_VOLT_CAPABILITIES) & USE_HW_PSTATE) )
> +        return -ENODEV;

I wonder if we could move this check into cpufreq_driver_init and get
rid of powernow_register_driver.

Thanks, Roger.