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

["Wang, Wei W" <wei.w.wang@xxxxxxxxx>]

On 23/06/2015 16:11, Jan Beulich wrote:
> >>> On 23.06.15 at 10:01, <wei.w.wang@xxxxxxxxx> wrote:
> > On 23/06/2015 15:31, Jan Beulich wrote:
> >> >>> 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:
> >> >> > -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.
> >
> > This is not arbitrary - "->target()" is dedicated to the Governor
> > framework, and "->setpolicy" is dedicated to the internal governor
> > implementation. The Linux kernel also takes advantage of this method.
> > I think we don't need to add another new functionally equivalent flag to do
> so.
> > Shall we keep using it?
> 
> If this distinction is being made clear by comments accompanying the
> definitions of the target and setpolicy fields, I'm fine with keeping it that 
> way.
> Without making this explicit it would continue to look arbitrary (and prone to
> break, should another driver elect to also implement the setpolicy hook [for
> whatever purpose]).

OK, I will add some explanation to the related commit message.

Best,
Wei

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