Re: [Xen-devel] [PATCH 2/2] cpufreq: fix turbo mode state reporting

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Sorry for late reply, just notice this thread. Some comments inline below.

Jan Beulich wrote:
>>>> On 19.06.13 at 22:14, Jacob Shin <jacob.shin@xxxxxxx> wrote:
>> Currently we report back 0 or 1, which is broken since xenpm expects
>> CPUFREQ_TURBO_DISABLED, CPUFREQ_TURBO_UNSUPPORTED, or
>> CPUFREQ_TURBO_ENABLED. Report back actual policy->turbo value
>> instead. 
> 
> I think that it's xenpm that's wrong here - the three constants above
> aren't exposed in public headers, and hence aren't part of the ABI.
> Furthermore the structure member name that the result of this
> function gets stored into is "turbo_enabled", which also suggests a
> boolean value to me. I.e. if we want to change the value set that
> the hypervisor may return here, the field should also get renamed,
> and the value set exposed.
> 

IMHO this patch itself is OK, just need minor update at xenpm and add the 3 
constants to public headers.

Tristate (cap, enable/disable) exposes more info to user than bool 
(enable/disable), and the original semantics of the ABI is tristate (though bug 
at Xen cpufreq side). As for 'turbo_enabled', rename it as 'turbo' is OK.

Thanks,
Jinsong

>> --- a/xen/drivers/cpufreq/utility.c
>> +++ b/xen/drivers/cpufreq/utility.c
>> @@ -428,7 +428,10 @@ int cpufreq_get_turbo_status(int cpuid)
>>      struct cpufreq_policy *policy;
>> 
>>      policy = per_cpu(cpufreq_cpu_policy, cpuid);
>> -    return policy && policy->turbo;
>> +    if (!policy)
>> +        return CPUFREQ_TURBO_UNSUPPORTED;
>> +
>> +    return policy->turbo;
> 
> Nevertheless this would need adjustment even for the boolean
> value case:
> 
> -    return policy && policy->turbo;
> +    return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;
> 
> Jan


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