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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 17 Nov 2019 09:02:08 +0100
From:   Giovanni Mascellani <gio@...ian.org>
To:     Guenter Roeck <linux@...ck-us.net>,
        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

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@...il.com>
Postdoc researcher - Université Libre de Bruxelles



Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)