[PATCH 0/3] Improve usability for amd-pstate

[PATCH 0/3] Improve usability for amd-pstate

[Mario Limonciello]

There has recently been some news coverage about `amd-pstate` being in
5.17, but this news also mentioned that it's a bit difficult to use.

You need to either block init calls, or compile the module into the kernel
to force it to take precedence over acpi-cpufreq.

This series aims to improve the usability of amd-pstate so that distros
can compile as a module, but users can still use it (relatively) easily.

A new module parameter is included that will force amd-pstate to take
precedence and a module table to let it load automatically on such systems.

With the patches in this series a user can make a file
/etc/modprobe.d/amd-pstate.conf:

options amd-pstate replace=1

Then upon the next reboot amd-pstate should load automatically even if
acpi-cpufreq was included on the system.
Mario Limonciello (3):
  cpufreq: Allow passing NULL as the argument for unregistering a driver
  cpufreq: amd-pstate: Allow replacing existing cpufreq drivers when
    loaded
  cpufreq: amd-pstate: Add a module device table

 drivers/cpufreq/amd-pstate.c | 19 ++++++++++++++++---
 drivers/cpufreq/cpufreq.c    |  4 ++--
 2 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/3] cpufreq: Allow passing NULL as the argument for unregistering a driver

[Mario Limonciello]

Currently the arguments passed to `cpufreq_unregister_driver` are matched
against the currently registered driver.

This means that the only way for a driver to be unregistered is if it's
module is unloaded.  Loosen that restriction to allow other kernel modules
to unregister a registered driver.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/cpufreq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 80f535cc8a75..4711c17a89bb 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2885,10 +2885,10 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
 {
 	unsigned long flags;
 
-	if (!cpufreq_driver || (driver != cpufreq_driver))
+	if (!cpufreq_driver)
 		return -EINVAL;
 
-	pr_debug("unregistering driver %s\n", driver->name);
+	pr_debug("unregistering driver %s\n", cpufreq_driver->name);
 
 	/* Protect against concurrent cpu hotplug */
 	cpus_read_lock();
-- 
2.34.1

[PATCH 2/3] cpufreq: amd-pstate: Allow replacing existing cpufreq drivers when loaded

[Mario Limonciello]

`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>
---
 drivers/cpufreq/amd-pstate.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 9ce75ed11f8e..31a04e818195 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -63,6 +63,11 @@ 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)");
 
+static bool replace = false;
+module_param(replace, bool, 0444);
+MODULE_PARM_DESC(replace,
+		  "replace other cpufreq drivers upon init if necessary");
+
 static struct cpufreq_driver amd_pstate_driver;
 
 /**
@@ -598,9 +603,11 @@ static int __init amd_pstate_init(void)
 		return -ENODEV;
 	}
 
-	/* don't keep reloading if cpufreq_driver exists */
-	if (cpufreq_get_current_driver())
-		return -EEXIST;
+	if (cpufreq_get_current_driver()) {
+		ret = replace ? cpufreq_unregister_driver(NULL) : -EEXIST;
+		if (ret)
+			return ret;
+	}
 
 	/* capability check */
 	if (boot_cpu_has(X86_FEATURE_CPPC)) {
-- 
2.34.1

[PATCH 3/3] cpufreq: amd-pstate: Add a module device table

[Mario Limonciello]

`amd-pstate` currently only loads automatically if compiled into the
kernel.  To improve the usability, add a module device table that
will load when AMD CPUs that support CPPC are detected.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 31a04e818195..44490292fa72 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -644,6 +644,12 @@ static void __exit amd_pstate_exit(void)
 	amd_pstate_enable(false);
 }
 
+static const struct x86_cpu_id __maybe_unused amd_pstate_ids[] = {
+	X86_MATCH_VENDOR_FEATURE(AMD, X86_FEATURE_CPPC, NULL),
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, amd_pstate_ids);
+
 module_init(amd_pstate_init);
 module_exit(amd_pstate_exit);
 
-- 
2.34.1

Re: [PATCH 0/3] Improve usability for amd-pstate

[Huang Rui]

On Fri, Mar 25, 2022 at 01:42:25PM +0800, Limonciello, Mario wrote:
> There has recently been some news coverage about `amd-pstate` being in
> 5.17, but this news also mentioned that it's a bit difficult to use.
> 
> You need to either block init calls, or compile the module into the kernel
> to force it to take precedence over acpi-cpufreq.
> 
> This series aims to improve the usability of amd-pstate so that distros
> can compile as a module, but users can still use it (relatively) easily.
> 
> A new module parameter is included that will force amd-pstate to take
> precedence and a module table to let it load automatically on such systems.
> 
> With the patches in this series a user can make a file
> /etc/modprobe.d/amd-pstate.conf:
> 
> options amd-pstate replace=1

Actually, because the amd-pstate is fairly new for current distos, we can
modify /etc/modules-load.d/modules.conf to add one line "amd_pstate" to
inform the system this module should be loaded at boot time.

But your method also looks fine for me as well, the amd-pstate can force to
replace the acpi-cpufreq. I am not sure whether anyone objects to this way.

Thanks,
Ray

> 
> Then upon the next reboot amd-pstate should load automatically even if
> acpi-cpufreq was included on the system.
> Mario Limonciello (3):
>   cpufreq: Allow passing NULL as the argument for unregistering a driver
>   cpufreq: amd-pstate: Allow replacing existing cpufreq drivers when
>     loaded
>   cpufreq: amd-pstate: Add a module device table
> 
>  drivers/cpufreq/amd-pstate.c | 19 ++++++++++++++++---
>  drivers/cpufreq/cpufreq.c    |  4 ++--
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 1/3] cpufreq: Allow passing NULL as the argument for unregistering a driver

[Huang Rui]

On Fri, Mar 25, 2022 at 01:42:26PM +0800, Limonciello, Mario wrote:
> Currently the arguments passed to `cpufreq_unregister_driver` are matched
> against the currently registered driver.
> 
> This means that the only way for a driver to be unregistered is if it's
> module is unloaded.  Loosen that restriction to allow other kernel modules
> to unregister a registered driver.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/cpufreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 80f535cc8a75..4711c17a89bb 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2885,10 +2885,10 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver)
>  {
>  	unsigned long flags;
>  
> -	if (!cpufreq_driver || (driver != cpufreq_driver))
> +	if (!cpufreq_driver)
>  		return -EINVAL;
>  
> -	pr_debug("unregistering driver %s\n", driver->name);
> +	pr_debug("unregistering driver %s\n", cpufreq_driver->name);
>  

This is amd-pstate only use case, I suggest we only modify the amd-pstate
driver, and don't impact generic cpufreq. Actually, only acpi-cpufreq could
be registered earlier than amd-pstate. So how about exposing the
acpi_cpufreq_exit(), and call this to free the acpi_cpufreq_driver in
amd-pstate directly?

Thanks,
Ray

Re: [PATCH 3/3] cpufreq: amd-pstate: Add a module device table

[Huang Rui]

On Fri, Mar 25, 2022 at 01:42:28PM +0800, Limonciello, Mario wrote:
> `amd-pstate` currently only loads automatically if compiled into the
> kernel.  To improve the usability, add a module device table that
> will load when AMD CPUs that support CPPC are detected.
> 

There is one AMD semi-custom processor which has the CPPC feature flag, but
the SBIOS doesn't support _CPC objects.

Thanks,
Ray

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 31a04e818195..44490292fa72 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -644,6 +644,12 @@ static void __exit amd_pstate_exit(void)
>  	amd_pstate_enable(false);
>  }
>  
> +static const struct x86_cpu_id __maybe_unused amd_pstate_ids[] = {
> +	X86_MATCH_VENDOR_FEATURE(AMD, X86_FEATURE_CPPC, NULL),
> +	{}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, amd_pstate_ids);
> +
>  module_init(amd_pstate_init);
>  module_exit(amd_pstate_exit);
>  
> -- 
> 2.34.1
> 

RE: [PATCH 0/3] Improve usability for amd-pstate

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Huang, Ray <Ray.Huang@amd.com>
> Sent: Sunday, March 27, 2022 06:56
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Rafael J . Wysocki <rafael@kernel.org>; Viresh Kumar
> <viresh.kumar@linaro.org>; open list:AMD PSTATE DRIVER <linux-
> pm@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH 0/3] Improve usability for amd-pstate
> 
> On Fri, Mar 25, 2022 at 01:42:25PM +0800, Limonciello, Mario wrote:
> > There has recently been some news coverage about `amd-pstate` being in
> > 5.17, but this news also mentioned that it's a bit difficult to use.
> >
> > You need to either block init calls, or compile the module into the kernel
> > to force it to take precedence over acpi-cpufreq.
> >
> > This series aims to improve the usability of amd-pstate so that distros
> > can compile as a module, but users can still use it (relatively) easily.
> >
> > A new module parameter is included that will force amd-pstate to take
> > precedence and a module table to let it load automatically on such systems.
> >
> > With the patches in this series a user can make a file
> > /etc/modprobe.d/amd-pstate.conf:
> >
> > options amd-pstate replace=1
> 
> Actually, because the amd-pstate is fairly new for current distos, we can
> modify /etc/modules-load.d/modules.conf to add one line "amd_pstate" to
> inform the system this module should be loaded at boot time.

Actually I don't believe that would work in the case that acpi-cpufreq is built-in
or gets loaded first.

> 
> But your method also looks fine for me as well, the amd-pstate can force to
> replace the acpi-cpufreq. I am not sure whether anyone objects to this way.

Thanks for your suggestions in the series, I'll adopt them for v2.

> 
> Thanks,
> Ray
> 
> >
> > Then upon the next reboot amd-pstate should load automatically even if
> > acpi-cpufreq was included on the system.
> > Mario Limonciello (3):
> >   cpufreq: Allow passing NULL as the argument for unregistering a driver
> >   cpufreq: amd-pstate: Allow replacing existing cpufreq drivers when
> >     loaded
> >   cpufreq: amd-pstate: Add a module device table
> >
> >  drivers/cpufreq/amd-pstate.c | 19 ++++++++++++++++---
> >  drivers/cpufreq/cpufreq.c    |  4 ++--
> >  2 files changed, 18 insertions(+), 5 deletions(-)
> >
> > --
> > 2.34.1
> >