Re: [Xen-devel] [PATCH v3 04/11] x86/intel_pstate: relocate the driver register function

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.06.15 at 10:27, <wei.w.wang@xxxxxxxxx> wrote:
> Register the CPU hotplug notifier when the driver is
> registered, and move the driver register function to
> the cpufreq.c.

The first half of the sentence fails to say why. And I suppose if you
explained that (to yourself) you'd figure that the change is wrong (or
at least altering behavior in a way that needs more explanation to
be verifiably correct): The calls to cpufreq_register_driver() sit in
__initcall handlers, yet what you replace is a presmp_initcall. I.e. all
APs being brought up at boot time won't get the callback invoked for
them anymore.

I suppose you tested your series on a system where the new driver
will kick in. You of course also need to test the case where this isn't
the case - everything needs to continue to function there.

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -630,12 +630,21 @@ static struct notifier_block cpu_nfb = {
>      .notifier_call = cpu_callback
>  };
>  
> -static int __init cpufreq_presmp_init(void)
> +int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>  {
>      void *cpu = (void *)(long)smp_processor_id();
>      cpu_callback(&cpu_nfb, CPU_ONLINE, cpu);
> +    if (!driver_data || !driver_data->init
> +        || !driver_data->verify || !driver_data->exit
> +        || (!driver_data->target == !driver_data->setpolicy))

Do you really want/need to enforce this policy (target set if and only
if setpolicy is not set) here? And if that's to uniformly hold, the two
could be put into a union...

Also - coding style.

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -171,32 +171,8 @@ struct cpufreq_driver {
>  
>  extern struct cpufreq_driver *cpufreq_driver;
>  
> -static __inline__ 
> -int cpufreq_register_driver(struct cpufreq_driver *driver_data)
> -{
> -    if (!driver_data         || 
> -        !driver_data->init   || 
> -        !driver_data->exit   || 
> -        !driver_data->verify || 
> -        !driver_data->target)
> -        return -EINVAL;
> -
> -    if (cpufreq_driver)
> -        return -EBUSY;
> -
> -    cpufreq_driver = driver_data;
> -    return 0;
> -}
> -
> -static __inline__ 
> -int cpufreq_unregister_driver(struct cpufreq_driver *driver)
> -{
> -    if (!cpufreq_driver || (driver != cpufreq_driver))
> -        return -EINVAL;
> -
> -    cpufreq_driver = NULL;
> -    return 0;
> -}
> +extern int cpufreq_register_driver(struct cpufreq_driver *driver_data);
> +extern int cpufreq_unregister_driver(struct cpufreq_driver *driver);

What's this declaration good for when there's no implementation?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel