| Message ID | 20210503192810.36084-4-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 03.05.2021 21:28, 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> Acked-by: Jan Beulich <jbeulich@suse.com> albeit ... > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c > @@ -340,7 +340,7 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu) > return extract_freq(get_cur_val(cpumask_of(cpu)), data); > } > > -static void feature_detect(void *info) > +void intel_feature_detect(void *info) > { > struct cpufreq_policy *policy = info; ... because of this (requiring the hwp code to stay in sync with possible changes here, without the compiler being able to point out inconsistencies) I'm not overly happy with such a change. Yet I guess this isn't the first case we have in the code base. Jan
On Wed, May 26, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 03.05.2021 21:28, 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> > > Acked-by: Jan Beulich <jbeulich@suse.com> > albeit ... > > > --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c > > +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c > > @@ -340,7 +340,7 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu) > > return extract_freq(get_cur_val(cpumask_of(cpu)), data); > > } > > > > -static void feature_detect(void *info) > > +void intel_feature_detect(void *info) > > { > > struct cpufreq_policy *policy = info; > > ... because of this (requiring the hwp code to stay in sync with > possible changes here, without the compiler being able to point > out inconsistencies) I'm not overly happy with such a change. Yet > I guess this isn't the first case we have in the code base. For acpi-cpufreq, this is called by on_selected_cpus(), but hwp calls this directly. You could do something like: void intel_feature_detect(struct cpufreq_policy *policy) { /* current feature_detect() */ } static void feature_detect(void *info) struct cpufreq_policy *policy = info; intel_feature_detect(policy); } Would you prefer that? Regards, Jason
On 26.05.2021 16:44, Jason Andryuk wrote: > On Wed, May 26, 2021 at 9:27 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 03.05.2021 21:28, Jason Andryuk wrote: >>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c >>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c >>> @@ -340,7 +340,7 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu) >>> return extract_freq(get_cur_val(cpumask_of(cpu)), data); >>> } >>> >>> -static void feature_detect(void *info) >>> +void intel_feature_detect(void *info) >>> { >>> struct cpufreq_policy *policy = info; >> >> ... because of this (requiring the hwp code to stay in sync with >> possible changes here, without the compiler being able to point >> out inconsistencies) I'm not overly happy with such a change. Yet >> I guess this isn't the first case we have in the code base. > > For acpi-cpufreq, this is called by on_selected_cpus(), but hwp calls > this directly. You could do something like: > > void intel_feature_detect(struct cpufreq_policy *policy) > { > /* current feature_detect() */ > } > > static void feature_detect(void *info) > struct cpufreq_policy *policy = info; > > intel_feature_detect(policy); > } > > Would you prefer that? Would feel less fragile, yes. And no need for the intermediate "policy" variable, afaics. Jan
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c index 5eac2f7321..8aae9b534d 100644 --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c @@ -340,7 +340,7 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu) return extract_freq(get_cur_val(cpumask_of(cpu)), data); } -static void feature_detect(void *info) +void intel_feature_detect(void *info) { struct cpufreq_policy *policy = info; unsigned int eax; @@ -596,7 +596,7 @@ acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) /* Check for APERF/MPERF support in hardware * also check for boost support */ if (c->x86_vendor == X86_VENDOR_INTEL && c->cpuid_level >= 6) - on_selected_cpus(cpumask_of(cpu), feature_detect, policy, 1); + on_selected_cpus(cpumask_of(cpu), intel_feature_detect, policy, 1); /* * the first call to ->target() should result in us actually diff --git a/xen/include/acpi/cpufreq/processor_perf.h b/xen/include/acpi/cpufreq/processor_perf.h index d8a1ba68a6..e2c08f0e6d 100644 --- a/xen/include/acpi/cpufreq/processor_perf.h +++ b/xen/include/acpi/cpufreq/processor_perf.h @@ -7,6 +7,8 @@ #define XEN_PX_INIT 0x80000000 +void intel_feature_detect(void *info); + int powernow_cpufreq_init(void); unsigned int powernow_register_driver(void); unsigned int get_measured_perf(unsigned int cpu, unsigned int flag);
Export feature_detect as intel_feature_detect so it can be re-used by HWP. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- xen/arch/x86/acpi/cpufreq/cpufreq.c | 4 ++-- xen/include/acpi/cpufreq/processor_perf.h | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-)