| Message ID | 20240826211358.2694603-3-superm1@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Adjustments for preferred core detection | expand |
Hello Mario, On Mon, Aug 26, 2024 at 04:13:52PM -0500, Mario Limonciello wrote: > From: Mario Limonciello <mario.limonciello@amd.com> > > The function name is ambiguous because it returns an intermediate value > for calculating maximum frequency rather than the CPPC 'Highest Perf' > register. > > Rename the function to clarify its use and allow the function to return > errors. Adjust the consumer in acpi-cpufreq to catch errors. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> [..snip..] > --- a/arch/x86/kernel/acpi/cppc.c > +++ b/arch/x86/kernel/acpi/cppc.c > @@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void) > return; > } > > - highest_perf = amd_get_highest_perf(); > + rc = amd_get_boost_ratio_numerator(0, &highest_perf); The variable is still named highest_perf, here! I suppose that will change in a subsequent patch? > + if (rc) > + pr_debug("Could not retrieve highest performance\n"); I understand that amd_get_boost_ratio_numerator() always returns a 0 in this patch and thus rc == 0, which means we never enter this "if" condition. However, when rc is non-zero, shouldn't this function return after printing the debug message? -- Thanks and Regards gautham. > nominal_perf = perf_caps.nominal_perf; > > - if (!highest_perf || !nominal_perf) { > - pr_debug("Could not retrieve highest or nominal performance\n"); > + if (!nominal_perf) { > + pr_debug("Could not retrieve nominal performance\n"); > return; > } > > @@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void) > mutex_unlock(&freq_invariance_lock); > } > > -u32 amd_get_highest_perf(void) > +/** > + * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation > + * @cpu: CPU to get numerator for. > + * @numerator: Output variable for numerator. > + * > + * Determine the numerator to use for calculating the boost ratio on > + * a CPU. On systems that support preferred cores, this will be a hardcoded > + * value. On other systems this will the highest performance register value. > + * > + * Return: 0 for success, negative error code otherwise. > + */ > +int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) > { > struct cpuinfo_x86 *c = &boot_cpu_data; > > if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > - (c->x86_model >= 0x70 && c->x86_model < 0x80))) > - return 166; > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) { > + *numerator = 166; > + return 0; > + } > > if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > - (c->x86_model >= 0x40 && c->x86_model < 0x70))) > - return 166; > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) { > + *numerator = 166; > + return 0; > + } > + *numerator = 255; > > - return 255; > + return 0; > } > -EXPORT_SYMBOL_GPL(amd_get_highest_perf); > +EXPORT_SYMBOL_GPL(amd_get_boost_ratio_numerator); > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index a8ca625a98b89..0f04feb6cafaf 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu) > return 0; > } > > - if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > - highest_perf = amd_get_highest_perf(); > - else > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) { > + ret = amd_get_boost_ratio_numerator(cpu, &highest_perf); > + if (ret) { > + pr_debug("CPU%d: Unable to get boost ratio numerator (%d)\n", > + cpu, ret); > + return 0; > + } > + } else { > highest_perf = perf_caps.highest_perf; > + } > > nominal_perf = perf_caps.nominal_perf; > > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h > index 930b6afba6f4d..f25a881cd46dd 100644 > --- a/include/acpi/cppc_acpi.h > +++ b/include/acpi/cppc_acpi.h > @@ -136,6 +136,12 @@ struct cppc_cpudata { > cpumask_var_t shared_cpu_map; > }; > > +#ifdef CONFIG_CPU_SUP_AMD > +extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator); > +#else /* !CONFIG_CPU_SUP_AMD */ > +static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; } > +#endif /* !CONFIG_CPU_SUP_AMD */ > + > #ifdef CONFIG_ACPI_CPPC_LIB > extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf); > extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf); > -- > 2.43.0 >
On 8/27/2024 09:42, Gautham R. Shenoy wrote: > Hello Mario, > > On Mon, Aug 26, 2024 at 04:13:52PM -0500, Mario Limonciello wrote: >> From: Mario Limonciello <mario.limonciello@amd.com> >> >> The function name is ambiguous because it returns an intermediate value >> for calculating maximum frequency rather than the CPPC 'Highest Perf' >> register. >> >> Rename the function to clarify its use and allow the function to return >> errors. Adjust the consumer in acpi-cpufreq to catch errors. >> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > [..snip..] > >> --- a/arch/x86/kernel/acpi/cppc.c >> +++ b/arch/x86/kernel/acpi/cppc.c >> @@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void) >> return; >> } >> >> - highest_perf = amd_get_highest_perf(); >> + rc = amd_get_boost_ratio_numerator(0, &highest_perf); > > The variable is still named highest_perf, here! I suppose that will > change in a subsequent patch? > > > >> + if (rc) >> + pr_debug("Could not retrieve highest performance\n"); > > I understand that amd_get_boost_ratio_numerator() always returns a 0 > in this patch and thus rc == 0, which means we never enter this "if" > condition. > > However, when rc is non-zero, shouldn't this function return after > printing the debug message? Both good points. Will fix for v2. > > -- > Thanks and Regards > gautham. > > > > >> nominal_perf = perf_caps.nominal_perf; >> >> - if (!highest_perf || !nominal_perf) { >> - pr_debug("Could not retrieve highest or nominal performance\n"); >> + if (!nominal_perf) { >> + pr_debug("Could not retrieve nominal performance\n"); >> return; >> } >> >> @@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void) >> mutex_unlock(&freq_invariance_lock); >> } >> >> -u32 amd_get_highest_perf(void) >> +/** >> + * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation >> + * @cpu: CPU to get numerator for. >> + * @numerator: Output variable for numerator. >> + * >> + * Determine the numerator to use for calculating the boost ratio on >> + * a CPU. On systems that support preferred cores, this will be a hardcoded >> + * value. On other systems this will the highest performance register value. >> + * >> + * Return: 0 for success, negative error code otherwise. >> + */ >> +int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) >> { >> struct cpuinfo_x86 *c = &boot_cpu_data; >> >> if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || >> - (c->x86_model >= 0x70 && c->x86_model < 0x80))) >> - return 166; >> + (c->x86_model >= 0x70 && c->x86_model < 0x80))) { >> + *numerator = 166; >> + return 0; >> + } >> >> if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || >> - (c->x86_model >= 0x40 && c->x86_model < 0x70))) >> - return 166; >> + (c->x86_model >= 0x40 && c->x86_model < 0x70))) { >> + *numerator = 166; >> + return 0; >> + } >> + *numerator = 255; >> >> - return 255; >> + return 0; >> } >> -EXPORT_SYMBOL_GPL(amd_get_highest_perf); >> +EXPORT_SYMBOL_GPL(amd_get_boost_ratio_numerator); >> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c >> index a8ca625a98b89..0f04feb6cafaf 100644 >> --- a/drivers/cpufreq/acpi-cpufreq.c >> +++ b/drivers/cpufreq/acpi-cpufreq.c >> @@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu) >> return 0; >> } >> >> - if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) >> - highest_perf = amd_get_highest_perf(); >> - else >> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) { >> + ret = amd_get_boost_ratio_numerator(cpu, &highest_perf); >> + if (ret) { >> + pr_debug("CPU%d: Unable to get boost ratio numerator (%d)\n", >> + cpu, ret); >> + return 0; >> + } >> + } else { >> highest_perf = perf_caps.highest_perf; >> + } >> >> nominal_perf = perf_caps.nominal_perf; >> >> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h >> index 930b6afba6f4d..f25a881cd46dd 100644 >> --- a/include/acpi/cppc_acpi.h >> +++ b/include/acpi/cppc_acpi.h >> @@ -136,6 +136,12 @@ struct cppc_cpudata { >> cpumask_var_t shared_cpu_map; >> }; >> >> +#ifdef CONFIG_CPU_SUP_AMD >> +extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator); >> +#else /* !CONFIG_CPU_SUP_AMD */ >> +static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; } >> +#endif /* !CONFIG_CPU_SUP_AMD */ >> + >> #ifdef CONFIG_ACPI_CPPC_LIB >> extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf); >> extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf); >> -- >> 2.43.0 >>
Hi Mario, kernel test robot noticed the following build errors: [auto build test ERROR on rafael-pm/linux-next] [also build test ERROR on rafael-pm/bleeding-edge tip/x86/core tip/master linus/master v6.11-rc5 next-20240827] [cannot apply to tip/auto-latest] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/x86-amd-Move-amd_get_highest_perf-from-amd-c-to-cppc-c/20240827-051648 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240826211358.2694603-3-superm1%40kernel.org patch subject: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator() config: x86_64-randconfig-074-20240828 (https://download.01.org/0day-ci/archive/20240828/202408281637.ssIOSO7H-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408281637.ssIOSO7H-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408281637.ssIOSO7H-lkp@intel.com/ All errors (new ones prefixed by >>): >> arch/x86/kernel/acpi/cppc.c:133:5: error: redefinition of 'amd_get_boost_ratio_numerator' 133 | int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from arch/x86/kernel/acpi/cppc.c:7: include/acpi/cppc_acpi.h:142:19: note: previous definition of 'amd_get_boost_ratio_numerator' with type 'int(unsigned int, u64 *)' {aka 'int(unsigned int, long long unsigned int *)'} 142 | static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; } | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/amd_get_boost_ratio_numerator +133 arch/x86/kernel/acpi/cppc.c 121 122 /** 123 * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation 124 * @cpu: CPU to get numerator for. 125 * @numerator: Output variable for numerator. 126 * 127 * Determine the numerator to use for calculating the boost ratio on 128 * a CPU. On systems that support preferred cores, this will be a hardcoded 129 * value. On other systems this will the highest performance register value. 130 * 131 * Return: 0 for success, negative error code otherwise. 132 */ > 133 int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index a75a07f4931fd..775acbdea1a96 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -691,8 +691,6 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu) } #ifdef CONFIG_CPU_SUP_AMD -extern u32 amd_get_highest_perf(void); - /* * Issue a DIV 0/1 insn to clear any division data from previous DIV * operations. @@ -705,7 +703,6 @@ static __always_inline void amd_clear_divider(void) extern void amd_check_microcode(void); #else -static inline u32 amd_get_highest_perf(void) { return 0; } static inline void amd_clear_divider(void) { } static inline void amd_check_microcode(void) { } #endif diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c index 7ec8f2ce859c8..1d631ac5ec328 100644 --- a/arch/x86/kernel/acpi/cppc.c +++ b/arch/x86/kernel/acpi/cppc.c @@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void) return; } - highest_perf = amd_get_highest_perf(); + rc = amd_get_boost_ratio_numerator(0, &highest_perf); + if (rc) + pr_debug("Could not retrieve highest performance\n"); nominal_perf = perf_caps.nominal_perf; - if (!highest_perf || !nominal_perf) { - pr_debug("Could not retrieve highest or nominal performance\n"); + if (!nominal_perf) { + pr_debug("Could not retrieve nominal performance\n"); return; } @@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void) mutex_unlock(&freq_invariance_lock); } -u32 amd_get_highest_perf(void) +/** + * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation + * @cpu: CPU to get numerator for. + * @numerator: Output variable for numerator. + * + * Determine the numerator to use for calculating the boost ratio on + * a CPU. On systems that support preferred cores, this will be a hardcoded + * value. On other systems this will the highest performance register value. + * + * Return: 0 for success, negative error code otherwise. + */ +int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { struct cpuinfo_x86 *c = &boot_cpu_data; if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || - (c->x86_model >= 0x70 && c->x86_model < 0x80))) - return 166; + (c->x86_model >= 0x70 && c->x86_model < 0x80))) { + *numerator = 166; + return 0; + } if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || - (c->x86_model >= 0x40 && c->x86_model < 0x70))) - return 166; + (c->x86_model >= 0x40 && c->x86_model < 0x70))) { + *numerator = 166; + return 0; + } + *numerator = 255; - return 255; + return 0; } -EXPORT_SYMBOL_GPL(amd_get_highest_perf); +EXPORT_SYMBOL_GPL(amd_get_boost_ratio_numerator); diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index a8ca625a98b89..0f04feb6cafaf 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu) return 0; } - if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) - highest_perf = amd_get_highest_perf(); - else + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) { + ret = amd_get_boost_ratio_numerator(cpu, &highest_perf); + if (ret) { + pr_debug("CPU%d: Unable to get boost ratio numerator (%d)\n", + cpu, ret); + return 0; + } + } else { highest_perf = perf_caps.highest_perf; + } nominal_perf = perf_caps.nominal_perf; diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h index 930b6afba6f4d..f25a881cd46dd 100644 --- a/include/acpi/cppc_acpi.h +++ b/include/acpi/cppc_acpi.h @@ -136,6 +136,12 @@ struct cppc_cpudata { cpumask_var_t shared_cpu_map; }; +#ifdef CONFIG_CPU_SUP_AMD +extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator); +#else /* !CONFIG_CPU_SUP_AMD */ +static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; } +#endif /* !CONFIG_CPU_SUP_AMD */ + #ifdef CONFIG_ACPI_CPPC_LIB extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf); extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf);