Messages in this thread | | | From | "Yuan, Perry" <> | Subject | RE: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate driver by default | Date | Wed, 8 May 2024 15:24:07 +0000 |
| |
[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)
| |