Re: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate driver by default

From: Mario Limonciello
Date: Tue May 07 2024 - 10:41:46 EST


On 5/7/2024 02:15, Perry Yuan wrote:
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 <andamu@xxxxxxxxxx>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
---
drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 3ff381c4edf7..f5368497ee79 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -66,7 +66,7 @@
static struct cpufreq_driver *current_pstate_driver;
static struct cpufreq_driver amd_pstate_driver;
static struct cpufreq_driver amd_pstate_epp_driver;
-static int cppc_state = AMD_PSTATE_UNDEFINED;
+static int cppc_state;
static bool cppc_enabled;
static bool amd_pstate_prefcore = true;
static struct quirk_entry *quirks;
@@ -1762,6 +1762,16 @@ static int __init amd_pstate_init(void)
if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
return -ENODEV;
+ /* 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");
+ return -ENODEV;
+ }
+
/* show debug message only if CPPC is not supported */
if (!amd_cppc_supported())
return -EOPNOTSUPP;
@@ -1781,28 +1791,21 @@ 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*/
+ cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;

Rather than setting it here within amd_pstate_init() I think you can just set it at line 67 to CONFIG_X86_AMD_PSTATE_DEFAULT_MODE.

+ pr_debug("cppc working state set to mode:%d\n", cppc_state);

I think this debug line is going to be unnecessary when it's already hardcoded to kernel config.

+
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_UNDEFINED:

With the intent of this patch I no longer see a need for AMD_PSTATE_UNDEFINED in the rest of the driver (or in this case statement even).

I feel you can drop it from amd-pstate.h.

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;
@@ -1823,7 +1826,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) with return %d\n", cppc_state, ret);
return ret;
}