>>> On 23.06.15 at 05:40, <wei.w.w...@intel.com> wrote:
> On 18/06/2015 22:30, Jan Beulich wrote:
>> >>> On 11.06.15 at 10:27, <wei.w.w...@intel.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
