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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 8 May 2024 15:24:07 +0000
From: "Yuan, Perry" <Perry.Yuan@....com>
To: Oleksandr Natalenko <oleksandr@...alenko.name>,
	"rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
	"viresh.kumar@...aro.org" <viresh.kumar@...aro.org>, "Huang, Ray"
	<Ray.Huang@....com>, "Shenoy, Gautham Ranjal" <gautham.shenoy@....com>,
	"Petkov, Borislav" <Borislav.Petkov@....com>, "Limonciello, Mario"
	<Mario.Limonciello@....com>
CC: "Deucher, Alexander" <Alexander.Deucher@....com>, "Huang, Shimmer"
	<Shimmer.Huang@....com>, "Du, Xiaojian" <Xiaojian.Du@....com>, "Meng, Li
 (Jassmine)" <Li.Meng@....com>, "linux-pm@...r.kernel.org"
	<linux-pm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate
 driver by default

[AMD Official Use Only - General]

> -----Original Message-----
> From: Oleksandr Natalenko <oleksandr@...alenko.name>
> Sent: Wednesday, May 8, 2024 11:21 PM
> To: Yuan, Perry <Perry.Yuan@....com>; rafael.j.wysocki@...el.com;
> viresh.kumar@...aro.org; Huang, Ray <Ray.Huang@....com>; Shenoy,
> Gautham Ranjal <gautham.shenoy@....com>; Petkov, Borislav
> <Borislav.Petkov@....com>; Limonciello, Mario
> <Mario.Limonciello@....com>
> Cc: Deucher, Alexander <Alexander.Deucher@....com>; Huang, Shimmer
> <Shimmer.Huang@....com>; Du, Xiaojian <Xiaojian.Du@....com>; Meng,
> Li (Jassmine) <Li.Meng@....com>; linux-pm@...r.kernel.org; linux-
> kernel@...r.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@...teo.net>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
> > > Signed-off-by: Perry Yuan <perry.yuan@....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)