Messages in this thread | | | Subject | Re: [PATCH v2] dell-smm-hwmon: Add support for disabling automatic BIOS fan control | From | Guenter Roeck <> | Date | Sat, 16 Nov 2019 07:53:10 -0800 |
| |
On 11/16/19 6:58 AM, Pali Rohár wrote: > On Saturday 16 November 2019 15:06:49 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@debian.org> >> Co-Developer-by: Pali Rohár <pali.rohar@gmail.com> > > Hello! Patch looks good, see small suggestions below. The only important > change for this patch is to hide those new pwmX_enable entries on > unsupported machines. > >> --- >> drivers/hwmon/dell-smm-hwmon.c | 111 ++++++++++++++++++++++++++++++--- >> 1 file changed, 101 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c >> index 4212d022d253..67b8c0adede8 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 smm_manual_fan; >> +static unsigned int smm_auto_fan;
The "smm_" prefix does not have any value here.
>> >> #define I8K_HWMON_HAVE_TEMP1 (1 << 0) >> #define I8K_HWMON_HAVE_TEMP2 (1 << 1) >> @@ -300,6 +302,17 @@ 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 = { }; >> + > > For safely reasons I would suggest to add: > > if (disallow_fan_support) > return -EINVAL; > >> + regs.eax = enable ? smm_auto_fan : smm_manual_fan; >> + return i8k_smm(®s); >> +} >> + >> /* >> * Set the fan speed (off, low, high). Returns the new fan status. >> */ >> @@ -726,6 +739,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 (!smm_auto_fan) >> + return -ENODEV; >> + >> + err = kstrtoul(buf, 10, &val); >> + if (err) >> + return err; >> + > > ==== > >> + if (val == 0) >> + return -EINVAL; >> + if (val > 2) >> + return -EINVAL; >> + >> + enable = (val != 1); > > ==== > > Just small suggestion: I would write it more-opencoded to immediately > see what values are valid and what are they meaning. It look me quite > more time to check that above logic is correct (according to hwmon > documentation). > > if (val == 1) > enable = false; > else if (val == 2) > enable = true; > else > return -EINVAL; > > or via switch: > > switch (val) { > case 1: > enable = false; > break; > case 2: > enable = true; > break; > default: > return -EINVAL; > } > >> + >> + mutex_lock(&i8k_mutex); >> + err = i8k_enable_fan_auto_mode(enable); >> + mutex_unlock(&i8k_mutex); >> + >> + return err ? -EIO : count; >> +} >> + >> 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 +791,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); >> >> static struct attribute *i8k_attrs[] = { >> &sensor_dev_attr_temp1_input.dev_attr.attr, /* 0 */ >> @@ -780,12 +825,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,13 +876,13 @@ 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; > > > Indexes 23, 27 and 31 should not be visible when "smm_auto_fan" is not > available. Please add check for this. > >> >> @@ -1135,12 +1183,48 @@ static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = { >> { } >> }; >> >> +struct i8k_manual_fan_data { >> + unsigned int smm_manual_fan; >> + unsigned int smm_auto_fan; >> +}; > > Just cosmetic suggestion: As this structure contains data for both > manual and automatic mode I would not use "manual" in name. But e.g. > something like "i8k_bios_fan_control_data"... > Or i8k_fan_control_data. Also, "manual" and "auto" for the variable names would be sufficient.
>> + >> +enum i8k_manual_fans { >> + I8K_FAN_34A3_35A3, >> +}; >> + >> +static const struct i8k_manual_fan_data i8k_manual_fan_data[] = { >> + [I8K_FAN_34A3_35A3] = { >> + .smm_manual_fan = 0x34a3, >> + .smm_auto_fan = 0x35a3, >> + }, >> +}; >> + >> +static struct dmi_system_id i8k_whitelist_manual_fan[] __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_manual_fan_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_manual_fan_data[I8K_FAN_34A3_35A3], >> + }, >> + { } >> +}; >> +
Drop the "manual" from the variable names. maybe use "fan_control" instead of "manual_fan".
>> /* >> * 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, *manual_fan; >> int fan, ret; >> >> /* >> @@ -1200,6 +1284,13 @@ 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); >> >> + manual_fan = dmi_first_match(i8k_whitelist_manual_fan); >> + if (manual_fan && manual_fan->driver_data) { >> + const struct i8k_manual_fan_data *manual_fan_data = manual_fan->driver_data; >> + smm_manual_fan = manual_fan_data->smm_manual_fan; >> + smm_auto_fan = manual_fan_data->smm_auto_fan; > > As this feature is experimental, I would suggest to add some pr_info > message that experimental BIOS fan control support is exported via hwmon > interface. It may happen that it would not work for some people and for > debugging purposes it would be easier to spot problem if dmesg provides > such message. > >> + } >> + >> if (!fan_mult) { >> /* >> * Autodetect fan multiplier based on nominal rpm >
| |