Re: [PATCH] i8k: Ignore temperature sensors which report invalid values

[Guenter Roeck <linux@roeck-us.net>]

On 10/20/2014 09:46 AM, Pali Rohár wrote:
> Ok, I will describe my problem. Guenter, maybe you can find
> another solution/fix for it.
>
> Calling i8k_get_temp(3) on my laptop without I8K_TEMPERATURE_BUG
> always returns value 193 (which is above I8K_MAX_TEMP).
>
> When I8K_TEMPERATURE_BUG is enabled (by default) then
> i8k_get_temp(3) returns value from prev[3] and store new value
> I8K_TEMPERATURE_BUG to prev[3]. Value in prev[3] is initialized
> to 0.
>
> What I want to achieve is: when i8k_get_temp() for particular
> sensor id always returns invalid value (> I8K_MAX_TEMP) then we
> should totally ignore sensor with that id and do not export it
> via hwmon.
>
> My solution is: initialize prev[id] to I8K_MAX_TEMP, so on
> invalid data first call to i8k_get_temp(id) returns I8K_MAX_TEMP.
> Then in i8k_init_hwmon check if value is < I8K_MAX_TEMP and if
> not ignore sensor id.
>
> Guenter, it is clear now? Are you ok that we should ignore sensor
> if always report value above I8K_MAX_TEMP? If you do not like my
> solution/patch for it, can you specify how other can it be fixed?
>

I still don't see the point in initializing prev[].

Yes, I am ok with ignoring sensor values if the reported temperature
is above I8K_MAX_TEMP. I am just not sure if we should check against
I8K_MAX_TEMP or against, say, 192. Reason is that we do know that
the sensor can erroneously return 0x99 on some systems once in a
while. We would not want to ignore those sensors just because they
happen to report 0x99 during initialization.

So maybe make it
	if (err >= 0 && err < 192)
and add a note before the first if(), explaining that higher values
suggest that there is no sensor attached.

Thanks,
Guenter

> On Sunday 19 October 2014 17:13:29 Guenter Roeck wrote:
>> On 10/19/2014 07:46 AM, Pali Rohár wrote:
>>> On some machines some temperature sensors report too high
>>> values which are
>>
>> What is "too high", and what is "some machines" ?
>> Would be great if you can be more specific.
>>
>>> invalid. When value is invalid function i8k_get_temp()
>>> returns previous value and at next call it returns
>>> I8K_MAX_TEMP.
>>>
>>> With this patch also firt i8k_get_temp() call returns
>>> I8K_MAX_TEMP and
>>
>> fix ? Also, I am not entirely sure I understand what exactly
>> you are fixing here.
>>
>>> i8k_init_hwmon() will ignore all sensor ids which report
>>> incorrect values.
>>>
>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>> ---
>>>
>>>    drivers/char/i8k.c |   10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
>>> index 7272b08..bc327fa 100644
>>> --- a/drivers/char/i8k.c
>>> +++ b/drivers/char/i8k.c
>>> @@ -298,7 +298,7 @@ static int i8k_get_temp(int sensor)
>>>
>>>    	int temp;
>>>
>>>    #ifdef I8K_TEMPERATURE_BUG
>>>
>>> -	static int prev[4];
>>> +	static int prev[4] = { I8K_MAX_TEMP, I8K_MAX_TEMP,
>>> I8K_MAX_TEMP, I8K_MAX_TEMP };
>>
>> I am not sure I understand what this change is expected to
>> accomplish. Please explain.
>>
>>>    #endif
>>>
>>>    	regs.ebx = sensor & 0xff;
>>>    	rc = i8k_smm(&regs);
>>>
>>> @@ -610,17 +610,17 @@ static int __init i8k_init_hwmon(void)
>>>
>>>    	/* CPU temperature attributes, if temperature reading is
>>>    	OK */ err = i8k_get_temp(0);
>>>
>>> -	if (err >= 0)
>>> +	if (err >= 0 && err < I8K_MAX_TEMP)
>>
>> I8K_MAX_TEMP is, at least in theory, a valid temperature, so
>> this should be "<=".
>>
>> It would be important to understand what the "too high"
>> temperature is to possibly be able to distinguish it from the
>> buggy temperature that the code is trying to fix.
>>
>> Thanks,
>> Guenter
>>
>>>    		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
>>>    	
>>>    	/* check for additional temperature sensors */
>>>    	err = i8k_get_temp(1);
>>>
>>> -	if (err >= 0)
>>> +	if (err >= 0 && err < I8K_MAX_TEMP)
>>>
>>>    		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
>>>    	
>>>    	err = i8k_get_temp(2);
>>>
>>> -	if (err >= 0)
>>> +	if (err >= 0 && err < I8K_MAX_TEMP)
>>>
>>>    		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
>>>    	
>>>    	err = i8k_get_temp(3);
>>>
>>> -	if (err >= 0)
>>> +	if (err >= 0 && err < I8K_MAX_TEMP)
>>>
>>>    		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
>>>    	
>>>    	/* Left fan attributes, if left fan is present */
>