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

Yuan, Perry
2024-05-08T15:24:18Z

[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)