[PATCH v3] dell-smm-hwmon: Add support for disabling automatic BIOS fan control

[PATCH v3] dell-smm-hwmon: Add support for disabling automatic BIOS fan control

[Giovanni Mascellani]

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>
---
 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(&regs) ? : (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(&regs);
+}
+
 /*
  * 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;
+}
+
 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);
 
 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");
+	}
+
 	if (!fan_mult) {
 		/*
 		 * Autodetect fan multiplier based on nominal rpm
-- 
2.24.0

Re: [PATCH v3] dell-smm-hwmon: Add support for disabling automatic BIOS fan control

[Guenter Roeck]

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@debian.org>
> Co-Developer-by: Pali Rohár <pali.rohar@gmail.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(&regs) ? : (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(&regs);
> +}
> +
>   /*
>    * 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
> 

Re: [PATCH v3] dell-smm-hwmon: Add support for disabling automatic BIOS fan control

[Giovanni Mascellani]

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

Hi,

Il 16/11/19 23:08, Guenter Roeck ha scritto:
>> +    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.

Ok, I thought it was the appropriate thing to do just because it was
done elsewhere. If it's not a good idea, do you think a patch removing
the other instances of this construct would be appropriate?

>> +}
>> +
>>   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.

I had no idea what is the convention here. No problem changing this thing.

>> @@ -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.

Hmm, it sounds right to me: there is a feature which is "BIOS fan
control" and this driver can "support" it or not, i.e., be aware of it
and interact with it or not. And all of this is "experimental". The
wording seems to capture this to me. However, no problem with changing
it. How would "enabling support for setting automatic/manual fan
control" work? Can you suggest a wording?

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 --]

Re: [PATCH v3] dell-smm-hwmon: Add support for disabling automatic BIOS fan control

[Guenter Roeck]

On 11/17/19 12:02 AM, Giovanni Mascellani wrote:
> Hi,
> 
> Il 16/11/19 23:08, Guenter Roeck ha scritto:
>>> +    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.
> 
> Ok, I thought it was the appropriate thing to do just because it was
> done elsewhere. If it's not a good idea, do you think a patch removing
> the other instances of this construct would be appropriate?
> 

In general it is never a good idea to override error codes. "I have seen
it elsewhere" is vener a good argument - you'll find examples for everything
in the Linux kernel.

As for fixing up the other instances in this driver, sure, if you feel like
it, but that would have to be a separate patch.

>>> +}
>>> +
>>>    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.
> 
> I had no idea what is the convention here. No problem changing this thing.
> 
>>> @@ -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.
> 
> Hmm, it sounds right to me: there is a feature which is "BIOS fan
> control" and this driver can "support" it or not, i.e., be aware of it
> and interact with it or not. And all of this is "experimental". The

"experimental" is fine, and I understand that those involved in this
exchange are aware what the message means. It does, however, not help
others not involved.

> wording seems to capture this to me. However, no problem with changing
> it. How would "enabling support for setting automatic/manual fan
> control" work? Can you suggest a wording?
> 

"enabling experimental support for ... " sounds good to me.

Thanks,
Guenter

> Thanks, Giovanni.
>