AMD Pstate Driver Fixes and Improvements

To enhance the debugging capability of the driver loading failure for broken CPPC ACPI tables, it 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 <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

drivers/cpufreq/amd-pstate.c +5 -0

index 6a342b0c0140..614f6fac0764 100644
@@ -889,6 +889,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 fail to load
+	 * max_freq is calculated according to (nominal_freq * highest_perf)/nominal_perf
+	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
+	 * Check _CPC in ACPI table objects if any values are incorrect

drivers/cpufreq/amd-pstate.c +3 -17

+		return -EINVAL;
+	}
+
 	return 0;
 }
 

@@ -927,15 +945,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);
 

@@ -1388,14 +1397,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;

To enhance the debugging capability of the driver loading failure for broken CPPC ACPI tables, it 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 <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

drivers/cpufreq/amd-pstate.c +18 -17

index 6a342b0c0140..614f6fac0764 100644
@@ -889,6 +889,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 fail to load
+	 * max_freq is calculated according to (nominal_freq * highest_perf)/nominal_perf
+	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
+	 * Check _CPC in ACPI table objects 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;
 }
 

@@ -927,15 +945,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);
 

@@ -1388,14 +1397,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;

Signature · 1 line

2.34.1

removed the unused variable nominal_freq for build warning. This variable was defined and assigned a value in the previous code, but it was not used in the subsequent code.

Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here

Reported-by kernel test robot <[email protected]>

Signed-off-by Perry Yuan <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/cpufreq/amd-pstate.c +2 -4

index 614f6fac0764..9ca22e94f3f3 100644
@@ -912,7 +912,7 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 
 static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 {
-	int min_freq, max_freq, nominal_freq, ret;
+	int min_freq, max_freq, ret;
 	struct device *dev;
 	struct amd_cpudata *cpudata;
 

@@ -943,7 +943,6 @@ static int amd_pstate_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);
 
 	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
 	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);

@@ -1363,7 +1362,7 @@ static bool amd_pstate_acpi_pm_profile_undefined(void)
 
 static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 {
-	int min_freq, max_freq, nominal_freq, ret;
+	int min_freq, max_freq, ret;
 	struct amd_cpudata *cpudata;
 	struct device *dev;
 	u64 value;

@@ -1396,7 +1395,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);
 
 	policy->cpuinfo.min_freq = min_freq;
 	policy->cpuinfo.max_freq = max_freq;

Signature · 1 line

2.34.1

Add CPU ID checking in case the driver attempt to load on systems where CPPC functionality is unavailable. And the warning message will not be shown if CPPC is not supported.

It will also print debug message if the CPU has no CPPC support that helps to debug the driver loading failure issue.

Reported-by Paul Menzel <[email protected]>

Closes https://lore.kernel.org/linux-pm/CYYPR12MB8655D32EA18574C9497E888A9C122@CYYPR12MB8655.namprd12.prod.outlook.com/T/#t open here

Signed-off-by Perry Yuan <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

drivers/cpufreq/amd-pstate.c +19 -0

index 9ca22e94f3f3..3109b46fb02e 100644
@@ -1701,6 +1701,20 @@ static int __init amd_pstate_set_driver(int mode_idx)
 	return -EINVAL;
 }
 
+/**
+ * CPPC function is not supported for family ID 17H with model_ID ranging from 0x10 to 0x2F.
+ * show the debug message that helps to check if the CPU has CPPC support for loading issue.
+ */
+static bool amd_cppc_supported(void)
+{
+	if ((boot_cpu_data.x86 == 0x17) && (boot_cpu_data.x86_model < 0x30)) {
+		pr_debug_once("CPPC feature is not supported by the processor\n");
+		return false;
+	}
+
+	return true;
+}
+
 static int __init amd_pstate_init(void)
 {
 	struct device *dev_root;

@@ -1709,6 +1723,11 @@ static int __init amd_pstate_init(void)
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
 		return -ENODEV;
 
+	/* show debug message only if CPPC is not supported */
+	if (!amd_cppc_supported())
+		return -EOPNOTSUPP;
+
+	/* show warning message when BIOS broken or ACPI disabled */
 	if (!acpi_cpc_valid()) {
 		pr_warn_once("the _CPC object is not present in SBIOS or ACPI disabled\n");
 		return -ENODEV;

Signature · 1 line

2.34.1

If the amd-pstate driver is not loaded automatically by default, it is because the kernel command line parameter has not been added. To resolve this issue, it is necessary to call the amd_pstate_set_driver() function to enable the desired mode (passive/active/guided) before registering the driver instance.
This ensures that the driver is loaded correctly without relying on the kernel command line parameter.

[    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd
[    0.982579] amd_pstate: failed to register with return -22

Reported-by Andrei Amuraritei <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=218705

Signed-off-by Perry Yuan <[email protected]>

---
 drivers/cpufreq/amd-pstate.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

drivers/cpufreq/amd-pstate.c +4 -0

index dce901a403c9..03342fef7d94 100644
@@ -1785,28 +1785,30 @@ static int __init amd_pstate_init(void)
 	/* check if this machine need CPPC quirks */
 	dmi_check_system(amd_pstate_quirks_table);
 
+	/* get default driver mode for loading */
+	if (cppc_state == AMD_PSTATE_UNDEFINED)
+		cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
+

drivers/cpufreq/amd-pstate.c +5 -17

 	switch (cppc_state) {
-	case AMD_PSTATE_UNDEFINED:
-		/* Disable on the following configs by default:
-		 * 1. Undefined platforms
-		 * 2. Server platforms
-		 * 3. Shared memory designs
-		 */
-		if (amd_pstate_acpi_pm_profile_undefined() ||
-		    amd_pstate_acpi_pm_profile_server() ||
-		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
-			pr_info("driver load is disabled, boot with specific mode to enable this\n");
-			return -ENODEV;
-		}
-		ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
-		if (ret)
-			return ret;
-		break;
 	case AMD_PSTATE_DISABLE:
+		pr_info("driver load is disabled, boot with specific mode to enable this\n");
 		return -ENODEV;
 	case AMD_PSTATE_PASSIVE:
 	case AMD_PSTATE_ACTIVE:
 	case AMD_PSTATE_GUIDED:
+		ret = amd_pstate_set_driver(cppc_state);
+		if (ret)
+			return ret;
 		break;
 	default:
 		return -EINVAL;

@@ -1827,7 +1829,7 @@ static int __init amd_pstate_init(void)
 	/* enable amd pstate feature */
 	ret = amd_pstate_enable(true);
 	if (ret) {
-		pr_err("failed to enable with return %d\n", ret);
+		pr_err("failed to enable driver mode(%d)\n", cppc_state);
 		return ret;
 	}
 

-----Original Message-----
From: Ugwekar, Dhananjay <[email protected]>
Sent: Monday, May 13, 2024 5:37 PM
To: Yuan, Perry <[email protected]>; Limonciello, Mario <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Huang, Ray <[email protected]>; Petkov, Borislav <[email protected]>
Cc: [email protected]; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v2 10/10] cpufreq: amd-pstate: automatically load pstate driver by default

Hello Perry,

Quoted 9 lines · Perry Yuan

If the amd-pstate driver is not loaded automatically by default, it is because the kernel command line parameter has not been added. To resolve this issue, it is necessary to call the amd_pstate_set_driver() function to enable the desired mode (passive/active/guided) before registering the driver instance. This ensures that the driver is loaded correctly without relying on the kernel command line parameter.

[    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-

v1 xhci-hcd

Quoted 5 lines

[    0.982579] amd_pstate: failed to register with return -22

Reported-by Andrei Amuraritei <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=218705

Signed-off-by Perry Yuan <[email protected]>

Quoted 7 lines

---
 drivers/cpufreq/amd-pstate.c | 36
+++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

(quoted diff) +0 -0

b/drivers/cpufreq/amd-pstate.c index dce901a403c9..03342fef7d94

100644

Quoted 7 lines

drivers/cpufreq/amd-pstate.c +1 -0

@@ -1785,28 +1785,30 @@ static int __init amd_pstate_init(void)
    /* check if this machine need CPPC quirks */
    dmi_check_system(amd_pstate_quirks_table);
 
+   /* get default driver mode for loading */

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 1798

Quoted 10 lines

drivers/cpufreq/amd-pstate.c +10 -0

+   if (cppc_state == AMD_PSTATE_UNDEFINED)
+           cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
+
+   /* Disable on the following configs by default:
+    * 1. Undefined platforms
+    * 2. Server platforms
+    */
+   if (amd_pstate_acpi_pm_profile_undefined() ||
+           amd_pstate_acpi_pm_profile_server()) {
+           pr_info("driver load is disabled for server or undefined

platform\n");

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 1801

Quoted 3 lines

drivers/cpufreq/amd-pstate.c +3 -0

+           return -ENODEV;
+   }
+

Wont this change make it impossible to use amd-pstate on server platforms? It wont work even if we pass "amd-pstate=active" in cmdline, because this check is before the switch case.

Thanks,
Dhananjay

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void)

Quoted 11 lines

drivers/cpufreq/amd-pstate.c +0 -10

    switch (cppc_state) {
-   case AMD_PSTATE_UNDEFINED:
-           /* Disable on the following configs by default:
-            * 1. Undefined platforms
-            * 2. Server platforms
-            * 3. Shared memory designs
-            */
-           if (amd_pstate_acpi_pm_profile_undefined() ||
-               amd_pstate_acpi_pm_profile_server() ||
-               !cpu_feature_enabled(X86_FEATURE_CPPC)) {
-                   pr_info("driver load is disabled, boot with specific

mode to enable this\n");

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void)

Quoted 3 lines

drivers/cpufreq/amd-pstate.c +0 -3

-                   return -ENODEV;
-           }
-           ret =

amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void)

Quoted 5 lines

drivers/cpufreq/amd-pstate.c +1 -3

-           if (ret)
-                   return ret;
-           break;
    case AMD_PSTATE_DISABLE:
+           pr_info("driver load is disabled, boot with specific mode to

enable

Quoted 20 lines

drivers/cpufreq/amd-pstate.c +5 -1

+this\n");
            return -ENODEV;
    case AMD_PSTATE_PASSIVE:
    case AMD_PSTATE_ACTIVE:
    case AMD_PSTATE_GUIDED:
+           ret = amd_pstate_set_driver(cppc_state);
+           if (ret)
+                   return ret;
            break;
    default:
            return -EINVAL;

@@ -1827,7 +1829,7 @@ static int __init amd_pstate_init(void)
    /* enable amd pstate feature */
    ret = amd_pstate_enable(true);
    if (ret) {
-           pr_err("failed to enable with return %d\n", ret);
+           pr_err("failed to enable driver mode(%d)\n", cppc_state);
            return ret;
    }