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

[Nathan Fontenot <nafonten@amd.com>]

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()?

Which leads me to wonder, should there be a more generic cpufreq_replace_driver() routine that
could handle this?

-Nathan

>  
>  	/* enable amd pstate feature */
>  	ret = amd_pstate_enable(true);