My original diff would be an improvement, but I think it is not sufficient,
because I have found some more problems with the refresh code.
The return value of aml_evalinteger is not checked and it sporadically _does_
return ACPI_E_BADVALUE. When aml_evalinteger returns non-zero, tmp is not
touched and is either uninitialized or retains the value from the previous
loop iteration. So sysctl shows 8 temperature sensors on my machine, all with
the same temperature, while only in the first loop iteration the aml_evalinteger
returned successfully.
Additionally during the wake up time after suspend the tmp value is 128 if
aml_evalinteger succeeds and the fan speed is reported as 65535.
The following diff addresses these issues by:
* checking the return value of aml_evalinteger and setting the SENSOR_FINVALID
flag when it is non-zero
* setting the SENSOR_FUNKNOWN flag when tmp is 128
* printf an error message when the temperature is invalid even though
aml_evalinteger returned 0 (but resetting the flags to 0 on the next run so
the sensor doesn't vanish forever)
* setting the SENSOR_FUNKNOWN flag when fan speed is 65535
After applying the diff the number of sensors displayed on my Thinkpad T520 is
reduced to one. Both the temperature sensors and the fan speed sensors
show "unknown" instead of disappearing during the wake up time.
comments?
regards
natano
Index: dev/acpi/acpithinkpad.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
retrieving revision 1.30
diff -u -p -r1.30 acpithinkpad.c
--- dev/acpi/acpithinkpad.c 2 Apr 2013 00:46:47 -0000 1.30
+++ dev/acpi/acpithinkpad.c 29 Jun 2013 09:07:17 -0000
@@ -178,23 +178,33 @@ thinkpad_sensor_refresh(void *arg)
{
struct acpithinkpad_softc *sc = arg;
u_int8_t lo, hi, i;
- int64_t tmp;
+ int64_t tmp, speed;
char sname[5];
/* Refresh sensor readings */
for (i=0; i<THINKPAD_NTEMPSENSORS; i++) {
snprintf(sname, sizeof(sname), "TMP%d", i);
- aml_evalinteger(sc->sc_acpi, sc->sc_ec->sc_devnode,
- sname, 0, 0, &tmp);
- sc->sc_sens[i].value = (tmp * 1000000) + 273150000;
- if (tmp > 127 || tmp < -127)
- sc->sc_sens[i].flags = SENSOR_FINVALID;
+ if (!aml_evalinteger(sc->sc_acpi, sc->sc_ec->sc_devnode,
+ sname, 0, 0, &tmp)) {
+ sc->sc_sens[i].value = (tmp * 1000000) + 273150000;
+ sc->sc_sens[i].flags = 0;
+ if (tmp == 128) {
+ sc->sc_sens[i].flags |= SENSOR_FUNKNOWN;
+ } else if (tmp > 127 || tmp < -127) {
+ printf("%s: invalid temperature: %lld\n",
+ DEVNAME(sc), tmp);
+ sc->sc_sens[i].flags |= SENSOR_FINVALID;
+ }
+ } else
+ sc->sc_sens[i].flags |= SENSOR_FINVALID;
}
/* Read fan RPM */
acpiec_read(sc->sc_ec, THINKPAD_ECOFFSET_FANLO, 1, &lo);
acpiec_read(sc->sc_ec, THINKPAD_ECOFFSET_FANHI, 1, &hi);
- sc->sc_sens[i].value = ((hi << 8L) + lo);
+ speed = ((hi << 8L) + lo);
+ sc->sc_sens[i].value = speed;
+ sc->sc_sens[i].flags = (speed == 0xffff) ? SENSOR_FUNKNOWN : 0;
}
void
On Sat, Jun 29, 2013 at 12:51:29PM +0400, Vadim Zhukov wrote:
> I got the point. So it's up to sysadmin to trust or not the sensor data.
> Then original patch should be fine, except small nit - I think,
> it's better to reset only SENSOR_FINVALID flag instead of all.
>
> CC'ing ACPI guys for okays.
>
> --
> WBR,
> Vadim Zhukov
>
>
> Index: acpithinkpad.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 acpithinkpad.c
> --- acpithinkpad.c 2 Apr 2013 00:46:47 -0000 1.30
> +++ acpithinkpad.c 29 Jun 2013 08:41:46 -0000
> @@ -189,6 +189,8 @@ thinkpad_sensor_refresh(void *arg)
> sc->sc_sens[i].value = (tmp * 1000000) + 273150000;
> if (tmp > 127 || tmp < -127)
> sc->sc_sens[i].flags = SENSOR_FINVALID;
> + else
> + sc->sc_sens[i].flags &= ~SENSOR_FINVALID;
> }
>
> /* Read fan RPM */
