[PATCH 2/2] e_powersaver: Require setting a module parameter to enable it

[PATCH 2/2] e_powersaver: Require setting a module parameter to enable it

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

The Kconfig text and comment at the top of the file say this is
DANGEROUS.  According to Rafał Bilski it is not so bad as it used to
be, but he still didn't think it should be auto-loaded.

Rafał already made a similar change to longhaul.

References: http://www.spinics.net/lists/cpufreq/msg02919.html
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/cpufreq/e_powersaver.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
index a0d2a42..880be39 100644
--- a/drivers/cpufreq/e_powersaver.c
+++ b/drivers/cpufreq/e_powersaver.c
@@ -45,6 +45,7 @@ static struct eps_cpu_data *eps_cpu[NR_CPUS];
 static int freq_failsafe_off;
 static int voltage_failsafe_off;
 static int set_max_voltage;
+static int enable;
 
 #if defined CONFIG_ACPI_PROCESSOR || defined CONFIG_ACPI_PROCESSOR_MODULE
 static int ignore_acpi_limit;
@@ -410,6 +411,10 @@ static int __init eps_init(void)
 {
 	if (!x86_match_cpu(eps_cpu_id) || boot_cpu_data.x86_model < 10)
 		return -ENODEV;
+	if (!enable) {
+		printk(KERN_ERR "eps: Option \"enable\" not set. Aborting.\n");
+		return -ENODEV;
+	}
 	if (cpufreq_register_driver(&eps_driver))
 		return -EINVAL;
 	return 0;
@@ -432,6 +437,10 @@ MODULE_PARM_DESC(ignore_acpi_limit, "Don't check ACPI's processor speed limit");
 #endif
 module_param(set_max_voltage, int, 0644);
 MODULE_PARM_DESC(set_max_voltage, "Set maximum CPU voltage (mV) C7-M only");
+/* By default driver is disabled to prevent incompatible
+ * system freeze. */
+module_param(enable, int, 0644);
+MODULE_PARM_DESC(enable, "Enable driver");
 
 MODULE_AUTHOR("Rafal Bilski <rafalbilski@interia.pl>");
 MODULE_DESCRIPTION("Enhanced PowerSaver driver for VIA C7 CPU's.");

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] e_powersaver: Require setting a module parameter to enable it

[Viresh Kumar]

On 9 June 2014 04:52, Ben Hutchings <ben@decadent.org.uk> wrote:
> The Kconfig text and comment at the top of the file say this is
> DANGEROUS.  According to Rafał Bilski it is not so bad as it used to
> be, but he still didn't think it should be auto-loaded.
>
> Rafał already made a similar change to longhaul.
>
> References: http://www.spinics.net/lists/cpufreq/msg02919.html
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  drivers/cpufreq/e_powersaver.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
> index a0d2a42..880be39 100644
> --- a/drivers/cpufreq/e_powersaver.c
> +++ b/drivers/cpufreq/e_powersaver.c
> @@ -45,6 +45,7 @@ static struct eps_cpu_data *eps_cpu[NR_CPUS];
>  static int freq_failsafe_off;
>  static int voltage_failsafe_off;
>  static int set_max_voltage;
> +static int enable;
>
>  #if defined CONFIG_ACPI_PROCESSOR || defined CONFIG_ACPI_PROCESSOR_MODULE
>  static int ignore_acpi_limit;
> @@ -410,6 +411,10 @@ static int __init eps_init(void)
>  {
>         if (!x86_match_cpu(eps_cpu_id) || boot_cpu_data.x86_model < 10)
>                 return -ENODEV;
> +       if (!enable) {
> +               printk(KERN_ERR "eps: Option \"enable\" not set. Aborting.\n");
> +               return -ENODEV;
> +       }
>         if (cpufreq_register_driver(&eps_driver))
>                 return -EINVAL;
>         return 0;
> @@ -432,6 +437,10 @@ MODULE_PARM_DESC(ignore_acpi_limit, "Don't check ACPI's processor speed limit");
>  #endif
>  module_param(set_max_voltage, int, 0644);
>  MODULE_PARM_DESC(set_max_voltage, "Set maximum CPU voltage (mV) C7-M only");
> +/* By default driver is disabled to prevent incompatible
> + * system freeze. */

This could have been written with proper style mentioned in CodingGuidelines

> +module_param(enable, int, 0644);
> +MODULE_PARM_DESC(enable, "Enable driver");
>
>  MODULE_AUTHOR("Rafal Bilski <rafalbilski@interia.pl>");
>  MODULE_DESCRIPTION("Enhanced PowerSaver driver for VIA C7 CPU's.");

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 2/2] e_powersaver: Require setting a module parameter to enable it

[Rafał Bilski]

> The Kconfig text and comment at the top of the file say this is
> DANGEROUS.  According to Rafał Bilski it is not so bad as it used to
> be, but he still didn't think it should be auto-loaded.
>
> Rafał already made a similar change to longhaul.
>
> References: http://www.spinics.net/lists/cpufreq/msg02919.html
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>   drivers/cpufreq/e_powersaver.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/cpufreq/e_powersaver.c b/drivers/cpufreq/e_powersaver.c
> index a0d2a42..880be39 100644
> --- a/drivers/cpufreq/e_powersaver.c
> +++ b/drivers/cpufreq/e_powersaver.c
> @@ -45,6 +45,7 @@ static struct eps_cpu_data *eps_cpu[NR_CPUS];
>   static int freq_failsafe_off;
>   static int voltage_failsafe_off;
>   static int set_max_voltage;
> +static int enable;
>   
>   #if defined CONFIG_ACPI_PROCESSOR || defined CONFIG_ACPI_PROCESSOR_MODULE
>   static int ignore_acpi_limit;
> @@ -410,6 +411,10 @@ static int __init eps_init(void)
>   {
>   	if (!x86_match_cpu(eps_cpu_id) || boot_cpu_data.x86_model < 10)
>   		return -ENODEV;
> +	if (!enable) {
> +		printk(KERN_ERR "eps: Option \"enable\" not set. Aborting.\n");
> +		return -ENODEV;
> +	}
>   	if (cpufreq_register_driver(&eps_driver))
>   		return -EINVAL;
>   	return 0;
> @@ -432,6 +437,10 @@ MODULE_PARM_DESC(ignore_acpi_limit, "Don't check ACPI's processor speed limit");
>   #endif
>   module_param(set_max_voltage, int, 0644);
>   MODULE_PARM_DESC(set_max_voltage, "Set maximum CPU voltage (mV) C7-M only");
> +/* By default driver is disabled to prevent incompatible
> + * system freeze. */
> +module_param(enable, int, 0644);
> +MODULE_PARM_DESC(enable, "Enable driver");
>   
>   MODULE_AUTHOR("Rafal Bilski <rafalbilski@interia.pl>");
>   MODULE_DESCRIPTION("Enhanced PowerSaver driver for VIA C7 CPU's.");
>
Actually that's quite good solution for conflict between ACPI P-states driver and this
driver. In this way ACPI will always be loaded first unless user will choose otherwise.
I would like to point out that this text and one in Kconfig is a bit incorrect. Processor
won't allow any frequency outside it's limits by design. Processor may get overheated
if system isn't designed for CPU to run at full frequency and BIOS sets limits by means
of P-states tables. Otherwise only difference is ACPI P-states driver seems to be using
SMI to change frequency while e_powersaver uses MSRs.

Rafał Bilski