Re: [Xen-devel] [PATCH v2 7/9] x86/intel_pstate: add a booting param to select the driver to load

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

On 04/06/2015 09:18, Wei Wang wrote
> On 03/06/2015 19:51, Jan Beulich wrote
> > >>> On 03.06.15 at 10:07, <wei.w.wang@xxxxxxxxx> wrote:
> > > On 26/05/2015 22:02, Jan Beulich wrote
> > >> >>> On 13.05.16 at 09:51, <wei.w.wang@xxxxxxxxx> wrote:
> > >> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > >> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > >> > --- a/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> > >> > +++ b/xen/arch/x86/acpi/cpufreq/intel_pstate.c
> > >> > @@ -766,6 +766,8 @@ static struct cpufreq_driver
> > >> > intel_pstate_driver =
> > {
> > >> >      .name         = "intel_pstate",
> > >> >  };
> > >> >
> > >> > +int __initdata load_intel_pstate = 0;
> > >>
> > >> static bool_t
> > >
> > > I think we cannot use "static" here, since load_intel_pstate is also
> > > used in cpufreq.c to select which driver to load.
> >
> > Iirc I had also requested to deal with that (as it looks pretty hackish 
> > right
> now).
> 
> I'm not sure about this. Can you please elaborate it - why it looks hackish by
> doing so?
> What is your preferred way to do it? Thanks.
> 
> The following is your another comment on "load_intel_pstate":
> 
> >> @@ -650,9 +650,12 @@ 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_INTEL)) {
> > >+        if (load_intel_pstate)
> > >+            ret = intel_pstate_init();
> > >+        if (!load_intel_pstate)
> > >+            ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> 
> >I don't see why you need load_intel_pstate here: Simply call the
> >original function whenever
> >intel_pstate_init() returns an error.
> 
> I plan to change it to:
>       if (load_intel_pstate)
>               ret = intel_pstate_init();
>       if (ret)
>               ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> 
> This allows the case that the machine supports the intel_pstate driver but
> people just prefer to use the old driver for their own reasons.


Missed one thing, it should be like this:
        if (load_intel_pstate)
                ret = intel_pstate_init();
        if (ret || ! load_intel_pstate)
                ret = cpufreq_register_driver(&acpi_cpufreq_driver);
 
 Best,
 Wei

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