`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 <[email protected]>
---
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
[Public]
> -----Original Message-----
> From: Fontenot, Nathan <[email protected]>
> Sent: Thursday, April 14, 2022 12:33
> To: Limonciello, Mario <[email protected]>; Huang, Ray
> <[email protected]>; Rafael J . Wysocki <[email protected]>; Viresh
> Kumar <[email protected]>
> Cc: open list:AMD PSTATE DRIVER <[email protected]>; Yuan, Perry
> <[email protected]>; open list <[email protected]>
> 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 <[email protected]>
> > ---
> > 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);
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 <[email protected]>
> ---
> 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);
On Thu, Apr 14, 2022 at 7:58 PM Limonciello, Mario
<[email protected]> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Fontenot, Nathan <[email protected]>
> > Sent: Thursday, April 14, 2022 12:33
> > To: Limonciello, Mario <[email protected]>; Huang, Ray
> > <[email protected]>; Rafael J . Wysocki <[email protected]>; Viresh
> > Kumar <[email protected]>
> > Cc: open list:AMD PSTATE DRIVER <[email protected]>; Yuan, Perry
> > <[email protected]>; open list <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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.
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
> <[email protected]> wrote:
> >
> > [Public]
> >
> >
> >
> > > -----Original Message-----
> > > From: Fontenot, Nathan <[email protected]>
> > > Sent: Thursday, April 14, 2022 12:33
> > > To: Limonciello, Mario <[email protected]>; Huang, Ray
> > > <[email protected]>; Rafael J . Wysocki <[email protected]>; Viresh
> > > Kumar <[email protected]>
> > > Cc: open list:AMD PSTATE DRIVER <[email protected]>; Yuan, Perry
> > > <[email protected]>; open list <[email protected]>
> > > 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 <[email protected]>
> > > > ---
> > > > 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
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
>> <[email protected]> wrote:
>>>
>>> [Public]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Fontenot, Nathan <[email protected]>
>>>> Sent: Thursday, April 14, 2022 12:33
>>>> To: Limonciello, Mario <[email protected]>; Huang, Ray
>>>> <[email protected]>; Rafael J . Wysocki <[email protected]>; Viresh
>>>> Kumar <[email protected]>
>>>> Cc: open list:AMD PSTATE DRIVER <[email protected]>; Yuan, Perry
>>>> <[email protected]>; open list <[email protected]>
>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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
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
> >> <[email protected]> wrote:
> >>>
> >>> [Public]
> >>>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Fontenot, Nathan <[email protected]>
> >>>> Sent: Thursday, April 14, 2022 12:33
> >>>> To: Limonciello, Mario <[email protected]>; Huang, Ray
> >>>> <[email protected]>; Rafael J . Wysocki <[email protected]>; Viresh
> >>>> Kumar <[email protected]>
> >>>> Cc: open list:AMD PSTATE DRIVER <[email protected]>; Yuan, Perry
> >>>> <[email protected]>; open list <[email protected]>
> >>>> 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 <[email protected]>
> >>>>> ---
> >>>>> 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