Re: [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

["Jan Beulich" <JBeulich@suse.com>]

>>> On 09.09.15 at 10:11, <wei.w.wang@intel.com> wrote:
> On 24/07/2015 22:16,  Jan Beulich wrote:
>>>> On 25.06.15 at 13:17, <wei.w.wang@intel.com> wrote:
>> --- a/xen/drivers/acpi/pmstat.c
>> +++ b/xen/drivers/acpi/pmstat.c
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -315,8 +315,18 @@ struct xen_get_cpufreq_para {
>>      uint32_t scaling_cur_freq;
>>  
>>      char scaling_governor[CPUFREQ_NAME_LEN];
>> -    uint32_t scaling_max_freq;
>> -    uint32_t scaling_min_freq;
>> +
>> +    union {
>> +        uint32_t freq;
>> +        uint32_t pct;
>> +    } scaling_max;
>> +
>> +    union {
>> +        uint32_t freq;
>> +        uint32_t pct;
>> +    } scaling_min;
> 
>>scaling_min and scaling_max should really be of the same type, so that 
> someone wanting to introduce helper functions 
>>or pointers to them can hand both interchangeably.
> 
>>Also I'm starting to get tired of repeating that it is still unclear how a 
> consumer of the structure will know which of the 
>>two fields of the unions are applicable.
> 
> Probably we don't need a union here. I plan to simply change them to 
> uint32_t scaling_max_perf;
> uint32_t scaling_max_perf;
> 
> Then it's up to the driver to put what kind of value to it. It's like we 
> simply provide a drinking vessel, and it depends on the user to put water or 
> milk into it. In our case, the intel_pstate driver assigns a percentage vale 
> to it (in the "uint32_t" type), and the legacy driver assigns the absolute 
> value to it (in the "uint32_t" type, too).

I don't see how this will solve the problem of the consumer not
knowing what kind of value it has to deal with.

Jan