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.coop...@citrix.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Wei Liu <w...@xen.org>
> ---
> 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....@citrix.com>
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.
