Re: [PATCH v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2022 11:22, Roger Pau Monné wrote:
> On Thu, Apr 07, 2022 at 10:48:50AM +0200, Jan Beulich wrote:
>> On 07.04.2022 10:18, Roger Pau Monne wrote:
>>> The values set in the shared_type field of xen_processor_performance
>>> have so far relied on Xen and Linux having the same
>>> CPUFREQ_SHARED_TYPE_ defines, as those have never been part of the
>>> public interface.
>>>
>>> Formalize by adding the defines for the allowed values in the public
>>> header, while renaming them to use the XEN_CPUPERF_SHARED_TYPE_ prefix
>>> for clarity.
>>>
>>> Set the Xen internal defines for CPUFREQ_SHARED_TYPE_ using the newly
>>> introduced XEN_CPUPERF_SHARED_TYPE_ public defines in order to avoid
>>> unnecessary code churn.  While there also drop
>>> CPUFREQ_SHARED_TYPE_NONE as it's unused.
>>>
>>> Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> with one remark:
>>
>>> --- a/xen/include/acpi/cpufreq/cpufreq.h
>>> +++ b/xen/include/acpi/cpufreq/cpufreq.h
>>> @@ -78,10 +78,9 @@ DECLARE_PER_CPU(struct cpufreq_policy *, 
>>> cpufreq_cpu_policy);
>>>  extern int __cpufreq_set_policy(struct cpufreq_policy *data,
>>>                                  struct cpufreq_policy *policy);
>>>  
>>> -#define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
>>
>> I realize this is unused, but do we really want/need to drop this?
>> I think it is used implicitly right now by assuming the value would
>> be zero; this could do with making explicit, in which case we'd
>> need the #define.
> 
> I don't think Xen uses it explicitly, all checks of shared_type are
> always against a specific CPUFREQ_SHARED_TYPE_{HW,ALL,ANY}.

Well, I said "implicitly"; if there was an explicit reference, you'd
have run into a build failure. But I did check now - all comparisons of
->shared_type are against explicit CPUFREQ_SHARED_TYPE_*. So I guess
dropping the value is fine.

Jan