diff mbox series

[2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()

Message ID 20240826211358.2694603-3-superm1@kernel.org (mailing list archive)
State Superseded, archived
Delegated to: Mario Limonciello
Headers show
Series Adjustments for preferred core detection | expand

Commit Message

Mario Limonciello Aug. 26, 2024, 9:13 p.m. UTC 
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>
---
 arch/x86/include/asm/processor.h |  3 ---
 arch/x86/kernel/acpi/cppc.c      | 38 +++++++++++++++++++++++---------
 drivers/cpufreq/acpi-cpufreq.c   | 12 +++++++---
 include/acpi/cppc_acpi.h         |  6 +++++
 4 files changed, 43 insertions(+), 16 deletions(-)

Comments

Gautham R. Shenoy Aug. 27, 2024, 2:42 p.m. UTC | #1 
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
>
Mario Limonciello Aug. 27, 2024, 6:18 p.m. UTC | #2 
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
>>
kernel test robot Aug. 28, 2024, 9:09 a.m. UTC | #3 
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 mbox series

Patch

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);