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

["Yuan, Perry" <Perry.Yuan@amd.com>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Dhananjay

> -----Original Message-----
> From: Ugwekar, Dhananjay <Dhananjay.Ugwekar@amd.com>
> Sent: Monday, May 13, 2024 5:37 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Huang, Ray <Ray.Huang@amd.com>; Petkov,
> Borislav <Borislav.Petkov@amd.com>
> Cc: rafael.j.wysocki@intel.com; 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 v2 10/10] cpufreq: amd-pstate: automatically load pstate
> driver by default
>
> Hello Perry,
>
> On 5/13/2024 7:37 AM, 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 | 36
> > +++++++++++++++++++-----------------
> >  1 file changed, 19 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index dce901a403c9..03342fef7d94
> 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -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;
> > +
> > +   /* 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;
> > +   }
> > +
>
> 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
>

Good catch,  the original checking is disabling server platform by default, it should not block the server platform when using "amd-pstate=active" in cmdline,  will fix it in v3 to allow parameter input for server platfrm from command line.


Regards.
Perry

> >     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;
> >     }
> >