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

[Pali RohÃr]

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?

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@xxxxxxxxx>
> > ---
> > 
> >   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 */

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.