| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3a10f96a-06e1-39f4-74a6-908d25b1f496@roeck-us.net>
Date: Sat, 16 Nov 2019 14:08:53 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Giovanni Mascellani <gio@...ian.org>,
Pali Rohár <pali.rohar@...il.com>,
Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] dell-smm-hwmon: Add support for disabling automatic
BIOS fan control
On 11/16/19 9:36 AM, Giovanni Mascellani wrote:
> This patch exports standard hwmon pwmX_enable sysfs attribute for
> enabling or disabling automatic fan control by BIOS. Standard value
> "1" is for disabling automatic BIOS fan control and value "2" for
> enabling.
>
> By default BIOS auto mode is enabled by laptop firmware.
>
> When BIOS auto mode is enabled, custom fan speed value (set via hwmon
> pwmX sysfs attribute) is overwritten by SMM in few seconds and
> therefore any custom settings are without effect. So this is reason
> why implementing option for disabling BIOS auto mode is needed.
>
> So finally this patch allows kernel to set and control fan speed on
> laptops, but it can be dangerous (like setting speed of other fans).
>
> The SMM commands to enable or disable automatic fan control are not
> documented and are not the same on all Dell laptops. Therefore a
> whitelist is used to send the correct codes only on laptopts for which
> they are known.
>
> This patch was originally developed by Pali Rohár; later Giovanni
> Mascellani implemented the whitelist.
>
> Signed-off-by: Giovanni Mascellani <gio@...ian.org>
> Co-Developer-by: Pali Rohár <pali.rohar@...il.com>
> ---
> drivers/hwmon/dell-smm-hwmon.c | 119 ++++++++++++++++++++++++++++++---
> 1 file changed, 109 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 4212d022d253..87f88896cc79 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -68,6 +68,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 unsigned int manual_fan;
> +static unsigned int auto_fan;
>
> #define I8K_HWMON_HAVE_TEMP1 (1 << 0)
> #define I8K_HWMON_HAVE_TEMP2 (1 << 1)
> @@ -300,6 +302,20 @@ static int i8k_get_fan_nominal_speed(int fan, int speed)
> return i8k_smm(®s) ? : (regs.eax & 0xffff) * i8k_fan_mult;
> }
>
> +/*
> + * Enable or disable automatic BIOS fan control support
> + */
> +static int i8k_enable_fan_auto_mode(bool enable)
> +{
> + struct smm_regs regs = { };
> +
> + if (disallow_fan_support)
> + return -EINVAL;
> +
> + regs.eax = enable ? auto_fan : manual_fan;
> + return i8k_smm(®s);
> +}
> +
> /*
> * Set the fan speed (off, low, high). Returns the new fan status.
> */
> @@ -726,6 +742,35 @@ static ssize_t i8k_hwmon_pwm_store(struct device *dev,
> return err < 0 ? -EIO : count;
> }
>
> +static ssize_t i8k_hwmon_pwm_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int err;
> + bool enable;
> + unsigned long val;
> +
> + if (!auto_fan)
> + return -ENODEV;
> +
> + err = kstrtoul(buf, 10, &val);
> + if (err)
> + return err;
> +
> + if (val == 1)
> + enable = false;
> + else if (val == 2)
> + enable = true;
> + else
> + return -EINVAL;
> +
> + mutex_lock(&i8k_mutex);
> + err = i8k_enable_fan_auto_mode(enable);
> + mutex_unlock(&i8k_mutex);
> +
> + return err ? -EIO : count;
Why override the error code ? i8k_enable_fan_auto_mode()
can return -EINVAL.
I can see that the rest of the driver has the same bad habit,
but that is not a reason to continue it.
> +}
> +
> static SENSOR_DEVICE_ATTR_RO(temp1_input, i8k_hwmon_temp, 0);
> static SENSOR_DEVICE_ATTR_RO(temp1_label, i8k_hwmon_temp_label, 0);
> static SENSOR_DEVICE_ATTR_RO(temp2_input, i8k_hwmon_temp, 1);
> @@ -749,12 +794,15 @@ static SENSOR_DEVICE_ATTR_RO(temp10_label, i8k_hwmon_temp_label, 9);
> static SENSOR_DEVICE_ATTR_RO(fan1_input, i8k_hwmon_fan, 0);
> static SENSOR_DEVICE_ATTR_RO(fan1_label, i8k_hwmon_fan_label, 0);
> static SENSOR_DEVICE_ATTR_RW(pwm1, i8k_hwmon_pwm, 0);
> +static SENSOR_DEVICE_ATTR_WO(pwm1_enable, i8k_hwmon_pwm_enable, 0);
> static SENSOR_DEVICE_ATTR_RO(fan2_input, i8k_hwmon_fan, 1);
> static SENSOR_DEVICE_ATTR_RO(fan2_label, i8k_hwmon_fan_label, 1);
> static SENSOR_DEVICE_ATTR_RW(pwm2, i8k_hwmon_pwm, 1);
> +static SENSOR_DEVICE_ATTR_WO(pwm2_enable, i8k_hwmon_pwm_enable, 0);
> static SENSOR_DEVICE_ATTR_RO(fan3_input, i8k_hwmon_fan, 2);
> static SENSOR_DEVICE_ATTR_RO(fan3_label, i8k_hwmon_fan_label, 2);
> static SENSOR_DEVICE_ATTR_RW(pwm3, i8k_hwmon_pwm, 2);
> +static SENSOR_DEVICE_ATTR_WO(pwm3_enable, i8k_hwmon_pwm_enable, 0);
Having three attributes do all the same is not very valuable.
I would suggest to stick with pwm1_enable and document that it applies
to all pwm channels.
>
> static struct attribute *i8k_attrs[] = {
> &sensor_dev_attr_temp1_input.dev_attr.attr, /* 0 */
> @@ -780,12 +828,15 @@ static struct attribute *i8k_attrs[] = {
> &sensor_dev_attr_fan1_input.dev_attr.attr, /* 20 */
> &sensor_dev_attr_fan1_label.dev_attr.attr, /* 21 */
> &sensor_dev_attr_pwm1.dev_attr.attr, /* 22 */
> - &sensor_dev_attr_fan2_input.dev_attr.attr, /* 23 */
> - &sensor_dev_attr_fan2_label.dev_attr.attr, /* 24 */
> - &sensor_dev_attr_pwm2.dev_attr.attr, /* 25 */
> - &sensor_dev_attr_fan3_input.dev_attr.attr, /* 26 */
> - &sensor_dev_attr_fan3_label.dev_attr.attr, /* 27 */
> - &sensor_dev_attr_pwm3.dev_attr.attr, /* 28 */
> + &sensor_dev_attr_pwm1_enable.dev_attr.attr, /* 23 */
> + &sensor_dev_attr_fan2_input.dev_attr.attr, /* 24 */
> + &sensor_dev_attr_fan2_label.dev_attr.attr, /* 25 */
> + &sensor_dev_attr_pwm2.dev_attr.attr, /* 26 */
> + &sensor_dev_attr_pwm2_enable.dev_attr.attr, /* 27 */
> + &sensor_dev_attr_fan3_input.dev_attr.attr, /* 28 */
> + &sensor_dev_attr_fan3_label.dev_attr.attr, /* 29 */
> + &sensor_dev_attr_pwm3.dev_attr.attr, /* 30 */
> + &sensor_dev_attr_pwm3_enable.dev_attr.attr, /* 31 */
> NULL
> };
>
> @@ -828,16 +879,20 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
> return 0;
>
> - if (index >= 20 && index <= 22 &&
> + if (index >= 20 && index <= 23 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
> return 0;
> - if (index >= 23 && index <= 25 &&
> + if (index >= 24 && index <= 27 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
> return 0;
> - if (index >= 26 && index <= 28 &&
> + if (index >= 28 && index <= 31 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
> return 0;
>
> + if ((index == 23 || index == 27 || index == 31) &&
> + !auto_fan)
> + return 0;
> +
> return attr->mode;
> }
>
> @@ -1135,12 +1190,48 @@ static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = {
> { }
> };
>
> +struct i8k_fan_control_data {
> + unsigned int manual_fan;
> + unsigned int auto_fan;
> +};
> +
> +enum i8k_fan_controls {
> + I8K_FAN_34A3_35A3,
> +};
> +
> +static const struct i8k_fan_control_data i8k_fan_control_data[] = {
> + [I8K_FAN_34A3_35A3] = {
> + .manual_fan = 0x34a3,
> + .auto_fan = 0x35a3,
> + },
> +};
> +
> +static struct dmi_system_id i8k_whitelist_fan_control[] __initdata = {
> + {
> + .ident = "Dell Precision 5530",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Precision 5530"),
> + },
> + .driver_data = (void *)&i8k_fan_control_data[I8K_FAN_34A3_35A3],
> + },
> + {
> + .ident = "Dell Latitude E6440",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> + },
> + .driver_data = (void *)&i8k_fan_control_data[I8K_FAN_34A3_35A3],
> + },
> + { }
> +};
> +
> /*
> * Probe for the presence of a supported laptop.
> */
> static int __init i8k_probe(void)
> {
> - const struct dmi_system_id *id;
> + const struct dmi_system_id *id, *fan_control;
> int fan, ret;
>
> /*
> @@ -1200,6 +1291,14 @@ static int __init i8k_probe(void)
> i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be 0 */
> i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
>
> + fan_control = dmi_first_match(i8k_whitelist_fan_control);
> + if (fan_control && fan_control->driver_data) {
> + const struct i8k_fan_control_data *fan_control_data = fan_control->driver_data;
> + manual_fan = fan_control_data->manual_fan;
> + auto_fan = fan_control_data->auto_fan;
> + pr_info("enabling experimental BIOS fan control support\n");
That isn't entirely accurate. What this enables is the ability
to select automatic or manual fan control.
> + }
> +
> if (!fan_mult) {
> /*
> * Autodetect fan multiplier based on nominal rpm
>
Powered by blists - more mailing lists