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

[Pali Rohár]

On some machines some temperature sensors report too high values which =
are
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
i8k_init_hwmon() will ignore all sensor ids which report incorrect valu=
es.

Signed-off-by: Pali Roh=C3=A1r <***@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;
=20
#ifdef I8K_TEMPERATURE_BUG
- static int prev[4];
+ static int prev[4] =3D { I8K_MAX_TEMP, I8K_MAX_TEMP, I8K_MAX_TEMP, I8=
K_MAX_TEMP };
#endif
regs.ebx =3D sensor & 0xff;
rc =3D i8k_smm(&regs);
@@ -610,17 +610,17 @@ static int __init i8k_init_hwmon(void)
=20
/* CPU temperature attributes, if temperature reading is OK */
err =3D i8k_get_temp(0);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)
i8k_hwmon_flags |=3D I8K_HWMON_HAVE_TEMP1;
/* check for additional temperature sensors */
err =3D i8k_get_temp(1);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)
i8k_hwmon_flags |=3D I8K_HWMON_HAVE_TEMP2;
err =3D i8k_get_temp(2);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)
i8k_hwmon_flags |=3D I8K_HWMON_HAVE_TEMP3;
err =3D i8k_get_temp(3);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)
i8k_hwmon_flags |=3D I8K_HWMON_HAVE_TEMP4;
=20
/* Left fan attributes, if left fan is present */
--=20
1.7.9.5

[Guenter Roeck]

On some machines some temperature sensors report too high values whic=

h 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 previo=

us

value and at next call it returns I8K_MAX_TEMP.
With this patch also firt i8k_get_temp() call returns I8K_MAX_TEMP an=

d

fix ? Also, I am not entirely sure I understand what exactly you are fi=
xing here.

i8k_init_hwmon() will ignore all sensor ids which report incorrect va=

lues.

---
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] =3D { 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 =3D sensor & 0xff;
rc =3D i8k_smm(&regs);
@@ -610,17 +610,17 @@ static int __init i8k_init_hwmon(void)
/* CPU temperature attributes, if temperature reading is OK */
err =3D i8k_get_temp(0);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)

I8K_MAX_TEMP is, at least in theory, a valid temperature, so this
should be "<=3D".

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 |=3D I8K_HWMON_HAVE_TEMP1;
/* check for additional temperature sensors */
err =3D i8k_get_temp(1);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)
i8k_hwmon_flags |=3D I8K_HWMON_HAVE_TEMP2;
err =3D i8k_get_temp(2);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)
i8k_hwmon_flags |=3D I8K_HWMON_HAVE_TEMP3;
err =3D i8k_get_temp(3);
- if (err >=3D 0)
+ if (err >=3D 0 && err < I8K_MAX_TEMP)
i8k_hwmon_flags |=3D I8K_HWMON_HAVE_TEMP4;
/* Left fan attributes, if left fan is present */

[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?

Post by Guenter Roeck

Post by Pali Rohár
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.

Post by Pali Rohár
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.

Post by Pali Rohár
i8k_init_hwmon() will ignore all sensor ids which report
incorrect values.
---
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.

Post by Pali Rohár
#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

Post by Pali Rohár
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 */