Re: [Xen-devel] [PATCH v3 07/11] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline

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

On 12/06/2015 19:40, Julien Grall wrote:
> On 11/06/2015 22:01, Wang, Wei W wrote:
> > On 11/06/2015 22:06, Julien Grall wrote:
> >> On 11/06/2015 04:28, Wei Wang wrote:
> >>> cpufreq_cpu_policy is used in intel_pstate_set_pstate(), so we
> >>> change to NULL it after the call of cpufreq_driver->exit. Otherwise,
> >>> a calltrace will show up on your screen due to the reference of a
> >>> NULL pointer when you power down the system.
> >>>
> >>> Signed-off-by: Wei Wang <wei.w.wang@xxxxxxxxx>
> >>> ---
> >>>    xen/drivers/cpufreq/cpufreq.c | 6 +++---
> >>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/drivers/cpufreq/cpufreq.c
> >>> b/xen/drivers/cpufreq/cpufreq.c index 6003a8c..a8772e8 100644
> >>> --- a/xen/drivers/cpufreq/cpufreq.c
> >>> +++ b/xen/drivers/cpufreq/cpufreq.c
> >>> @@ -335,12 +335,11 @@ int cpufreq_del_cpu(unsigned int cpu)
> >>>
> >>>        /* for HW_ALL, stop gov for each core of the _PSD domain */
> >>>        /* for SW_ALL & SW_ANY, stop gov for the 1st core of the _PSD
> >>> domain
> >> */
> >>> -    if (hw_all || (cpumask_weight(cpufreq_dom->map) ==
> >>> -                   perf->domain_info.num_processors))
> >>> +    if (!policy->policy && (hw_all ||
> >>> + (cpumask_weight(cpufreq_dom->map)
> >> ==
> >>> +                   perf->domain_info.num_processors)))
> >>
> >> Based on your patch #6, the field policy contains value which is
> >> defined per- cpufreq driver (because you defined internal value). How
> >> can you be sure that a driver will never use 0 as a valid value?
> >
> > Hi Julien, what do you mean by "per-cpufreq driver"?
> >
> > We currently have two P-state drivers. This filed is currently only used by
> the intel_pstate driver, and the four usable values are:
> > #define CPUFREQ_POLICY_POWERSAVE           (1)
> > #define CPUFREQ_POLICY_PERFORMANCE      (2)
> > #define CPUFREQ_POLICY_USERSPACE              (3)
> > #define CPUFREQ_POLICY_ONDEMAND            (4)
> >
> > The intel_pstate won't use 0 as a valid value, and the default value is
> CPUFREQ_POLICY_ONDEMAND.  If it's 0, it basically means the old acpi-
> cpufreq driver is being used.
> 
> You seem to rely on nobody else with use the cpufreq framework... which is
> wrong. intel_pstate won't be the only possible cpufreq driver. Some ARM
> developper are working on adding a cpufreq for ARM power management.
> 
> You said that CPUFREQ_POLICY_* is specific to the intel driver. But use them
> in the common code. If any cpufreq driver can use the value, then make it
> common. Otherwise please move this code outside of the framework.

To me, we can deal with your concerns by
1) renaming the previously mentioned things to be intel_pstate specific;
2) moving them to a new header file.

Jan, please also ACK if you agree on these changes.

Best,
Wei


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