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

[Julien Grall <julien.grall@citrix.com>]

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@intel.com>
>>> ---
>>>    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.

Regards,

-- 
Julien Grall