diff mbox series

[01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification

Message ID 0049ad44052b051cf57d1059bf71b7ce227a5f21.1715065568.git.perry.yuan@amd.com (mailing list archive)
State Superseded, archived
Headers show
Series AMD Pstate Driver Fixes and Improvements | expand

Commit Message

Yuan, Perry May 7, 2024, 7:15 a.m. UTC 
To enhance the debugging capability of the driver loading failure for
broken CPPC ACPI tables, we can optimize the expression by moving the
verification of `min_freq`, `nominal_freq`, and other dependency values
to the `amd_pstate_init_freq()` function where they are initialized.
If any of these values are incorrect, the `amd-pstate` driver will not be registered.

By ensuring that these values are correct before they are used, it will facilitate
the debugging process when encountering driver loading failures due to faulty CPPC
ACPI tables from BIOS

Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

Comments

Mario Limonciello May 7, 2024, 2:44 p.m. UTC | #1 
The title has a typo s/optimiza/optimize/

On 5/7/2024 02:15, Perry Yuan wrote:
> To enhance the debugging capability of the driver loading failure for
> broken CPPC ACPI tables, we can optimize the expression by moving the

Remove the "we" here.

> verification of `min_freq`, `nominal_freq`, and other dependency values
> to the `amd_pstate_init_freq()` function where they are initialized.
> If any of these values are incorrect, the `amd-pstate` driver will not be registered.
> 
> By ensuring that these values are correct before they are used, it will facilitate
> the debugging process when encountering driver loading failures due to faulty CPPC
> ACPI tables from BIOS
> 
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 35 ++++++++++++++++++-----------------
>   1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 2db095867d03..be7f2d3c86b6 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -873,6 +873,24 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
>   	WRITE_ONCE(cpudata->max_freq, max_freq);
>   
> +	/**
> +	 * Below values need to be initialized correctly, otherwise driver will be failed to load

"will fail to load"

> +	 * max_freq is calculated accoreding to (nominal_freq * highest_perf)/nominal_perf

according to

> +	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
> +	 * Check _CPC in ACPI table ojbects if any values are incorrect

objects

> +	 */
> +	if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {
> +		pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
> +			min_freq, max_freq, nominal_freq);
> +		return -EINVAL;
> +	}
> +
> +	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
> +		pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
> +			lowest_nonlinear_freq, min_freq, nominal_freq * 1000);
> +		return -EINVAL;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -911,15 +929,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>   	max_freq = READ_ONCE(cpudata->max_freq);
>   	nominal_freq = READ_ONCE(cpudata->nominal_freq);
>   
> -	if (min_freq <= 0 || max_freq <= 0 ||
> -	    nominal_freq <= 0 || min_freq > max_freq) {
> -		dev_err(dev,
> -			"min_freq(%d) or max_freq(%d) or nominal_freq (%d) value is incorrect, check _CPC in ACPI tables\n",
> -			min_freq, max_freq, nominal_freq);
> -		ret = -EINVAL;
> -		goto free_cpudata1;
> -	}
> -
>   	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
>   	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
>   
> @@ -1372,14 +1381,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>   	min_freq = READ_ONCE(cpudata->min_freq);
>   	max_freq = READ_ONCE(cpudata->max_freq);
>   	nominal_freq = READ_ONCE(cpudata->nominal_freq);
> -	if (min_freq <= 0 || max_freq <= 0 ||
> -	    nominal_freq <= 0 || min_freq > max_freq) {
> -		dev_err(dev,
> -			"min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect, check _CPC in ACPI tables\n",
> -			min_freq, max_freq, nominal_freq);
> -		ret = -EINVAL;
> -		goto free_cpudata1;
> -	}
>   
>   	policy->cpuinfo.min_freq = min_freq;
>   	policy->cpuinfo.max_freq = max_freq;
kernel test robot May 7, 2024, 9:02 p.m. UTC | #2 
Hi Perry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge next-20240507]
[cannot apply to tip/x86/core linus/master v6.9-rc7]
[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/Perry-Yuan/cpufreq-amd-pstate-optimiza-the-initial-frequency-values-verification/20240507-151930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/0049ad44052b051cf57d1059bf71b7ce227a5f21.1715065568.git.perry.yuan%40amd.com
patch subject: [PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification
config: x86_64-randconfig-102-20240507 (https://download.01.org/0day-ci/archive/20240508/202405080431.BPU6Yg9s-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080431.BPU6Yg9s-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/202405080431.BPU6Yg9s-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_cpu_init':
>> drivers/cpufreq/amd-pstate.c:899:33: warning: variable 'nominal_freq' set but not used [-Wunused-but-set-variable]
     899 |         int min_freq, max_freq, nominal_freq, ret;
         |                                 ^~~~~~~~~~~~
   drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_epp_cpu_init':
   drivers/cpufreq/amd-pstate.c:1350:33: warning: variable 'nominal_freq' set but not used [-Wunused-but-set-variable]
    1350 |         int min_freq, max_freq, nominal_freq, ret;
         |                                 ^~~~~~~~~~~~


vim +/nominal_freq +899 drivers/cpufreq/amd-pstate.c

5547c0ebfc2efd Perry Yuan        2024-04-25  896  
ec437d71db77a1 Huang Rui         2021-12-24  897  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
ec437d71db77a1 Huang Rui         2021-12-24  898  {
5c3fd1edaa8b4c Perry Yuan        2024-04-30 @899  	int min_freq, max_freq, nominal_freq, ret;
ec437d71db77a1 Huang Rui         2021-12-24  900  	struct device *dev;
ec437d71db77a1 Huang Rui         2021-12-24  901  	struct amd_cpudata *cpudata;
ec437d71db77a1 Huang Rui         2021-12-24  902  
919f4557696939 Wyes Karny        2022-11-17  903  	/*
919f4557696939 Wyes Karny        2022-11-17  904  	 * Resetting PERF_CTL_MSR will put the CPU in P0 frequency,
919f4557696939 Wyes Karny        2022-11-17  905  	 * which is ideal for initialization process.
919f4557696939 Wyes Karny        2022-11-17  906  	 */
919f4557696939 Wyes Karny        2022-11-17  907  	amd_perf_ctl_reset(policy->cpu);
ec437d71db77a1 Huang Rui         2021-12-24  908  	dev = get_cpu_device(policy->cpu);
ec437d71db77a1 Huang Rui         2021-12-24  909  	if (!dev)
ec437d71db77a1 Huang Rui         2021-12-24  910  		return -ENODEV;
ec437d71db77a1 Huang Rui         2021-12-24  911  
ec437d71db77a1 Huang Rui         2021-12-24  912  	cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
ec437d71db77a1 Huang Rui         2021-12-24  913  	if (!cpudata)
ec437d71db77a1 Huang Rui         2021-12-24  914  		return -ENOMEM;
ec437d71db77a1 Huang Rui         2021-12-24  915  
ec437d71db77a1 Huang Rui         2021-12-24  916  	cpudata->cpu = policy->cpu;
ec437d71db77a1 Huang Rui         2021-12-24  917  
f3a052391822b7 Meng Li           2024-01-19  918  	amd_pstate_init_prefcore(cpudata);
f3a052391822b7 Meng Li           2024-01-19  919  
ec437d71db77a1 Huang Rui         2021-12-24  920  	ret = amd_pstate_init_perf(cpudata);
ec437d71db77a1 Huang Rui         2021-12-24  921  	if (ret)
41271016dfa4a0 Huang Rui         2021-12-24  922  		goto free_cpudata1;
ec437d71db77a1 Huang Rui         2021-12-24  923  
5547c0ebfc2efd Perry Yuan        2024-04-25  924  	ret = amd_pstate_init_freq(cpudata);
5547c0ebfc2efd Perry Yuan        2024-04-25  925  	if (ret)
5547c0ebfc2efd Perry Yuan        2024-04-25  926  		goto free_cpudata1;
5547c0ebfc2efd Perry Yuan        2024-04-25  927  
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25  928  	min_freq = READ_ONCE(cpudata->min_freq);
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25  929  	max_freq = READ_ONCE(cpudata->max_freq);
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25  930  	nominal_freq = READ_ONCE(cpudata->nominal_freq);
ec437d71db77a1 Huang Rui         2021-12-24  931  
069a2bb8c48c43 Perry Yuan        2024-04-25  932  	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
069a2bb8c48c43 Perry Yuan        2024-04-25  933  	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
ec437d71db77a1 Huang Rui         2021-12-24  934  
ec437d71db77a1 Huang Rui         2021-12-24  935  	policy->min = min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  936  	policy->max = max_freq;
ec437d71db77a1 Huang Rui         2021-12-24  937  
ec437d71db77a1 Huang Rui         2021-12-24  938  	policy->cpuinfo.min_freq = min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  939  	policy->cpuinfo.max_freq = max_freq;
ec437d71db77a1 Huang Rui         2021-12-24  940  
ec437d71db77a1 Huang Rui         2021-12-24  941  	/* It will be updated by governor */
ec437d71db77a1 Huang Rui         2021-12-24  942  	policy->cur = policy->cpuinfo.min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  943  
e059c184da47e9 Huang Rui         2021-12-24  944  	if (boot_cpu_has(X86_FEATURE_CPPC))
1d215f0319c206 Huang Rui         2021-12-24  945  		policy->fast_switch_possible = true;
1d215f0319c206 Huang Rui         2021-12-24  946  
41271016dfa4a0 Huang Rui         2021-12-24  947  	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
41271016dfa4a0 Huang Rui         2021-12-24  948  				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
41271016dfa4a0 Huang Rui         2021-12-24  949  	if (ret < 0) {
41271016dfa4a0 Huang Rui         2021-12-24  950  		dev_err(dev, "Failed to add min-freq constraint (%d)\n", ret);
41271016dfa4a0 Huang Rui         2021-12-24  951  		goto free_cpudata1;
41271016dfa4a0 Huang Rui         2021-12-24  952  	}
41271016dfa4a0 Huang Rui         2021-12-24  953  
41271016dfa4a0 Huang Rui         2021-12-24  954  	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[1],
41271016dfa4a0 Huang Rui         2021-12-24  955  				   FREQ_QOS_MAX, policy->cpuinfo.max_freq);
41271016dfa4a0 Huang Rui         2021-12-24  956  	if (ret < 0) {
41271016dfa4a0 Huang Rui         2021-12-24  957  		dev_err(dev, "Failed to add max-freq constraint (%d)\n", ret);
41271016dfa4a0 Huang Rui         2021-12-24  958  		goto free_cpudata2;
41271016dfa4a0 Huang Rui         2021-12-24  959  	}
41271016dfa4a0 Huang Rui         2021-12-24  960  
febab20caebac9 Wyes Karny        2023-11-17  961  	cpudata->max_limit_freq = max_freq;
febab20caebac9 Wyes Karny        2023-11-17  962  	cpudata->min_limit_freq = min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  963  
ec437d71db77a1 Huang Rui         2021-12-24  964  	policy->driver_data = cpudata;
ec437d71db77a1 Huang Rui         2021-12-24  965  
41271016dfa4a0 Huang Rui         2021-12-24  966  	amd_pstate_boost_init(cpudata);
abd61c08ef349a Perry Yuan        2023-01-31  967  	if (!current_pstate_driver->adjust_perf)
abd61c08ef349a Perry Yuan        2023-01-31  968  		current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
41271016dfa4a0 Huang Rui         2021-12-24  969  
ec437d71db77a1 Huang Rui         2021-12-24  970  	return 0;
ec437d71db77a1 Huang Rui         2021-12-24  971  
41271016dfa4a0 Huang Rui         2021-12-24  972  free_cpudata2:
41271016dfa4a0 Huang Rui         2021-12-24  973  	freq_qos_remove_request(&cpudata->req[0]);
41271016dfa4a0 Huang Rui         2021-12-24  974  free_cpudata1:
ec437d71db77a1 Huang Rui         2021-12-24  975  	kfree(cpudata);
ec437d71db77a1 Huang Rui         2021-12-24  976  	return ret;
ec437d71db77a1 Huang Rui         2021-12-24  977  }
ec437d71db77a1 Huang Rui         2021-12-24  978
diff mbox series

Patch

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 2db095867d03..be7f2d3c86b6 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -873,6 +873,24 @@  static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
 	WRITE_ONCE(cpudata->max_freq, max_freq);
 
+	/**
+	 * Below values need to be initialized correctly, otherwise driver will be failed to load
+	 * max_freq is calculated accoreding to (nominal_freq * highest_perf)/nominal_perf
+	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
+	 * Check _CPC in ACPI table ojbects if any values are incorrect
+	 */
+	if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {
+		pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
+			min_freq, max_freq, nominal_freq);
+		return -EINVAL;
+	}
+
+	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
+		pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
+			lowest_nonlinear_freq, min_freq, nominal_freq * 1000);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -911,15 +929,6 @@  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	max_freq = READ_ONCE(cpudata->max_freq);
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
 
-	if (min_freq <= 0 || max_freq <= 0 ||
-	    nominal_freq <= 0 || min_freq > max_freq) {
-		dev_err(dev,
-			"min_freq(%d) or max_freq(%d) or nominal_freq (%d) value is incorrect, check _CPC in ACPI tables\n",
-			min_freq, max_freq, nominal_freq);
-		ret = -EINVAL;
-		goto free_cpudata1;
-	}
-
 	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
 	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
 
@@ -1372,14 +1381,6 @@  static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	min_freq = READ_ONCE(cpudata->min_freq);
 	max_freq = READ_ONCE(cpudata->max_freq);
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
-	if (min_freq <= 0 || max_freq <= 0 ||
-	    nominal_freq <= 0 || min_freq > max_freq) {
-		dev_err(dev,
-			"min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect, check _CPC in ACPI tables\n",
-			min_freq, max_freq, nominal_freq);
-		ret = -EINVAL;
-		goto free_cpudata1;
-	}
 
 	policy->cpuinfo.min_freq = min_freq;
 	policy->cpuinfo.max_freq = max_freq;