[PATCH] hwmon: (dell-smm) Show fan_max param in sysfs

[PATCH] hwmon: (dell-smm) Show fan_max param in sysfs

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

For allowing tools like i8kutils to query the fan state
without having to rely on the deprecated /proc/i8k interface,
they need to scale the pwm values (0 - 255) back to the
real hardware values (0 - 2/3).
Show fan_max in sysfs to allow for such a scenario.

Tested on a Dell Latitude C600.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 774c1b0715d9..6d3fd4f0f99d 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0);
 MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)");

 static uint fan_max;
-module_param(fan_max, uint, 0);
+module_param(fan_max, uint, 0444);
 MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");

 struct smm_regs {
@@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 			fan_max = conf->fan_max;
 	}

-	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
+	if (!fan_max)	/* Must not be 0*/
+		fan_max = I8K_FAN_HIGH;
+
+	data->i8k_fan_max = fan_max;
 	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);

 	fan_control = dmi_first_match(i8k_whitelist_fan_control);
--
2.20.1

Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs

[Pali Rohár]

On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> For allowing tools like i8kutils to query the fan state
> without having to rely on the deprecated /proc/i8k interface,
> they need to scale the pwm values (0 - 255) back to the
> real hardware values (0 - 2/3).
> Show fan_max in sysfs to allow for such a scenario.

Guenter, I think that this is general problem and not specific to
dell-smm-hwmon.c driver and i8kutils.

All other hwmon tools should have similar problem. If e.g. GUI tool has
slider for controlling pwm then such tool needs to know number of steps.
Otherwise usage of such slider would be not very user friendly...

Currently in hwmon sysfs API there is not attribute which could export
this kind of information.

What about e.g. introducing 'pwm_steps' attribute which would provide
this information?

> Tested on a Dell Latitude C600.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 774c1b0715d9..6d3fd4f0f99d 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0);
>  MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)");
> 
>  static uint fan_max;
> -module_param(fan_max, uint, 0);
> +module_param(fan_max, uint, 0444);
>  MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
> 
>  struct smm_regs {
> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  			fan_max = conf->fan_max;
>  	}
> 
> -	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
> +	if (!fan_max)	/* Must not be 0*/
> +		fan_max = I8K_FAN_HIGH;
> +
> +	data->i8k_fan_max = fan_max;
>  	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
> 
>  	fan_control = dmi_first_match(i8k_whitelist_fan_control);
> --
> 2.20.1
> 

Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs

[Guenter Roeck]

On 9/3/21 7:49 AM, Pali Rohár wrote:
> On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> For allowing tools like i8kutils to query the fan state
>> without having to rely on the deprecated /proc/i8k interface,
>> they need to scale the pwm values (0 - 255) back to the
>> real hardware values (0 - 2/3).
>> Show fan_max in sysfs to allow for such a scenario.
> 
> Guenter, I think that this is general problem and not specific to
> dell-smm-hwmon.c driver and i8kutils.
> 
> All other hwmon tools should have similar problem. If e.g. GUI tool has
> slider for controlling pwm then such tool needs to know number of steps.
> Otherwise usage of such slider would be not very user friendly...
> 
> Currently in hwmon sysfs API there is not attribute which could export
> this kind of information.
> 
> What about e.g. introducing 'pwm_steps' attribute which would provide
> this information?
> 

It isn't really a problem. The problem is that the tool writers insist
not following the ABI. All they would have to do is to use a scale of 0..255,
read back any written pwm value, and adjust the displayed value based
on the returned value. I do not see why this would be "not user friendly".

An attribute such as pwm_steps would not work because the step size can be
non-linear.

Guenter

>> Tested on a Dell Latitude C600.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 774c1b0715d9..6d3fd4f0f99d 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0);
>>   MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with (default: autodetect)");
>>
>>   static uint fan_max;
>> -module_param(fan_max, uint, 0);
>> +module_param(fan_max, uint, 0444);
>>   MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed (default: autodetect)");
>>
>>   struct smm_regs {
>> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>>   			fan_max = conf->fan_max;
>>   	}
>>
>> -	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>> +	if (!fan_max)	/* Must not be 0*/
>> +		fan_max = I8K_FAN_HIGH;
>> +
>> +	data->i8k_fan_max = fan_max;
>>   	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
>>
>>   	fan_control = dmi_first_match(i8k_whitelist_fan_control);
>> --
>> 2.20.1
>>

Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs

[Armin Wolf]

Am 03.09.21 um 16:59 schrieb Guenter Roeck:
> On 9/3/21 7:49 AM, Pali Rohár wrote:
>> On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote:
>>> From: Armin Wolf <W_Armin@gmx.de>
>>>
>>> For allowing tools like i8kutils to query the fan state
>>> without having to rely on the deprecated /proc/i8k interface,
>>> they need to scale the pwm values (0 - 255) back to the
>>> real hardware values (0 - 2/3).
>>> Show fan_max in sysfs to allow for such a scenario.
>>
>> Guenter, I think that this is general problem and not specific to
>> dell-smm-hwmon.c driver and i8kutils.
>>
>> All other hwmon tools should have similar problem. If e.g. GUI tool has
>> slider for controlling pwm then such tool needs to know number of steps.
>> Otherwise usage of such slider would be not very user friendly...
>>
>> Currently in hwmon sysfs API there is not attribute which could export
>> this kind of information.
>>
>> What about e.g. introducing 'pwm_steps' attribute which would provide
>> this information?
>>
>
> It isn't really a problem. The problem is that the tool writers insist
> not following the ABI. All they would have to do is to use a scale of
> 0..255,
> read back any written pwm value, and adjust the displayed value based
> on the returned value. I do not see why this would be "not user
> friendly".
>
> An attribute such as pwm_steps would not work because the step size
> can be
> non-linear.
>
> Guenter
>
i8kutils uses values from 0 to 2/3 for historic reasons, as the
deprecated /proc/i8k interface
provided such values. In order to not break backwards compatibility,
they cannot simply
switch to standard pwm values (ithink).
I believe that showing fan_max via sysfs would allow i8kutils to finally
use the standard interface,
since when this happens, we maybe can remove the /proc/i8k interface.
>>> Tested on a Dell Latitude C600.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>> b/drivers/hwmon/dell-smm-hwmon.c
>>> index 774c1b0715d9..6d3fd4f0f99d 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0);
>>>   MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with
>>> (default: autodetect)");
>>>
>>>   static uint fan_max;
>>> -module_param(fan_max, uint, 0);
>>> +module_param(fan_max, uint, 0444);
>>>   MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed
>>> (default: autodetect)");
>>>
>>>   struct smm_regs {
>>> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct
>>> platform_device *pdev)
>>>               fan_max = conf->fan_max;
>>>       }
>>>
>>> -    data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;    /* Must not be
>>> 0 */
>>> +    if (!fan_max)    /* Must not be 0*/
>>> +        fan_max = I8K_FAN_HIGH;
>>> +
>>> +    data->i8k_fan_max = fan_max;
>>>       data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
>>>
>>>       fan_control = dmi_first_match(i8k_whitelist_fan_control);
>>> --
>>> 2.20.1
>>>
>

Re: [PATCH] hwmon: (dell-smm) Show fan_max param in sysfs

[Guenter Roeck]

On 9/3/21 8:40 AM, Armin Wolf wrote:
> Am 03.09.21 um 16:59 schrieb Guenter Roeck:
>> On 9/3/21 7:49 AM, Pali Rohár wrote:
>>> On Friday 03 September 2021 16:24:56 W_Armin@gmx.de wrote:
>>>> From: Armin Wolf <W_Armin@gmx.de>
>>>>
>>>> For allowing tools like i8kutils to query the fan state
>>>> without having to rely on the deprecated /proc/i8k interface,
>>>> they need to scale the pwm values (0 - 255) back to the
>>>> real hardware values (0 - 2/3).
>>>> Show fan_max in sysfs to allow for such a scenario.
>>>
>>> Guenter, I think that this is general problem and not specific to
>>> dell-smm-hwmon.c driver and i8kutils.
>>>
>>> All other hwmon tools should have similar problem. If e.g. GUI tool has
>>> slider for controlling pwm then such tool needs to know number of steps.
>>> Otherwise usage of such slider would be not very user friendly...
>>>
>>> Currently in hwmon sysfs API there is not attribute which could export
>>> this kind of information.
>>>
>>> What about e.g. introducing 'pwm_steps' attribute which would provide
>>> this information?
>>>
>>
>> It isn't really a problem. The problem is that the tool writers insist
>> not following the ABI. All they would have to do is to use a scale of
>> 0..255,
>> read back any written pwm value, and adjust the displayed value based
>> on the returned value. I do not see why this would be "not user
>> friendly".
>>
>> An attribute such as pwm_steps would not work because the step size
>> can be
>> non-linear.
>>
>> Guenter
>>
> i8kutils uses values from 0 to 2/3 for historic reasons, as the
> deprecated /proc/i8k interface
> provided such values. In order to not break backwards compatibility,
> they cannot simply
> switch to standard pwm values (ithink).
> I believe that showing fan_max via sysfs would allow i8kutils to finally
> use the standard interface,
> since when this happens, we maybe can remove the /proc/i8k interface.

No, we can't, because that is an ABI, and we can not remove it
because someone might insist using an old version of i8kutils with
a new kernel. Also, fan_max is _not_ a 'standard' interface.
If i8kutils wants to use fan_max, it can get it from the existing
proc interface.

Besides, fan_max is repurposed here: So far it is a module parameter.
With the proposed changes in place, it would no longer be a module
parameter but a sysfs attribute disguised as module parameter
that is abused to report a value (one changed by the driver)
to userspace.

Guenter

>>>> Tested on a Dell Latitude C600.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>>   drivers/hwmon/dell-smm-hwmon.c | 7 +++++--
>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>>> b/drivers/hwmon/dell-smm-hwmon.c
>>>> index 774c1b0715d9..6d3fd4f0f99d 100644
>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>> @@ -107,7 +107,7 @@ module_param(fan_mult, uint, 0);
>>>>   MODULE_PARM_DESC(fan_mult, "Factor to multiply fan speed with
>>>> (default: autodetect)");
>>>>
>>>>   static uint fan_max;
>>>> -module_param(fan_max, uint, 0);
>>>> +module_param(fan_max, uint, 0444);
>>>>   MODULE_PARM_DESC(fan_max, "Maximum configurable fan speed
>>>> (default: autodetect)");
>>>>
>>>>   struct smm_regs {
>>>> @@ -1245,7 +1245,10 @@ static int __init dell_smm_probe(struct
>>>> platform_device *pdev)
>>>>               fan_max = conf->fan_max;
>>>>       }
>>>>
>>>> -    data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;    /* Must not be
>>>> 0 */
>>>> +    if (!fan_max)    /* Must not be 0*/
>>>> +        fan_max = I8K_FAN_HIGH;
>>>> +
>>>> +    data->i8k_fan_max = fan_max;
>>>>       data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);
>>>>
>>>>       fan_control = dmi_first_match(i8k_whitelist_fan_control);
>>>> -- 
>>>> 2.20.1
>>>>
>>