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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 07, 2022 at 11:35:20AM +0200, Jan Beulich wrote:
> 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.

Yes, that's what I've tried to explain, unsuccessfully it seems :), on
my reply.  I should have used 'explicit' instead of 'specific'.

Thanks, Roger.