Re: [PATCH v2 03/13] cpufreq: Export intel_feature_detect

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, Aug 15, 2022 at 10:34 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 10.08.2022 21:29, Jason Andryuk wrote:
> > Export feature_detect as intel_feature_detect so it can be re-used by
> > HWP.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> > ---
> > v2
> > export intel_feature_detect with typed pointer
> > Move intel_feature_detect to acpi/cpufreq/cpufreq.h since the
> > declaration now contains struct cpufreq_policy *.
>
> I don't mind the new placement, but I don't follow the reasoning.

v1 added
void intel_feature_detect(void *info);
to acpi/cpufreq/processor_perf.h

v2 adds
void intel_feature_detect(struct cpufreq_policy *policy)
to acpi/cpufreq/cpufreq.h, which was selected to have the type available.

> > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> > @@ -340,9 +340,8 @@ static unsigned int cf_check 
> > get_cur_freq_on_cpu(unsigned int cpu)
> >      return extract_freq(get_cur_val(cpumask_of(cpu)), data);
> >  }
> >
> > -static void cf_check feature_detect(void *info)
> > +void intel_feature_detect(struct cpufreq_policy *policy)
> >  {
> > -    struct cpufreq_policy *policy = info;
> >      unsigned int eax;
> >
> >      eax = cpuid_eax(6);
> > @@ -354,6 +353,11 @@ static void cf_check feature_detect(void *info)
> >      }
> >  }
> >
> > +static void cf_check feature_detect(void *info)
>
> This function is invoked indirectly via on_selected_cpus() (hence
> the cf_check attribute) - I wonder how you get away without that for
> HWP. Or else why we need this as a wrapper here when then you'd have
> another similar wrapper elsewhere.

HWP calls hwp_init_msrs via on_selected_cpus, which then directly
calls intel_feature_detect().  ACPI needs the cf_check on
feature_detect, but intel_feature_detect is only called directly.

> > +{
> > +    intel_feature_detect((struct cpufreq_policy *)info);
>
> Why the cast?

I thought it was necessary to keep the compiler happy.  Double
checking, you are correct that it is not needed.

Thanks,
Jason