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

[Guenter Roeck]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/