RE: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Fontenot, Nathan <Nathan.Fontenot@xxxxxxx>
> Sent: Thursday, April 14, 2022 12:33
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; Rafael J . Wysocki <rafael@xxxxxxxxxx>; Viresh
> Kumar <viresh.kumar@xxxxxxxxxx>
> Cc: open list:AMD PSTATE DRIVER <linux-pm@xxxxxxxxxxxxxxx>; Yuan, Perry
> <Perry.Yuan@xxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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);