Re: [PATCH 4/4] Return the fan speed via sysfs

[Goffredo Baroncelli <kreijack@gmail.com>]

On 08/03/2014 05:59 PM, Jean Delvare wrote:
> On Sun, 03 Aug 2014 17:27:17 +0200, Goffredo Baroncelli wrote:
>> On 08/03/2014 04:17 PM, Jean Delvare wrote:
>>> On Fri,  1 Aug 2014 14:00:50 +0000, Goffredo Baroncelli wrote:
>>>> Return the fan speed via sysfs:
>>>> /sys/devices/temperature/fan_level
>>>
>>> Good idea. Even better would be if the driver would expose a standard
>>> hwmon interface for the temperature values. Fan level could go in
>>> attribute "pwm1" after proper scaling.
>>
>> I tought the same. But until now the value logged is an integer value
>> between 1 and 11. So I preferred to leave it as is.
>>
>> The thing that I can do is to *add* a further attribute called pwm1.
>> ( I have to check how adm1031.c computes its pwm), because is a 
>> more standard interface.
> 
> The temperature attributes in hwmon would need different names and
> units too (temp1_input and temp2_input, in millidegree C.) The
> advantage is that all monitoring applications out there would pick up
> these values automatically.

Are you suggesting to add also a "temp1_input" attribute ?

> 
>> I even thought to allow to change the fan speed from user space....
> 
> Ben will never let you do that ;-)

What would be the risk ?. When the CPU temperature goes behind the limit, 
then the computer is switched off by an hardware protection (I am sure because
I had to changed a cpu board because a drift of the temperature sensor).

I am not suggesting to allow to change the fan speed, I am only asking which would
be the risk.

> 
>>>> @@ -265,6 +271,7 @@ setup_hardware( void )
>>>>  
>>>>  	err = device_create_file( &x.of_dev->dev, &dev_attr_cpu_temperature );
>>>>  	err |= device_create_file( &x.of_dev->dev, &dev_attr_case_temperature );
>>>> +	err |= device_create_file( &x.of_dev->dev, &dev_attr_fan_level );
>>>>  	if (err)
>>>>  		printk(KERN_WARNING
>>>>  			"Failed to create temperature attribute file(s).\n");
>>>
>>> That's not your fault but please note that this construct is broken.
>>> You can't "or" error codes together, the result if two or more calls
>>> fail with different error codes is pretty random. Instead, each error
>>> must be tested individually. I know checkpatch.pl will complain if you
>>> do that but you can ignore it as is it the right thing to do in that
>>> case.
>>
>> The really question is what we should do if the 2nd device_create_file()
>> would fail: we should fails the driver initialization ? or we should
>> continue, because even if there aren't some sysfs attributes the driver
>> work enough ?
> 
> I would log a warning and continue because it's a secondary feature of
> the driver. Exactly as the driver already does - no change here.
> 
> In practice it's never going to happen so it doesn't really matter. I
> just wanted to point out that the construct used in the driver was
> dangerous. In this specific case it's harmless because the value of
> "err" is never used (other than the fact that it's non-zero.) But if
> the error code was included in the log message for example (which is
> recommended), it would possibly make no sense.
> 
> Feel free to ignore this problem in this patch, it's not so important.
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5