* [PATCH v3 0/6] Improve usability for amd-pstate @ 2022-04-14 16:47 Mario Limonciello 2022-04-14 16:47 ` [PATCH v3 1/6] cpufreq: Export acpu_cpufreq_exit for other drivers to call Mario Limonciello ` (5 more replies) 0 siblings, 6 replies; 15+ messages in thread From: Mario Limonciello @ 2022-04-14 16:47 UTC (permalink / raw) To: Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list, 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. By default with this series it will replace acpi-cpufreq no matter the module load order. If users want to prefer acpi-cpufreq when amd-pstate is a module they can make a modprobe configuration file. /etc/modprobe.d/amd-pstate.conf: options amd-pstate replace=0 Mario Limonciello (6): cpufreq: Export acpu_cpufreq_exit for other drivers to call cpufreq: amd-pstate: Only show shared memory solution message once cpufreq: amd-pstate: Move cpufreq driver check later cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded cpufreq: amd-pstate: Add a module device table cpufreq: amd-pstate: Default to replace acpi-cpufreq drivers/cpufreq/acpi-cpufreq.c | 10 +++++++-- drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++++++++++++----- include/linux/cpufreq.h | 3 +++ 3 files changed, 45 insertions(+), 7 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/6] cpufreq: Export acpu_cpufreq_exit for other drivers to call 2022-04-14 16:47 [PATCH v3 0/6] Improve usability for amd-pstate Mario Limonciello @ 2022-04-14 16:47 ` Mario Limonciello 2022-04-14 16:47 ` [PATCH v3 2/6] cpufreq: amd-pstate: Only show shared memory solution message once Mario Limonciello ` (4 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Mario Limonciello @ 2022-04-14 16:47 UTC (permalink / raw) To: Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list, Mario Limonciello Currently, the only way to load an alternative cpufreq driver is to unload acpi-cpufreq first. Loosen that restriction to allow other kernel modules to unregister a registered driver. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v2->v3: * Move to include header * Create a new symbol to prevent NX protected page fault calling exit function from other modules. v1->v2: * Export symbol instead of changing internals drivers/cpufreq/acpi-cpufreq.c | 10 ++++++++-- include/linux/cpufreq.h | 3 +++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 3d514b82d055..38358ed1f932 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -1033,7 +1033,7 @@ static int __init acpi_cpufreq_init(void) return ret; } -static void __exit acpi_cpufreq_exit(void) +void acpi_cpufreq_exit(void) { pr_debug("%s\n", __func__); @@ -1043,6 +1043,12 @@ static void __exit acpi_cpufreq_exit(void) free_acpi_perf_data(); } +EXPORT_SYMBOL_GPL(acpi_cpufreq_exit); + +void __exit acpi_cpufreq_module_exit(void) +{ + acpi_cpufreq_exit(); +} module_param(acpi_pstate_strict, uint, 0644); MODULE_PARM_DESC(acpi_pstate_strict, @@ -1050,7 +1056,7 @@ MODULE_PARM_DESC(acpi_pstate_strict, "performed during frequency changes."); late_initcall(acpi_cpufreq_init); -module_exit(acpi_cpufreq_exit); +module_exit(acpi_cpufreq_module_exit); static const struct x86_cpu_id __maybe_unused acpi_cpufreq_ids[] = { X86_MATCH_FEATURE(X86_FEATURE_ACPI, NULL), diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 35c7d6db4139..223bf9760117 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -273,6 +273,9 @@ static inline void cpufreq_stats_record_transition(struct cpufreq_policy *policy unsigned int new_freq) { } #endif /* CONFIG_CPU_FREQ_STAT */ +#if defined(CONFIG_X86_ACPI_CPUFREQ) || defined(CONFIG_X86_ACPI_CPUFREQ_MODULE) +void acpi_cpufreq_exit(void); +#endif /********************************************************************* * CPUFREQ DRIVER INTERFACE * *********************************************************************/ -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/6] cpufreq: amd-pstate: Only show shared memory solution message once 2022-04-14 16:47 [PATCH v3 0/6] Improve usability for amd-pstate Mario Limonciello 2022-04-14 16:47 ` [PATCH v3 1/6] cpufreq: Export acpu_cpufreq_exit for other drivers to call Mario Limonciello @ 2022-04-14 16:47 ` Mario Limonciello 2022-04-27 13:46 ` Huang Rui 2022-04-14 16:47 ` [PATCH v3 3/6] cpufreq: amd-pstate: Move cpufreq driver check later Mario Limonciello ` (3 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Mario Limonciello @ 2022-04-14 16:47 UTC (permalink / raw) To: Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list, Mario Limonciello A message is emitted to let users know they can enable shared memory, but it shows on all CPUs. As this parameter is system-wide not CPU specific, it doesn't make sense to show 8+ times. Modify it to print only once. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v2->v3: * New patch drivers/cpufreq/amd-pstate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 7be38bc6a673..ecd1fd5e5b5a 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -666,7 +666,7 @@ static int __init amd_pstate_init(void) static_call_update(amd_pstate_init_perf, cppc_init_perf); static_call_update(amd_pstate_update_perf, cppc_update_perf); } else { - pr_info("This processor supports shared memory solution, you can enable it with amd_pstate.shared_mem=1\n"); + pr_info_once("A processor on this system supports the shared memory solution, you can enable it with amd_pstate.shared_mem=1\n"); return -ENODEV; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 2/6] cpufreq: amd-pstate: Only show shared memory solution message once 2022-04-14 16:47 ` [PATCH v3 2/6] cpufreq: amd-pstate: Only show shared memory solution message once Mario Limonciello @ 2022-04-27 13:46 ` Huang Rui 0 siblings, 0 replies; 15+ messages in thread From: Huang Rui @ 2022-04-27 13:46 UTC (permalink / raw) To: Limonciello, Mario Cc: Rafael J . Wysocki, Viresh Kumar, open list:AMD PSTATE DRIVER, Yuan, Perry, open list On Fri, Apr 15, 2022 at 12:47:57AM +0800, Limonciello, Mario wrote: > A message is emitted to let users know they can enable shared memory, > but it shows on all CPUs. > > As this parameter is system-wide not CPU specific, it doesn't make > sense to show 8+ times. Modify it to print only once. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Reviewed-by: Huang Rui <ray.huang@amd.com> > --- > v2->v3: > * New patch > > drivers/cpufreq/amd-pstate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 7be38bc6a673..ecd1fd5e5b5a 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -666,7 +666,7 @@ static int __init amd_pstate_init(void) > static_call_update(amd_pstate_init_perf, cppc_init_perf); > static_call_update(amd_pstate_update_perf, cppc_update_perf); > } else { > - pr_info("This processor supports shared memory solution, you can enable it with amd_pstate.shared_mem=1\n"); > + pr_info_once("A processor on this system supports the shared memory solution, you can enable it with amd_pstate.shared_mem=1\n"); > return -ENODEV; > } > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 3/6] cpufreq: amd-pstate: Move cpufreq driver check later 2022-04-14 16:47 [PATCH v3 0/6] Improve usability for amd-pstate Mario Limonciello 2022-04-14 16:47 ` [PATCH v3 1/6] cpufreq: Export acpu_cpufreq_exit for other drivers to call Mario Limonciello 2022-04-14 16:47 ` [PATCH v3 2/6] cpufreq: amd-pstate: Only show shared memory solution message once Mario Limonciello @ 2022-04-14 16:47 ` Mario Limonciello 2022-04-27 14:45 ` Huang Rui 2022-04-14 16:47 ` [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded Mario Limonciello ` (2 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Mario Limonciello @ 2022-04-14 16:47 UTC (permalink / raw) To: Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list, Mario Limonciello The cpufreq driver check occurs before we verify if the CPU is supported. Depending upon module load order, this may mean that users are never notified they can enable the shared memory solution. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v2->v3: * New patch drivers/cpufreq/amd-pstate.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index ecd1fd5e5b5a..d323f3e3888c 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -653,10 +653,6 @@ static int __init amd_pstate_init(void) return -ENODEV; } - /* don't keep reloading if cpufreq_driver exists */ - if (cpufreq_get_current_driver()) - return -EEXIST; - /* capability check */ if (boot_cpu_has(X86_FEATURE_CPPC)) { pr_debug("AMD CPPC MSR based functionality is supported\n"); @@ -670,6 +666,10 @@ static int __init amd_pstate_init(void) return -ENODEV; } + /* don't keep reloading if cpufreq_driver exists */ + if (cpufreq_get_current_driver()) + return -EEXIST; + /* enable amd pstate feature */ ret = amd_pstate_enable(true); if (ret) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/6] cpufreq: amd-pstate: Move cpufreq driver check later 2022-04-14 16:47 ` [PATCH v3 3/6] cpufreq: amd-pstate: Move cpufreq driver check later Mario Limonciello @ 2022-04-27 14:45 ` Huang Rui 0 siblings, 0 replies; 15+ messages in thread From: Huang Rui @ 2022-04-27 14:45 UTC (permalink / raw) To: Limonciello, Mario Cc: Rafael J . Wysocki, Viresh Kumar, open list:AMD PSTATE DRIVER, Yuan, Perry, open list On Fri, Apr 15, 2022 at 12:47:58AM +0800, Limonciello, Mario wrote: > The cpufreq driver check occurs before we verify if the CPU is supported. > > Depending upon module load order, this may mean that users are never > notified they can enable the shared memory solution. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Acked-by: Huang Rui <ray.huang@amd.com> > --- > v2->v3: > * New patch > > drivers/cpufreq/amd-pstate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index ecd1fd5e5b5a..d323f3e3888c 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -653,10 +653,6 @@ static int __init amd_pstate_init(void) > return -ENODEV; > } > > - /* don't keep reloading if cpufreq_driver exists */ > - if (cpufreq_get_current_driver()) > - return -EEXIST; > - > /* capability check */ > if (boot_cpu_has(X86_FEATURE_CPPC)) { > pr_debug("AMD CPPC MSR based functionality is supported\n"); > @@ -670,6 +666,10 @@ static int __init amd_pstate_init(void) > return -ENODEV; > } > > + /* don't keep reloading if cpufreq_driver exists */ > + if (cpufreq_get_current_driver()) > + return -EEXIST; > + > /* enable amd pstate feature */ > ret = amd_pstate_enable(true); > if (ret) { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded 2022-04-14 16:47 [PATCH v3 0/6] Improve usability for amd-pstate Mario Limonciello ` (2 preceding siblings ...) 2022-04-14 16:47 ` [PATCH v3 3/6] cpufreq: amd-pstate: Move cpufreq driver check later Mario Limonciello @ 2022-04-14 16:47 ` Mario Limonciello 2022-04-14 17:32 ` Nathan Fontenot 2022-04-14 16:48 ` [PATCH v3 5/6] cpufreq: amd-pstate: Add a module device table Mario Limonciello 2022-04-14 16:48 ` [PATCH v3 6/6] cpufreq: amd-pstate: Default to replace acpi-cpufreq Mario Limonciello 5 siblings, 1 reply; 15+ messages in thread From: Mario Limonciello @ 2022-04-14 16:47 UTC (permalink / raw) To: Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list, 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> --- 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 + } /* enable amd pstate feature */ ret = amd_pstate_enable(true); -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded 2022-04-14 16:47 ` [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded Mario Limonciello @ 2022-04-14 17:32 ` Nathan Fontenot 2022-04-14 17:58 ` Limonciello, Mario 0 siblings, 1 reply; 15+ messages in thread From: Nathan Fontenot @ 2022-04-14 17:32 UTC (permalink / raw) To: Mario Limonciello, Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list 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); ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded 2022-04-14 17:32 ` Nathan Fontenot @ 2022-04-14 17:58 ` Limonciello, Mario 2022-04-21 18:38 ` Rafael J. Wysocki 0 siblings, 1 reply; 15+ messages in thread From: Limonciello, Mario @ 2022-04-14 17:58 UTC (permalink / raw) To: Fontenot, Nathan, Huang, Ray, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Yuan, Perry, open list [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); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded 2022-04-14 17:58 ` Limonciello, Mario @ 2022-04-21 18:38 ` Rafael J. Wysocki 2022-04-28 7:15 ` Huang Rui 0 siblings, 1 reply; 15+ messages in thread From: Rafael J. Wysocki @ 2022-04-21 18:38 UTC (permalink / raw) To: Limonciello, Mario Cc: Fontenot, Nathan, Huang, Ray, Rafael J . Wysocki, Viresh Kumar, open list:AMD PSTATE DRIVER, Yuan, Perry, open list On Thu, Apr 14, 2022 at 7:58 PM Limonciello, Mario <Mario.Limonciello@amd.com> wrote: > > [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? IMV there need to be two things to make this really work. First, the currently running driver needs to provide a way to tell it to go away. For example, intel_pstate has the "off" mode (in which it doesn't do anything) for that and similar interfaces can be added to other drivers as needed. The reason why is because, for example, intel_pstate cannot go into the "off" mode when HWP is enabled, because it cannot be disabled and running acpi_cpufreq in that configuration wouldn't work. So in general you need to know that it is OK to unregister the current driver. Second, there needs to be a mechanism for registering a driver "weakly" for future use, so if it cannot be used right away, it will be added to a list and wait until there's room for it to run. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded 2022-04-21 18:38 ` Rafael J. Wysocki @ 2022-04-28 7:15 ` Huang Rui 2022-04-29 2:36 ` Nathan Fontenot 0 siblings, 1 reply; 15+ messages in thread From: Huang Rui @ 2022-04-28 7:15 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Limonciello, Mario, Fontenot, Nathan, Viresh Kumar, open list:AMD PSTATE DRIVER, Yuan, Perry, open list On Fri, Apr 22, 2022 at 02:38:32AM +0800, Rafael J. Wysocki wrote: > On Thu, Apr 14, 2022 at 7:58 PM Limonciello, Mario > <Mario.Limonciello@amd.com> wrote: > > > > [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? > > IMV there need to be two things to make this really work. > > First, the currently running driver needs to provide a way to tell it > to go away. For example, intel_pstate has the "off" mode (in which it > doesn't do anything) for that and similar interfaces can be added to > other drivers as needed. > > The reason why is because, for example, intel_pstate cannot go into > the "off" mode when HWP is enabled, because it cannot be disabled and > running acpi_cpufreq in that configuration wouldn't work. So in > general you need to know that it is OK to unregister the current > driver. > > Second, there needs to be a mechanism for registering a driver > "weakly" for future use, so if it cannot be used right away, it will > be added to a list and wait until there's room for it to run. The amd-pstate is a new module, we need to add "amd-pstate" on /etc/modules-load.d/modules.conf for most of the Linux distro to tell the module to load at boot time. However, there are some issues that we unregister acpi-cpufreq at runtime while the acpi-cpufreq is in-build. As inspired by your suggestion, I am thinking whether we can add "off" mode in acpi-cpufreq driver, if user would like to use the amd-pstate driver on shared memory processors, they can set acpi-cpufreq "off" and set "shared_mem" on amd-pstate to enable the amd-pstate driver. I can add the RST documentation to describe the steps. Or I can introduce a processor list (family id/model id) that can let user know clear which type of processors they are running. Then they can choose which driver that they want to use manually as well. Thanks, Ray ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded 2022-04-28 7:15 ` Huang Rui @ 2022-04-29 2:36 ` Nathan Fontenot 2022-04-29 6:26 ` Huang Rui 0 siblings, 1 reply; 15+ messages in thread From: Nathan Fontenot @ 2022-04-29 2:36 UTC (permalink / raw) To: Huang Rui, Rafael J. Wysocki Cc: Limonciello, Mario, Fontenot, Nathan, Viresh Kumar, open list:AMD PSTATE DRIVER, Yuan, Perry, open list On 4/28/22 02:15, Huang Rui wrote: > On Fri, Apr 22, 2022 at 02:38:32AM +0800, Rafael J. Wysocki wrote: >> On Thu, Apr 14, 2022 at 7:58 PM Limonciello, Mario >> <Mario.Limonciello@amd.com> wrote: >>> >>> [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? >> >> IMV there need to be two things to make this really work. >> >> First, the currently running driver needs to provide a way to tell it >> to go away. For example, intel_pstate has the "off" mode (in which it >> doesn't do anything) for that and similar interfaces can be added to >> other drivers as needed. >> >> The reason why is because, for example, intel_pstate cannot go into >> the "off" mode when HWP is enabled, because it cannot be disabled and >> running acpi_cpufreq in that configuration wouldn't work. So in >> general you need to know that it is OK to unregister the current >> driver. >> >> Second, there needs to be a mechanism for registering a driver >> "weakly" for future use, so if it cannot be used right away, it will >> be added to a list and wait until there's room for it to run. > > The amd-pstate is a new module, we need to add "amd-pstate" on > /etc/modules-load.d/modules.conf for most of the Linux distro to tell the > module to load at boot time. However, there are some issues that we > unregister acpi-cpufreq at runtime while the acpi-cpufreq is in-build. > > As inspired by your suggestion, I am thinking whether we can add "off" mode > in acpi-cpufreq driver, if user would like to use the amd-pstate driver on > shared memory processors, they can set acpi-cpufreq "off" and set > "shared_mem" on amd-pstate to enable the amd-pstate driver. I can add the > RST documentation to describe the steps. > > Or I can introduce a processor list (family id/model id) that can let user > know clear which type of processors they are running. Then they can choose > which driver that they want to use manually as well. > This sounds like a move towards an infrastructure similar to governors where all supported drivers register and users can choose from a list of available drivers. Ray, this seems like a lot of work to be able to dynamically switch to the amd-pstate driver after boot. Given that this behavior does not currently work how crucial is it to have this ability? -Nathan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded 2022-04-29 2:36 ` Nathan Fontenot @ 2022-04-29 6:26 ` Huang Rui 0 siblings, 0 replies; 15+ messages in thread From: Huang Rui @ 2022-04-29 6:26 UTC (permalink / raw) To: Fontenot, Nathan Cc: Rafael J. Wysocki, Limonciello, Mario, Viresh Kumar, open list:AMD PSTATE DRIVER, Yuan, Perry, open list On Fri, Apr 29, 2022 at 10:36:06AM +0800, Fontenot, Nathan wrote: > On 4/28/22 02:15, Huang Rui wrote: > > On Fri, Apr 22, 2022 at 02:38:32AM +0800, Rafael J. Wysocki wrote: > >> On Thu, Apr 14, 2022 at 7:58 PM Limonciello, Mario > >> <Mario.Limonciello@amd.com> wrote: > >>> > >>> [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? > >> > >> IMV there need to be two things to make this really work. > >> > >> First, the currently running driver needs to provide a way to tell it > >> to go away. For example, intel_pstate has the "off" mode (in which it > >> doesn't do anything) for that and similar interfaces can be added to > >> other drivers as needed. > >> > >> The reason why is because, for example, intel_pstate cannot go into > >> the "off" mode when HWP is enabled, because it cannot be disabled and > >> running acpi_cpufreq in that configuration wouldn't work. So in > >> general you need to know that it is OK to unregister the current > >> driver. > >> > >> Second, there needs to be a mechanism for registering a driver > >> "weakly" for future use, so if it cannot be used right away, it will > >> be added to a list and wait until there's room for it to run. > > > > The amd-pstate is a new module, we need to add "amd-pstate" on > > /etc/modules-load.d/modules.conf for most of the Linux distro to tell the > > module to load at boot time. However, there are some issues that we > > unregister acpi-cpufreq at runtime while the acpi-cpufreq is in-build. > > > > As inspired by your suggestion, I am thinking whether we can add "off" mode > > in acpi-cpufreq driver, if user would like to use the amd-pstate driver on > > shared memory processors, they can set acpi-cpufreq "off" and set > > "shared_mem" on amd-pstate to enable the amd-pstate driver. I can add the > > RST documentation to describe the steps. > > > > Or I can introduce a processor list (family id/model id) that can let user > > know clear which type of processors they are running. Then they can choose > > which driver that they want to use manually as well. > > > > This sounds like a move towards an infrastructure similar to governors where > all supported drivers register and users can choose from a list of available > drivers. > > Ray, this seems like a lot of work to be able to dynamically switch to the > amd-pstate driver after boot. Given that this behavior does not currently work > how crucial is it to have this ability? > Hmm, maybe we can enable an easy way to help the users to rework the module in their existing platform like acpi-cpufreq "off" and set "shared_mem" on amd-pstate firstly. Actually, many users mails me that how to enable the module. We can dig out an easy way for them. Thanks, Ray ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 5/6] cpufreq: amd-pstate: Add a module device table 2022-04-14 16:47 [PATCH v3 0/6] Improve usability for amd-pstate Mario Limonciello ` (3 preceding siblings ...) 2022-04-14 16:47 ` [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded Mario Limonciello @ 2022-04-14 16:48 ` Mario Limonciello 2022-04-14 16:48 ` [PATCH v3 6/6] cpufreq: amd-pstate: Default to replace acpi-cpufreq Mario Limonciello 5 siblings, 0 replies; 15+ messages in thread From: Mario Limonciello @ 2022-04-14 16:48 UTC (permalink / raw) To: Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list, 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. Reviewed-by: Huang Rui <ray.huang@amd.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v2->v3: * Add Ray's tag v1->v2: * Add comment to indicate need of SBIOS support. drivers/cpufreq/amd-pstate.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 8ae65a2072d6..3330504b7070 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -710,6 +710,17 @@ static void __exit amd_pstate_exit(void) amd_pstate_enable(false); } +/* + * This will only match the HW feature, there still needs to be appropriate + * SBIOS support, so it's possible that in such cases this causes a module + * load with -ENODEV as the result. + */ +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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 6/6] cpufreq: amd-pstate: Default to replace acpi-cpufreq 2022-04-14 16:47 [PATCH v3 0/6] Improve usability for amd-pstate Mario Limonciello ` (4 preceding siblings ...) 2022-04-14 16:48 ` [PATCH v3 5/6] cpufreq: amd-pstate: Add a module device table Mario Limonciello @ 2022-04-14 16:48 ` Mario Limonciello 5 siblings, 0 replies; 15+ messages in thread From: Mario Limonciello @ 2022-04-14 16:48 UTC (permalink / raw) To: Rui Huang, Rafael J . Wysocki, Viresh Kumar Cc: open list:AMD PSTATE DRIVER, Perry Yuan, open list, Mario Limonciello As a side effect of using the symbol `acpi_cpufreq_exit` attempting to `modprobe amd-pstate` loads the acpi-cpufreq driver if it's compiled as a module. This means that module load order can't work anymore from those changes. To make this a more obvious default behavior set the "replace" module parameter for amd-pstate to true. Expected outcome is that no matter what configuration for module or compiled in, amd-pstate will take precedence. If a user prefers to use acpi_cpufreq, they can set `amd_pstate.replace=0` and then the following outcomes will happen when attempting to load amd-pstate: * acpi_cpufreq module & amd_pstate module -> acpi_cpufreq * acpi_cpufreq builtin & amd_pstate module -> acpi_cpufreq * acpi_cpufreq builtin & amd_pstate builtin -> amd_pstate * acpi_cpufreq module & amd_pstate builtin -> amd_pstate Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- v2->v3: * New patch, added since worse experience from patch 4. drivers/cpufreq/amd-pstate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 3330504b7070..538c9c4cea6d 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -64,7 +64,7 @@ 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; +static bool replace = true; module_param(replace, bool, 0444); MODULE_PARM_DESC(replace, "replace acpi-cpufreq driver upon init if necessary"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-04-29 6:27 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-14 16:47 [PATCH v3 0/6] Improve usability for amd-pstate Mario Limonciello 2022-04-14 16:47 ` [PATCH v3 1/6] cpufreq: Export acpu_cpufreq_exit for other drivers to call Mario Limonciello 2022-04-14 16:47 ` [PATCH v3 2/6] cpufreq: amd-pstate: Only show shared memory solution message once Mario Limonciello 2022-04-27 13:46 ` Huang Rui 2022-04-14 16:47 ` [PATCH v3 3/6] cpufreq: amd-pstate: Move cpufreq driver check later Mario Limonciello 2022-04-27 14:45 ` Huang Rui 2022-04-14 16:47 ` [PATCH v3 4/6] cpufreq: amd-pstate: Allow replacing acpi-cpufreq when loaded Mario Limonciello 2022-04-14 17:32 ` Nathan Fontenot 2022-04-14 17:58 ` Limonciello, Mario 2022-04-21 18:38 ` Rafael J. Wysocki 2022-04-28 7:15 ` Huang Rui 2022-04-29 2:36 ` Nathan Fontenot 2022-04-29 6:26 ` Huang Rui 2022-04-14 16:48 ` [PATCH v3 5/6] cpufreq: amd-pstate: Add a module device table Mario Limonciello 2022-04-14 16:48 ` [PATCH v3 6/6] cpufreq: amd-pstate: Default to replace acpi-cpufreq Mario Limonciello
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.