linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
@ 2019-11-14 21:14 Giovanni Mascellani
  2019-11-14 21:39 ` Guenter Roeck
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Giovanni Mascellani @ 2019-11-14 21:14 UTC (permalink / raw)
  To: Pali Rohár, Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel
  Cc: Giovanni Mascellani

On some Dell laptops the BIOS directly controls fan speed, ignoring
SET_FAN commands. Also, the BIOS controller often happens to be buggy,
failing to increase fan speed when the CPU is under heavy load and
setting fan at full speed even when the CPU is idle and relatively
cool.

Disable BIOS fan control on such laptops as soon as a SET_FAN command
is issued, and re-enable it at module unloading, so that a userspace
controller like i8kmon can take control of the fan.

There is a missing feature: interaction with PM; I think that when
suspending on RAM the fans should be stopped (this BIOS doesn't always
do this automatically, neither when fan control is enabled nor when it
is disabled); when recovering from hibernation to disk, also, the
command to disable BIOS fan control should be issued again, because
the computer will have had a power cycle in the meantime.

I don't know how to implement these two actions; can someone suggest a
way? Also, I would be happy to know from more experienced people if
these actions are sensible.

Signed-off-by: Giovanni Mascellani <gio@debian.org>
---
 drivers/hwmon/dell-smm-hwmon.c | 64 ++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 4212d022d253..6d72e207076f 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -32,6 +32,12 @@
 
 #include <linux/i8k.h>
 
+/*
+ * The code for enabling and disabling BIOS fan control were found by
+ * trial and error with the program at
+ *  https://github.com/clopez/dellfan.
+ */
+
 #define I8K_SMM_FN_STATUS	0x0025
 #define I8K_SMM_POWER_STATUS	0x0069
 #define I8K_SMM_SET_FAN		0x01a3
@@ -43,6 +49,8 @@
 #define I8K_SMM_GET_TEMP_TYPE	0x11a3
 #define I8K_SMM_GET_DELL_SIG1	0xfea3
 #define I8K_SMM_GET_DELL_SIG2	0xffa3
+#define I8K_SMM_DISABLE_BIOS	0x30a3
+#define I8K_SMM_ENABLE_BIOS	0x31a3
 
 #define I8K_FAN_MULT		30
 #define I8K_FAN_MAX_RPM		30000
@@ -68,6 +76,8 @@ static uint i8k_pwm_mult;
 static uint i8k_fan_max = I8K_FAN_HIGH;
 static bool disallow_fan_type_call;
 static bool disallow_fan_support;
+static bool disable_bios;
+static bool bios_disabled;
 
 #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
@@ -419,6 +429,26 @@ static int i8k_get_power_status(void)
 	return (regs.eax & 0xff) == I8K_POWER_AC ? I8K_AC : I8K_BATTERY;
 }
 
+/*
+ * Disable BIOS fan control.
+ */
+static int i8k_disable_bios(void)
+{
+	struct smm_regs regs = { .eax = I8K_SMM_DISABLE_BIOS, .ebx = 0 };
+
+	return i8k_smm(&regs) ? : regs.eax & 0xff;
+}
+
+/*
+ * Enable BIOS fan control.
+ */
+static int i8k_enable_bios(void)
+{
+	struct smm_regs regs = { .eax = I8K_SMM_ENABLE_BIOS, .ebx = 0 };
+
+	return i8k_smm(&regs) ? : regs.eax & 0xff;
+}
+
 /*
  * Procfs interface
  */
@@ -488,6 +518,13 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&speed, argp + 1, sizeof(int)))
 			return -EFAULT;
 
+                if (disable_bios && !bios_disabled) {
+			val = i8k_disable_bios();
+			if (val < 0)
+				return val;
+			bios_disabled = true;
+                }
+
 		val = i8k_set_fan(val, speed);
 		break;
 
@@ -1135,6 +1172,22 @@ static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
 	{ }
 };
 
+/*
+ * On some machines the BIOS disregards all SET_FAN commands unless it
+ * is explicitly disabled.
+ * See bug: https://bugzilla.kernel.org/show_bug.cgi?id=200949
+ */
+static struct dmi_system_id i8k_disable_bios_dmi_table[] __initdata = {
+	{
+		.ident = "Dell Precision 5530",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Precision 5530"),
+		},
+	},
+	{ }
+};
+
 /*
  * Probe for the presence of a supported laptop.
  */
@@ -1169,6 +1222,11 @@ static int __init i8k_probe(void)
 			disallow_fan_type_call = true;
 	}
 
+	if (dmi_check_system(i8k_disable_bios_dmi_table)) {
+		pr_warn("broken Dell BIOS detected, will disable BIOS fan control\n");
+		disable_bios = true;
+	}
+
 	strlcpy(bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
 		sizeof(bios_version));
 	strlcpy(bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
@@ -1241,6 +1299,12 @@ static void __exit i8k_exit(void)
 {
 	hwmon_device_unregister(i8k_hwmon_dev);
 	i8k_exit_procfs();
+
+	if (bios_disabled) {
+		pr_warn("re-enabling BIOS fan control\n");
+		if (i8k_enable_bios())
+			pr_warn("could not re-enable BIOS fan control\n");
+	}
 }
 
 module_init(i8k_init);
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-14 21:14 [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN Giovanni Mascellani
@ 2019-11-14 21:39 ` Guenter Roeck
  2019-11-14 21:51   ` Pali Rohár
  2019-11-15  7:12 ` kbuild test robot
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2019-11-14 21:39 UTC (permalink / raw)
  To: Giovanni Mascellani
  Cc: Pali Rohár, Jean Delvare, linux-hwmon, linux-kernel

On Thu, Nov 14, 2019 at 10:14:08PM +0100, Giovanni Mascellani wrote:
> On some Dell laptops the BIOS directly controls fan speed, ignoring
> SET_FAN commands. Also, the BIOS controller often happens to be buggy,
> failing to increase fan speed when the CPU is under heavy load and
> setting fan at full speed even when the CPU is idle and relatively
> cool.
> 
> Disable BIOS fan control on such laptops as soon as a SET_FAN command
> is issued, and re-enable it at module unloading, so that a userspace
> controller like i8kmon can take control of the fan.
> 
> There is a missing feature: interaction with PM; I think that when
> suspending on RAM the fans should be stopped (this BIOS doesn't always
> do this automatically, neither when fan control is enabled nor when it
> is disabled); when recovering from hibernation to disk, also, the
> command to disable BIOS fan control should be issued again, because
> the computer will have had a power cycle in the meantime.
> 
> I don't know how to implement these two actions; can someone suggest a
> way? Also, I would be happy to know from more experienced people if
> these actions are sensible.
> 

I think this is too drastic: Not everyone wants to use this driver for
fan control, and even automatic switching to manual mode on write
operations may be unexpected.

I can see two possibilities: Either add a pwm1_enable attribute to
be able to set manual/automatic fan control, or add a module parameter
to enable manual fan speed control on affected systems.

As for PM support, this would have to be implemented using the standard
PM functions.

Guenter

> Signed-off-by: Giovanni Mascellani <gio@debian.org>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 64 ++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 4212d022d253..6d72e207076f 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -32,6 +32,12 @@
>  
>  #include <linux/i8k.h>
>  
> +/*
> + * The code for enabling and disabling BIOS fan control were found by
> + * trial and error with the program at
> + *  https://github.com/clopez/dellfan.
> + */
> +
>  #define I8K_SMM_FN_STATUS	0x0025
>  #define I8K_SMM_POWER_STATUS	0x0069
>  #define I8K_SMM_SET_FAN		0x01a3
> @@ -43,6 +49,8 @@
>  #define I8K_SMM_GET_TEMP_TYPE	0x11a3
>  #define I8K_SMM_GET_DELL_SIG1	0xfea3
>  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> +#define I8K_SMM_DISABLE_BIOS	0x30a3
> +#define I8K_SMM_ENABLE_BIOS	0x31a3
>  
>  #define I8K_FAN_MULT		30
>  #define I8K_FAN_MAX_RPM		30000
> @@ -68,6 +76,8 @@ static uint i8k_pwm_mult;
>  static uint i8k_fan_max = I8K_FAN_HIGH;
>  static bool disallow_fan_type_call;
>  static bool disallow_fan_support;
> +static bool disable_bios;
> +static bool bios_disabled;
>  
>  #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
>  #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
> @@ -419,6 +429,26 @@ static int i8k_get_power_status(void)
>  	return (regs.eax & 0xff) == I8K_POWER_AC ? I8K_AC : I8K_BATTERY;
>  }
>  
> +/*
> + * Disable BIOS fan control.
> + */
> +static int i8k_disable_bios(void)
> +{
> +	struct smm_regs regs = { .eax = I8K_SMM_DISABLE_BIOS, .ebx = 0 };
> +
> +	return i8k_smm(&regs) ? : regs.eax & 0xff;
> +}
> +
> +/*
> + * Enable BIOS fan control.
> + */
> +static int i8k_enable_bios(void)
> +{
> +	struct smm_regs regs = { .eax = I8K_SMM_ENABLE_BIOS, .ebx = 0 };
> +
> +	return i8k_smm(&regs) ? : regs.eax & 0xff;
> +}
> +
>  /*
>   * Procfs interface
>   */
> @@ -488,6 +518,13 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&speed, argp + 1, sizeof(int)))
>  			return -EFAULT;
>  
> +                if (disable_bios && !bios_disabled) {
> +			val = i8k_disable_bios();
> +			if (val < 0)
> +				return val;
> +			bios_disabled = true;
> +                }
> +
>  		val = i8k_set_fan(val, speed);
>  		break;
>  
> @@ -1135,6 +1172,22 @@ static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
>  	{ }
>  };
>  
> +/*
> + * On some machines the BIOS disregards all SET_FAN commands unless it
> + * is explicitly disabled.
> + * See bug: https://bugzilla.kernel.org/show_bug.cgi?id=200949
> + */
> +static struct dmi_system_id i8k_disable_bios_dmi_table[] __initdata = {
> +	{
> +		.ident = "Dell Precision 5530",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Precision 5530"),
> +		},
> +	},
> +	{ }
> +};
> +
>  /*
>   * Probe for the presence of a supported laptop.
>   */
> @@ -1169,6 +1222,11 @@ static int __init i8k_probe(void)
>  			disallow_fan_type_call = true;
>  	}
>  
> +	if (dmi_check_system(i8k_disable_bios_dmi_table)) {
> +		pr_warn("broken Dell BIOS detected, will disable BIOS fan control\n");

The above is your interpretation: Since there is a command to enable/disable
BIOS fan control, we can not just claim that it is broken because we don't like
how it works.

> +		disable_bios = true;
> +	}
> +
>  	strlcpy(bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
>  		sizeof(bios_version));
>  	strlcpy(bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
> @@ -1241,6 +1299,12 @@ static void __exit i8k_exit(void)
>  {
>  	hwmon_device_unregister(i8k_hwmon_dev);
>  	i8k_exit_procfs();
> +
> +	if (bios_disabled) {
> +		pr_warn("re-enabling BIOS fan control\n");

I don't see the point of this warning.

> +		if (i8k_enable_bios())
> +			pr_warn("could not re-enable BIOS fan control\n");
> +	}
>  }
>  
>  module_init(i8k_init);
> -- 
> 2.24.0
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-14 21:39 ` Guenter Roeck
@ 2019-11-14 21:51   ` Pali Rohár
  2019-11-15  9:51     ` Giovanni Mascellani
  0 siblings, 1 reply; 13+ messages in thread
From: Pali Rohár @ 2019-11-14 21:51 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Giovanni Mascellani, Jean Delvare, linux-hwmon, linux-kernel

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

On Thursday 14 November 2019 13:39:01 Guenter Roeck wrote:
> I can see two possibilities: Either add a pwm1_enable attribute to
> be able to set manual/automatic fan control,

I already proposed such patch in past:
https://patchwork.kernel.org/patch/9130921/

> > @@ -43,6 +49,8 @@
> >  #define I8K_SMM_GET_TEMP_TYPE	0x11a3
> >  #define I8K_SMM_GET_DELL_SIG1	0xfea3
> >  #define I8K_SMM_GET_DELL_SIG2	0xffa3
> > +#define I8K_SMM_DISABLE_BIOS	0x30a3
> > +#define I8K_SMM_ENABLE_BIOS	0x31a3

This is model or BIOS specific. For example on E6440 are used 0x34a3 /
0x35a3 SMM calls. Because of these platform specific problems we have
never incorporated this patch into mainline kernel.

Also note that userspace can issue those SMM commands on its own (via
sys_iopl or sys_ioperm), fully bypassing such "protection" proposed in
this new patch.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-14 21:14 [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN Giovanni Mascellani
  2019-11-14 21:39 ` Guenter Roeck
@ 2019-11-15  7:12 ` kbuild test robot
  2019-11-15  8:19 ` kbuild test robot
  2019-11-16 15:03 ` Pali Rohár
  3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-11-15  7:12 UTC (permalink / raw)
  To: Giovanni Mascellani
  Cc: kbuild-all, Pali Rohár, Jean Delvare, Guenter Roeck,
	linux-hwmon, linux-kernel, Giovanni Mascellani

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

Hi Giovanni,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v5.4-rc7 next-20191114]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Giovanni-Mascellani/hwmon-dell-smm-hwmon-Disable-BIOS-fan-control-on-SET_FAN/20191115-052315
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-c001-20191115 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/hwmon/dell-smm-hwmon.c: In function 'i8k_exit':
>> drivers/hwmon/dell-smm-hwmon.c:1305:7: error: implicit declaration of function 'i8k_enable_bios' [-Werror=implicit-function-declaration]
      if (i8k_enable_bios())
          ^~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/i8k_enable_bios +1305 drivers/hwmon/dell-smm-hwmon.c

  1297	
  1298	static void __exit i8k_exit(void)
  1299	{
  1300		hwmon_device_unregister(i8k_hwmon_dev);
  1301		i8k_exit_procfs();
  1302	
  1303		if (bios_disabled) {
  1304			pr_warn("re-enabling BIOS fan control\n");
> 1305			if (i8k_enable_bios())
  1306				pr_warn("could not re-enable BIOS fan control\n");
  1307		}
  1308	}
  1309	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30155 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-14 21:14 [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN Giovanni Mascellani
  2019-11-14 21:39 ` Guenter Roeck
  2019-11-15  7:12 ` kbuild test robot
@ 2019-11-15  8:19 ` kbuild test robot
  2019-11-16 15:03 ` Pali Rohár
  3 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-11-15  8:19 UTC (permalink / raw)
  To: Giovanni Mascellani
  Cc: kbuild-all, Pali Rohár, Jean Delvare, Guenter Roeck,
	linux-hwmon, linux-kernel, Giovanni Mascellani

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

Hi Giovanni,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v5.4-rc7 next-20191114]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Giovanni-Mascellani/hwmon-dell-smm-hwmon-Disable-BIOS-fan-control-on-SET_FAN/20191115-052315
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-h001-20191115 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/ioport.h:13:0,
                    from include/linux/device.h:15,
                    from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from drivers/hwmon/dell-smm-hwmon.c:15:
   drivers/hwmon/dell-smm-hwmon.c: In function 'i8k_exit':
   drivers/hwmon/dell-smm-hwmon.c:1305:7: error: implicit declaration of function 'i8k_enable_bios' [-Werror=implicit-function-declaration]
      if (i8k_enable_bios())
          ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^~~~
>> drivers/hwmon/dell-smm-hwmon.c:1305:3: note: in expansion of macro 'if'
      if (i8k_enable_bios())
      ^~
   Cyclomatic Complexity 1 include/linux/err.h:PTR_ERR
   Cyclomatic Complexity 1 include/linux/percpu-defs.h:__this_cpu_preempt_check
   Cyclomatic Complexity 1 include/linux/dmi.h:dmi_check_system
   Cyclomatic Complexity 1 include/linux/dmi.h:dmi_get_system_info
   Cyclomatic Complexity 1 include/linux/dmi.h:dmi_first_match
   Cyclomatic Complexity 3 drivers/hwmon/dell-smm-hwmon.c:i8k_get_dmi_data
   Cyclomatic Complexity 1 drivers/hwmon/dell-smm-hwmon.c:i8k_init_procfs
   Cyclomatic Complexity 14 drivers/hwmon/dell-smm-hwmon.c:i8k_smm_func
   Cyclomatic Complexity 124 drivers/hwmon/dell-smm-hwmon.c:i8k_is_visible
   Cyclomatic Complexity 1 include/linux/cpu.h:get_online_cpus
   Cyclomatic Complexity 1 include/linux/cpu.h:put_online_cpus
   Cyclomatic Complexity 1 drivers/hwmon/dell-smm-hwmon.c:i8k_smm
   Cyclomatic Complexity 6 drivers/hwmon/dell-smm-hwmon.c:i8k_get_dell_signature
   Cyclomatic Complexity 5 drivers/hwmon/dell-smm-hwmon.c:i8k_get_fan_nominal_speed
   Cyclomatic Complexity 64 drivers/hwmon/dell-smm-hwmon.c:i8k_probe
   Cyclomatic Complexity 5 drivers/hwmon/dell-smm-hwmon.c:i8k_get_fan_speed
   Cyclomatic Complexity 2 drivers/hwmon/dell-smm-hwmon.c:_i8k_get_temp
   Cyclomatic Complexity 2 drivers/hwmon/dell-smm-hwmon.c:i8k_get_temp_type
   Cyclomatic Complexity 5 drivers/hwmon/dell-smm-hwmon.c:i8k_get_fan_status
   Cyclomatic Complexity 6 drivers/hwmon/dell-smm-hwmon.c:i8k_set_fan
   Cyclomatic Complexity 10 drivers/hwmon/dell-smm-hwmon.c:_i8k_get_fan_type
   Cyclomatic Complexity 4 drivers/hwmon/dell-smm-hwmon.c:i8k_get_fan_type
   Cyclomatic Complexity 1 include/linux/kernel.h:kstrtoul
   Cyclomatic Complexity 5 drivers/hwmon/dell-smm-hwmon.c:i8k_hwmon_pwm_store
   Cyclomatic Complexity 4 drivers/hwmon/dell-smm-hwmon.c:i8k_hwmon_pwm_show
   Cyclomatic Complexity 11 drivers/hwmon/dell-smm-hwmon.c:i8k_hwmon_fan_label_show
   Cyclomatic Complexity 4 drivers/hwmon/dell-smm-hwmon.c:i8k_hwmon_fan_show
   Cyclomatic Complexity 7 drivers/hwmon/dell-smm-hwmon.c:i8k_hwmon_temp_label_show
   Cyclomatic Complexity 7 drivers/hwmon/dell-smm-hwmon.c:i8k_get_temp
   Cyclomatic Complexity 4 drivers/hwmon/dell-smm-hwmon.c:i8k_hwmon_temp_show
   Cyclomatic Complexity 1 include/linux/err.h:IS_ERR
   Cyclomatic Complexity 51 drivers/hwmon/dell-smm-hwmon.c:i8k_init_hwmon
   Cyclomatic Complexity 6 drivers/hwmon/dell-smm-hwmon.c:i8k_init
   Cyclomatic Complexity 1 drivers/hwmon/dell-smm-hwmon.c:i8k_exit_procfs
   Cyclomatic Complexity 6 drivers/hwmon/dell-smm-hwmon.c:i8k_exit
   cc1: some warnings being treated as errors

vim +/if +1305 drivers/hwmon/dell-smm-hwmon.c

  1297	
  1298	static void __exit i8k_exit(void)
  1299	{
  1300		hwmon_device_unregister(i8k_hwmon_dev);
  1301		i8k_exit_procfs();
  1302	
  1303		if (bios_disabled) {
  1304			pr_warn("re-enabling BIOS fan control\n");
> 1305			if (i8k_enable_bios())
  1306				pr_warn("could not re-enable BIOS fan control\n");
  1307		}
  1308	}
  1309	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32644 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-14 21:51   ` Pali Rohár
@ 2019-11-15  9:51     ` Giovanni Mascellani
  2019-11-15 11:29       ` Pali Rohár
  0 siblings, 1 reply; 13+ messages in thread
From: Giovanni Mascellani @ 2019-11-15  9:51 UTC (permalink / raw)
  To: Pali Rohár, Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1356 bytes --]

Hi,

Il 14/11/19 22:51, Pali Rohár ha scritto:
> This is model or BIOS specific. For example on E6440 are used 0x34a3 /
> 0x35a3 SMM calls. Because of these platform specific problems we have
> never incorporated this patch into mainline kernel.

Would it be sensible to use a dmi_system_id table to discriminate
between the known models and choose the right commands? Of course we
wouldn't know the complete table at the beginning, but it can be filled
as unknown models are reported.

As a matter of facts, testing your patch I discovered that 0x34a3 /
0x35a3 work on my system as well (Dell Precision 5530). Do you know
systems on which other codes only are known to work?

> Also note that userspace can issue those SMM commands on its own (via
> sys_iopl or sys_ioperm), fully bypassing such "protection" proposed in
> this new patch.

Yes, I know, but this is incompatible with Secure Boot, so I believe
that this feature should go in the kernel module, and userspace should
eventually stop doing direct requests and rely on the module. Isn't
userspace sidestepping the kernel in this way already assumed to take
their own responsibilities, much like userspace writing random things to
/dev/mem?

Thanks, Giovanni.
-- 
Giovanni Mascellani <g.mascellani@gmail.com>
Postdoc researcher - Université Libre de Bruxelles


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-15  9:51     ` Giovanni Mascellani
@ 2019-11-15 11:29       ` Pali Rohár
  2019-11-15 13:44         ` Giovanni Mascellani
  0 siblings, 1 reply; 13+ messages in thread
From: Pali Rohár @ 2019-11-15 11:29 UTC (permalink / raw)
  To: Giovanni Mascellani
  Cc: Guenter Roeck, Jean Delvare, linux-hwmon, linux-kernel

Hi!

On Friday 15 November 2019 10:51:48 Giovanni Mascellani wrote:
> Hi,
> 
> Il 14/11/19 22:51, Pali Rohár ha scritto:
> > This is model or BIOS specific. For example on E6440 are used 0x34a3 /
> > 0x35a3 SMM calls. Because of these platform specific problems we have
> > never incorporated this patch into mainline kernel.
> 
> Would it be sensible to use a dmi_system_id table to discriminate
> between the known models and choose the right commands? Of course we
> wouldn't know the complete table at the beginning, but it can be filled
> as unknown models are reported.

Yes, we can create a whitelist table with known models and correct SMM
commands.

> As a matter of facts, testing your patch I discovered that 0x34a3 /
> 0x35a3 work on my system as well (Dell Precision 5530). Do you know
> systems on which other codes only are known to work?

No. I have not tested that my patch on other models. You can look at
that my patch link, some people already reported that on some models it
does not work...

> > Also note that userspace can issue those SMM commands on its own (via
> > sys_iopl or sys_ioperm), fully bypassing such "protection" proposed in
> > this new patch.
> 
> Yes, I know, but this is incompatible with Secure Boot

What is incompatible with Secure Boot? sys_iopl nor sys_ioperm has
nothing to do with UEFI Secure Boot. They are just ordinary syscalls
like any other and are executed on kernel side. And IIRC it is up to the
kernel how it set privileges for I/O ports. Maybe bootloaders under
Secure Boot can set some other default values, but kernel can overwrite
them. I really do not see reason why it could be incompatible with UEFI
Secure Boot nor why it should not work. It just looks like improper
setup from userspace.

> so I believe
> that this feature should go in the kernel module, and userspace should
> eventually stop doing direct requests and rely on the module. Isn't
> userspace sidestepping the kernel in this way already assumed to take
> their own responsibilities, much like userspace writing random things to
> /dev/mem?

It makes sense to have implemented in kernel switching between automatic
and manual mode as kernel has API for it. But it depends on the fact
that we know which SMM API to call. And currently it is just some random
call which we somehow observed that is working on 2 machines and on some
more other does not. Until we have fully working implementation we
cannot put it into kernel.

What does not make sense for me is to have that "protection" in kernel.

-- 
Pali Rohár
pali.rohar@gmail.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-15 11:29       ` Pali Rohár
@ 2019-11-15 13:44         ` Giovanni Mascellani
  2019-11-15 14:36           ` Pali Rohár
  0 siblings, 1 reply; 13+ messages in thread
From: Giovanni Mascellani @ 2019-11-15 13:44 UTC (permalink / raw)
  To: Pali Rohár; +Cc: Guenter Roeck, Jean Delvare, linux-hwmon, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 2592 bytes --]

Hi,

Il 15/11/19 12:29, Pali Rohár ha scritto:
> No. I have not tested that my patch on other models. You can look at
> that my patch link, some people already reported that on some models it
> does not work...

Yes, I saw that. But they are also using other laptops, which could be
excluded by the whitelist until we have a working command for them.

> What is incompatible with Secure Boot? sys_iopl nor sys_ioperm has
> nothing to do with UEFI Secure Boot. They are just ordinary syscalls
> like any other and are executed on kernel side. And IIRC it is up to the
> kernel how it set privileges for I/O ports. Maybe bootloaders under
> Secure Boot can set some other default values, but kernel can overwrite
> them. I really do not see reason why it could be incompatible with UEFI
> Secure Boot nor why it should not work. It just looks like improper
> setup from userspace.

Ah, my fault here: there is a patch to lock down the kernel when it is
started with Secure Boot[1], and I believed that was already in
mainline, but apparently it is not.

 [1]
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit?id=160a99536afc317b337212dd40eaba341702343e

> It makes sense to have implemented in kernel switching between automatic
> and manual mode as kernel has API for it. But it depends on the fact
> that we know which SMM API to call. And currently it is just some random
> call which we somehow observed that is working on 2 machines and on some
> more other does not. Until we have fully working implementation we
> cannot put it into kernel.

Mmh, but then what is a plausible way forward to have this? Can't we
start populating a whitelist for the machines we already have a solution
for, and add more entries when they are discovered? This would already
give a benefit to the users of supported laptops, without impacting
users of unsupported laptops. My feeling is that if we pretend to have
information for all possible models supported by dell-smm-hwmon, we will
never benefit any user. Or can you suggest a plan?

> What does not make sense for me is to have that "protection" in kernel.

I am not really sure which "protection" you mean. I didn't mean to
introduce any protection from userspace in my patch, I just wanted to
make SET_FAN working. I think that the kernel module cannot (and should
not) reliably protect itself from userspace sending random IO port
reads/writes.

Thanks, Giovanni.
-- 
Giovanni Mascellani <g.mascellani@gmail.com>
Postdoc researcher - Université Libre de Bruxelles


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-15 13:44         ` Giovanni Mascellani
@ 2019-11-15 14:36           ` Pali Rohár
  2019-11-15 14:46             ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Pali Rohár @ 2019-11-15 14:36 UTC (permalink / raw)
  To: Giovanni Mascellani
  Cc: Guenter Roeck, Jean Delvare, linux-hwmon, linux-kernel

On Friday 15 November 2019 14:44:59 Giovanni Mascellani wrote:
> Hi,
> 
> Il 15/11/19 12:29, Pali Rohár ha scritto:
> > No. I have not tested that my patch on other models. You can look at
> > that my patch link, some people already reported that on some models it
> > does not work...
> 
> Yes, I saw that. But they are also using other laptops, which could be
> excluded by the whitelist until we have a working command for them.
> 
> > What is incompatible with Secure Boot? sys_iopl nor sys_ioperm has
> > nothing to do with UEFI Secure Boot. They are just ordinary syscalls
> > like any other and are executed on kernel side. And IIRC it is up to the
> > kernel how it set privileges for I/O ports. Maybe bootloaders under
> > Secure Boot can set some other default values, but kernel can overwrite
> > them. I really do not see reason why it could be incompatible with UEFI
> > Secure Boot nor why it should not work. It just looks like improper
> > setup from userspace.
> 
> Ah, my fault here: there is a patch to lock down the kernel when it is
> started with Secure Boot[1], and I believed that was already in
> mainline, but apparently it is not.

Ok, so, this is not a problem.

I hope that such patch is not going to be part of mainline kernel as it
would break lot of things. UEFI Secure Boot and kernel lock down are two
different things. It would be really suspicious that for "workarounding"
broken functionality would be needed to turn of totally unrelated option
in firmware SETUP.

>  [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit?id=160a99536afc317b337212dd40eaba341702343e
> 
> > It makes sense to have implemented in kernel switching between automatic
> > and manual mode as kernel has API for it. But it depends on the fact
> > that we know which SMM API to call. And currently it is just some random
> > call which we somehow observed that is working on 2 machines and on some
> > more other does not. Until we have fully working implementation we
> > cannot put it into kernel.
> 
> Mmh, but then what is a plausible way forward to have this? Can't we
> start populating a whitelist for the machines we already have a solution
> for, and add more entries when they are discovered? This would already
> give a benefit to the users of supported laptops, without impacting
> users of unsupported laptops. My feeling is that if we pretend to have
> information for all possible models supported by dell-smm-hwmon, we will
> never benefit any user. Or can you suggest a plan?

The following question is up to the hwmon maintainers. Guenter, should
we start creating whilelist of machines which support those SMM calls
for enabling / disabling BIOS auto mode? And maintaining this whilelist
in kernel dell-smm-hwmon driver?

> > What does not make sense for me is to have that "protection" in kernel.
> 
> I am not really sure which "protection" you mean. I didn't mean to
> introduce any protection from userspace in my patch, I just wanted to
> make SET_FAN working. I think that the kernel module cannot (and should
> not) reliably protect itself from userspace sending random IO port
> reads/writes.

I mean protection to disallow calling SET_FAN operation when auto mode
is enabled.

-- 
Pali Rohár
pali.rohar@gmail.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-15 14:36           ` Pali Rohár
@ 2019-11-15 14:46             ` Guenter Roeck
  2019-11-15 19:10               ` Giovanni Mascellani
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2019-11-15 14:46 UTC (permalink / raw)
  To: Pali Rohár, Giovanni Mascellani
  Cc: Jean Delvare, linux-hwmon, linux-kernel

On 11/15/19 6:36 AM, Pali Rohár wrote:
> On Friday 15 November 2019 14:44:59 Giovanni Mascellani wrote:
>> Hi,
>>
>> Il 15/11/19 12:29, Pali Rohár ha scritto:
>>> No. I have not tested that my patch on other models. You can look at
>>> that my patch link, some people already reported that on some models it
>>> does not work...
>>
>> Yes, I saw that. But they are also using other laptops, which could be
>> excluded by the whitelist until we have a working command for them.
>>
>>> What is incompatible with Secure Boot? sys_iopl nor sys_ioperm has
>>> nothing to do with UEFI Secure Boot. They are just ordinary syscalls
>>> like any other and are executed on kernel side. And IIRC it is up to the
>>> kernel how it set privileges for I/O ports. Maybe bootloaders under
>>> Secure Boot can set some other default values, but kernel can overwrite
>>> them. I really do not see reason why it could be incompatible with UEFI
>>> Secure Boot nor why it should not work. It just looks like improper
>>> setup from userspace.
>>
>> Ah, my fault here: there is a patch to lock down the kernel when it is
>> started with Secure Boot[1], and I believed that was already in
>> mainline, but apparently it is not.
> 
> Ok, so, this is not a problem.
> 
> I hope that such patch is not going to be part of mainline kernel as it
> would break lot of things. UEFI Secure Boot and kernel lock down are two
> different things. It would be really suspicious that for "workarounding"
> broken functionality would be needed to turn of totally unrelated option
> in firmware SETUP.
> 
>>   [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit?id=160a99536afc317b337212dd40eaba341702343e
>>
>>> It makes sense to have implemented in kernel switching between automatic
>>> and manual mode as kernel has API for it. But it depends on the fact
>>> that we know which SMM API to call. And currently it is just some random
>>> call which we somehow observed that is working on 2 machines and on some
>>> more other does not. Until we have fully working implementation we
>>> cannot put it into kernel.
>>
>> Mmh, but then what is a plausible way forward to have this? Can't we
>> start populating a whitelist for the machines we already have a solution
>> for, and add more entries when they are discovered? This would already
>> give a benefit to the users of supported laptops, without impacting
>> users of unsupported laptops. My feeling is that if we pretend to have
>> information for all possible models supported by dell-smm-hwmon, we will
>> never benefit any user. Or can you suggest a plan?
> 
> The following question is up to the hwmon maintainers. Guenter, should
> we start creating whilelist of machines which support those SMM calls
> for enabling / disabling BIOS auto mode? And maintaining this whilelist
> in kernel dell-smm-hwmon driver?
> 
Ok with me.

>>> What does not make sense for me is to have that "protection" in kernel.
>>
>> I am not really sure which "protection" you mean. I didn't mean to
>> introduce any protection from userspace in my patch, I just wanted to
>> make SET_FAN working. I think that the kernel module cannot (and should
>> not) reliably protect itself from userspace sending random IO port
>> reads/writes.
> 
> I mean protection to disallow calling SET_FAN operation when auto mode
> is enabled.
> 
I don't have a problem with that, as long as it only applies in conjunction
with the whitelist. The whitelist would determine if the pwmX_enable attribute
is supported. If it is, pwmX can only be written if pwm1_enable is set to 1
(manual fan speed control). This is pretty common for other drivers.

Guenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-15 14:46             ` Guenter Roeck
@ 2019-11-15 19:10               ` Giovanni Mascellani
  0 siblings, 0 replies; 13+ messages in thread
From: Giovanni Mascellani @ 2019-11-15 19:10 UTC (permalink / raw)
  To: Guenter Roeck, Pali Rohár; +Cc: Jean Delvare, linux-hwmon, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 516 bytes --]

Il 15/11/19 15:46, Guenter Roeck ha scritto:
> I don't have a problem with that, as long as it only applies in conjunction
> with the whitelist. The whitelist would determine if the pwmX_enable
> attribute
> is supported. If it is, pwmX can only be written if pwm1_enable is set to 1
> (manual fan speed control). This is pretty common for other drivers.

Good, I'ldd try to put together a patch ASAP.
-- 
Giovanni Mascellani <g.mascellani@gmail.com>
Postdoc researcher - Université Libre de Bruxelles


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-14 21:14 [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN Giovanni Mascellani
                   ` (2 preceding siblings ...)
  2019-11-15  8:19 ` kbuild test robot
@ 2019-11-16 15:03 ` Pali Rohár
  2019-11-16 17:14   ` Giovanni Mascellani
  3 siblings, 1 reply; 13+ messages in thread
From: Pali Rohár @ 2019-11-16 15:03 UTC (permalink / raw)
  To: Giovanni Mascellani
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel

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

On Thursday 14 November 2019 22:14:08 Giovanni Mascellani wrote:
> +#define I8K_SMM_DISABLE_BIOS	0x30a3
> +#define I8K_SMM_ENABLE_BIOS	0x31a3

In my old notes I have written these two comments:

0x30a3  disable the Fn- key combinations (except the volume functions) key and disable BIOS auto fan control (no args)
0x31a3  enable the Fn- key combinations (except the volume functions) key and enable BIOS auto fan control (no args)

So seems that these two commands have some side effect on some
machines.

For 0x34a3/0x35a3 I have only these comments:

0x34a3  disable BIOS auto fan control (no args)
0x35a3  enable BIOS auto fan control (no args)

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN
  2019-11-16 15:03 ` Pali Rohár
@ 2019-11-16 17:14   ` Giovanni Mascellani
  0 siblings, 0 replies; 13+ messages in thread
From: Giovanni Mascellani @ 2019-11-16 17:14 UTC (permalink / raw)
  To: Pali Rohár; +Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 783 bytes --]

Il 16/11/19 16:03, Pali Rohár ha scritto:
> In my old notes I have written these two comments:
> 
> 0x30a3  disable the Fn- key combinations (except the volume functions) key and disable BIOS auto fan control (no args)
> 0x31a3  enable the Fn- key combinations (except the volume functions) key and enable BIOS auto fan control (no args)

This does not seem to happen on my laptop: both command pairs set and
unset auto fan control, without other apparent effects. However, I have
dropped 0x30a3 and 0x31a3, since 0x34a3 and 0x35a3 are known to work on
all laptops where at least one command pair is known to work, and there
are no known side effects.

Giovanni.
-- 
Giovanni Mascellani <g.mascellani@gmail.com>
Postdoc researcher - Université Libre de Bruxelles


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-11-16 17:14 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-14 21:14 [PATCH] hwmon: (dell-smm-hwmon) Disable BIOS fan control on SET_FAN Giovanni Mascellani
2019-11-14 21:39 ` Guenter Roeck
2019-11-14 21:51   ` Pali Rohár
2019-11-15  9:51     ` Giovanni Mascellani
2019-11-15 11:29       ` Pali Rohár
2019-11-15 13:44         ` Giovanni Mascellani
2019-11-15 14:36           ` Pali Rohár
2019-11-15 14:46             ` Guenter Roeck
2019-11-15 19:10               ` Giovanni Mascellani
2019-11-15  7:12 ` kbuild test robot
2019-11-15  8:19 ` kbuild test robot
2019-11-16 15:03 ` Pali Rohár
2019-11-16 17:14   ` Giovanni Mascellani

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).