Messages in this thread | | | From | "Limonciello, Mario" <> | Subject | RE: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded | Date | Thu, 14 Apr 2022 17:58:17 +0000 |
| |
[Public]
> -----Original Message----- > From: Fontenot, Nathan <Nathan.Fontenot@amd.com> > Sent: Thursday, April 14, 2022 12:33 > To: Limonciello, Mario <Mario.Limonciello@amd.com>; Huang, Ray > <Ray.Huang@amd.com>; Rafael J . Wysocki <rafael@kernel.org>; Viresh > Kumar <viresh.kumar@linaro.org> > Cc: open list:AMD PSTATE DRIVER <linux-pm@vger.kernel.org>; Yuan, Perry > <Perry.Yuan@amd.com>; open list <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi- > cpufreq when loaded > > On 4/14/22 11:47, Mario Limonciello wrote: > > `amd-pstate` can be compiled as a module. This however can be a > > deficiency because `acpi-cpufreq` will be loaded earlier when compiled > > into the kernel meaning `amd-pstate` doesn't get a chance. > > `acpi-cpufreq` is also unable to be unloaded in this circumstance. > > > > To better improve the usability of `amd-pstate` when compiled as a > module, > > add an optional module parameter that will force it to replace other > > cpufreq drivers at startup. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > v2->v3: > > * Rebase on earlier patches > > * Use IS_REACHABLE > > * Only add replace parameter if acpu-cpufreq is enabled > > * Only show info message once > > v1->v2: > > * Update to changes from v1. > > * Verify the driver being matched is acpi-cpufreq. > > * Show a message letting users know they can use amd-pstate. > > > > drivers/cpufreq/amd-pstate.c | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > > index d323f3e3888c..8ae65a2072d6 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -63,6 +63,13 @@ module_param(shared_mem, bool, 0444); > > MODULE_PARM_DESC(shared_mem, > > "enable amd-pstate on processors with shared memory > solution (false = disabled (default), true = enabled)"); > > > > +#if defined(CONFIG_X86_ACPI_CPUFREQ) || > defined(CONFIG_X86_ACPI_CPUFREQ_MODULE) > > +static bool replace = false; > > +module_param(replace, bool, 0444); > > +MODULE_PARM_DESC(replace, > > + "replace acpi-cpufreq driver upon init if necessary"); > > +#endif > > + > > static struct cpufreq_driver amd_pstate_driver; > > > > /** > > @@ -643,6 +650,7 @@ static struct cpufreq_driver amd_pstate_driver = { > > > > static int __init amd_pstate_init(void) > > { > > + const char *current_driver; > > int ret; > > > > if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > > @@ -666,9 +674,19 @@ static int __init amd_pstate_init(void) > > return -ENODEV; > > } > > > > - /* don't keep reloading if cpufreq_driver exists */ > > - if (cpufreq_get_current_driver()) > > + current_driver = cpufreq_get_current_driver(); > > + if (current_driver) { > > +#if IS_REACHABLE(CONFIG_X86_ACPI_CPUFREQ) > > + if (replace && strcmp(current_driver, "acpi-cpufreq") == 0) { > > + acpi_cpufreq_exit(); > > + } else { > > + pr_info_once("A processor on this system supports > amd-pstate, you can enable it with amd_pstate.replace=1\n"); > > + return -EEXIST; > > + } > > +#else > > return -EEXIST; > > +#endif > > + } > > A couple of thoughts. First, should this also provide a path to restore the > acpi_cpufreq driver > if the amd-pstate driver fails during init some time after calling > acpi_cpufreq_exit()?
I think that's a reasonable idea; it would involve exporting acpi_cpufreq_init as well.
> > Which leads me to wonder, should there be a more generic > cpufreq_replace_driver() routine that > could handle this?
If changing the API for this, my proposal would be that there is a flag used in cpufreq_driver->flags to indicate that this driver should replace existing drivers when calling cpufreq_register_driver rather than a new routine. Then if it fails to register for any reason then the old driver can be restored.
Rafael, what are your thoughts on this?
> > -Nathan > > > > > /* enable amd pstate feature */ > > ret = amd_pstate_enable(true);
| |