[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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_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.


>> --- 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 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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.