| Message ID | 60bdfbeb426512d74faa91597453fd7960ebd7b5.1715065568.git.perry.yuan@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | AMD Pstate Driver Fixes and Improvements | expand |
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@posteo.net> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > --- > 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; > } >
Hello. On úterý 7. května 2024 16:41:29, SELČ Mario Limonciello wrote: > 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@posteo.net> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > --- > > 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. To me it seems like setting it here kills a possibility to set the mode via the kernel cmdline. Regardless of what will be set in `amd_pstate=`, `CONFIG_X86_AMD_PSTATE_DEFAULT_MODE` will be used instead. > > > + 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; > > } > > > > >
[AMD Official Use Only - General] > -----Original Message----- > From: Oleksandr Natalenko <oleksandr@natalenko.name> > Sent: Wednesday, May 8, 2024 11:21 PM > To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com; > viresh.kumar@linaro.org; Huang, Ray <Ray.Huang@amd.com>; Shenoy, > Gautham Ranjal <gautham.shenoy@amd.com>; Petkov, Borislav > <Borislav.Petkov@amd.com>; Limonciello, Mario > <Mario.Limonciello@amd.com> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer > <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng, > Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate > driver by default > > Hello. > > On úterý 7. května 2024 16:41:29, SELČ Mario Limonciello wrote: > > 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@posteo.net> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 > > > Signed-off-by: Perry Yuan <perry.yuan@amd.com> > > > --- > > > 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. > > To me it seems like setting it here kills a possibility to set the mode via the > kernel cmdline. Regardless of what will be set in `amd_pstate=`, > `CONFIG_X86_AMD_PSTATE_DEFAULT_MODE` will be used instead. You are right, the kernel command line will be missed by this change, will fix it in next version. Thanks Perry. > > > > > > + 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; > > > } > > > > > > > > > > > > -- > Oleksandr Natalenko (post-factum)
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; + pr_debug("cppc working state set to mode:%d\n", cppc_state); + 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: 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; }
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@posteo.net> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705 Signed-off-by: Perry Yuan <perry.yuan@amd.com> --- drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 18 deletions(-)