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 <[email protected]>
---
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(®s) ? : 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(®s) ? : 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
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 <[email protected]>
> ---
> 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(®s) ? : 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(®s) ? : 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
>
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
[email protected]
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 <[email protected]>
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/[email protected] Intel Corporation
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 <[email protected]>
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/[email protected] Intel Corporation
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 <[email protected]>
Postdoc researcher - Université Libre de Bruxelles
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
[email protected]
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 <[email protected]>
Postdoc researcher - Université Libre de Bruxelles
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
[email protected]
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
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 <[email protected]>
Postdoc researcher - Université Libre de Bruxelles
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
[email protected]
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 <[email protected]>
Postdoc researcher - Université Libre de Bruxelles