| Message ID | 20220810192944.102135-4-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
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@gmail.com> > --- > 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. > --- 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. > +{ > + intel_feature_detect((struct cpufreq_policy *)info); Why the cast? Jan
On Mon, Aug 15, 2022 at 10:34 AM Jan Beulich <jbeulich@suse.com> 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@gmail.com> > > --- > > 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
On 15.08.2022 16:58, Jason Andryuk wrote: > On Mon, Aug 15, 2022 at 10:34 AM Jan Beulich <jbeulich@suse.com> 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@gmail.com> >>> --- >>> 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. But you don't need the type to be available just for a function declaration. For such a purpose a forward decl of the struct is sufficient. Anyway - not an issue here. Jan
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c index ded0150b3b..b5eb869227 100644 --- 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) +{ + intel_feature_detect((struct cpufreq_policy *)info); +} + static unsigned int check_freqs(const cpumask_t *mask, unsigned int freq, struct acpi_cpufreq_data *data) { diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h index a06aa92f62..0f334d2a43 100644 --- a/xen/include/acpi/cpufreq/cpufreq.h +++ b/xen/include/acpi/cpufreq/cpufreq.h @@ -243,4 +243,6 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq); void cpufreq_dbs_timer_suspend(void); void cpufreq_dbs_timer_resume(void); +void intel_feature_detect(struct cpufreq_policy *policy); + #endif /* __XEN_CPUFREQ_PM_H__ */
Export feature_detect as intel_feature_detect so it can be re-used by HWP. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- 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 *. --- xen/arch/x86/acpi/cpufreq/cpufreq.c | 8 ++++++-- xen/include/acpi/cpufreq/cpufreq.h | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-)