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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.15 at 05:40, <wei.w.wang@xxxxxxxxx> wrote:
> On 18/06/2015 22:30, Jan Beulich wrote:
>> >>> 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.
> 
> The cpu_callback() is removed in the following patch (05/11) because it's 
> redundant.
> Functions in cpufreq_register_driver() are also called only once, because 
> other calls to cpufreq_register_driver() will just return due to the 
> unsuccessful condition check at the beginning of the function.

If this is the case today, then the removal should be a separate
patch early in the series (properly explaining the situation).

>> > --- 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...
> 
> driver_data->target() is used by a driver which relies on the old Governor 
> framework.
> driver_data->setpolicy() is used by a driver which implements its internal 
> governor.
> So, the driver either uses the old Governor framework or has its own private 
> internal governor.
> We shouldn't change to use union, because in many places, we distinguish the 
> two by checking if it's using "->target" or "->setpolicy".

The distinction between the two driver modes shouldn't be based on
arbitrary accessors they may or may not implement. There should be
a dedicated flag or alike.

Jan


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