05.04.2023 03:45, joshua stein пишет: > acpithinkpad sets up a hard-coded number of temperature sensors and > doesn't check the result of aml_evalinteger when polling, so for all > invalid sensors it ends up reporting the value of the previous > successful sensor check resulting in this (for a machine with only a > TMP0 sensor): > > hw.sensors.acpithinkpad0.temp0=47.00 degC > hw.sensors.acpithinkpad0.temp1=47.00 degC > hw.sensors.acpithinkpad0.temp2=47.00 degC > hw.sensors.acpithinkpad0.temp3=47.00 degC > hw.sensors.acpithinkpad0.temp4=47.00 degC > hw.sensors.acpithinkpad0.temp5=47.00 degC > hw.sensors.acpithinkpad0.temp6=47.00 degC > hw.sensors.acpithinkpad0.temp7=47.00 degC > hw.sensors.acpithinkpad0.fan0=3605 RPM > hw.sensors.acpithinkpad0.indicator0=Off (port replicator), UNKNOWN > > This diff checks whether the TMP[0-8] node actually exists during > attach and if not, it doesn't create the sensor. It also checks the > return value during polling and sets the sensor to invalid if it > failed for some reason.
This makes sense, however... > > hw.sensors.acpithinkpad0.temp0=55.00 degC > hw.sensors.acpithinkpad0.fan0=5045 RPM > hw.sensors.acpithinkpad0.indicator0=Off (port replicator), UNKNOWN > > > Index: sys/dev/acpi/acpithinkpad.c > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpithinkpad.c,v > retrieving revision 1.70 > diff -u -p -u -p -r1.70 acpithinkpad.c > --- sys/dev/acpi/acpithinkpad.c 6 Apr 2022 18:59:27 -0000 1.70 > +++ sys/dev/acpi/acpithinkpad.c 5 Apr 2023 03:43:59 -0000 > @@ -112,11 +112,10 @@ > #define THINKPAD_TABLET_SCREEN_CHANGED 0x60c0 > #define THINKPAD_SWITCH_WIRELESS 0x7000 > > -#define THINKPAD_NSENSORS 10 > -#define THINKPAD_NTEMPSENSORS 8 > - > -#define THINKPAD_SENSOR_FANRPM THINKPAD_NTEMPSENSORS > -#define THINKPAD_SENSOR_PORTREPL THINKPAD_NTEMPSENSORS + 1 > +#define THINKPAD_SENSOR_FANRPM 0 > +#define THINKPAD_SENSOR_PORTREPL 1 > +#define THINKPAD_SENSOR_TMP0 2 > +#define THINKPAD_NSENSORS 10 > > #define THINKPAD_ECOFFSET_VOLUME 0x30 > #define THINKPAD_ECOFFSET_VOLUME_MUTE_MASK 0x40 > @@ -140,6 +139,7 @@ struct acpithinkpad_softc { > > struct ksensor sc_sens[THINKPAD_NSENSORS]; > struct ksensordev sc_sensdev; > + int sc_ntempsens; > > uint64_t sc_hkey_version; > > @@ -178,6 +178,7 @@ int thinkpad_get_brightness(struct acpit > int thinkpad_set_brightness(void *, int); > int thinkpad_get_param(struct wsdisplay_param *); > int thinkpad_set_param(struct wsdisplay_param *); > +int thinkpad_get_temp(struct acpithinkpad_softc *, int, int64_t *); > > void thinkpad_sensor_attach(struct acpithinkpad_softc *sc); > void thinkpad_sensor_refresh(void *); > @@ -228,6 +229,7 @@ thinkpad_match(struct device *parent, vo > void > thinkpad_sensor_attach(struct acpithinkpad_softc *sc) > { > + int64_t tmp; > int i; > > if (sc->sc_acpi->sc_ec == NULL) > @@ -237,10 +239,16 @@ thinkpad_sensor_attach(struct acpithinkp > /* Add temperature probes */ > strlcpy(sc->sc_sensdev.xname, DEVNAME(sc), > sizeof(sc->sc_sensdev.xname)); > - for (i=0; i<THINKPAD_NTEMPSENSORS; i++) { > - sc->sc_sens[i].type = SENSOR_TEMP; > - sensor_attach(&sc->sc_sensdev, &sc->sc_sens[i]); > - } > + sc->sc_ntempsens = 0; > + for (i = 0; i < THINKPAD_NSENSORS - THINKPAD_SENSOR_TMP0; i++) { > + if (thinkpad_get_temp(sc, i, &tmp) != 0) > + break; doesn't this mean, that legit sensors which happen to fail this read upon attach won't ever be visible at runtime? I have no idea how reliable those sensors are, but given that the code handles spurious failure, this seems not too far fetched. > + > + sc->sc_sens[THINKPAD_SENSOR_TMP0 + i].type = SENSOR_TEMP; > + sensor_attach(&sc->sc_sensdev, > + &sc->sc_sens[THINKPAD_SENSOR_TMP0 + i]); > + sc->sc_ntempsens++; > + } > > /* Add fan probe */ > sc->sc_sens[THINKPAD_SENSOR_FANRPM].type = SENSOR_FANRPM; > @@ -262,17 +270,19 @@ thinkpad_sensor_refresh(void *arg) > struct acpithinkpad_softc *sc = arg; > uint8_t lo, hi, i; > int64_t tmp; > - 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; > - } > + for (i = 0; i < sc->sc_ntempsens; i++) { > + if (thinkpad_get_temp(sc, i, &tmp) != 0) { > + sc->sc_sens[i].flags = SENSOR_FINVALID; > + continue; > + } > + > + sc->sc_sens[THINKPAD_SENSOR_TMP0 + i].value = > + (tmp * 1000000) + 273150000; > + sc->sc_sens[THINKPAD_SENSOR_TMP0 + i].flags = > + (tmp > 127 || tmp < -127) ? SENSOR_FINVALID : 0; > + } > > /* Read fan RPM */ > acpiec_read(sc->sc_ec, THINKPAD_ECOFFSET_FANLO, 1, &lo); > @@ -321,10 +331,8 @@ thinkpad_attach(struct device *parent, s > wskbd_set_backlight = thinkpad_set_kbd_backlight; > } > > - /* On version 2 and newer, let *drm or acpivout control brightness */ > - if (sc->sc_hkey_version == THINKPAD_HKEY_VERSION1 && > - (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG", > - 0, NULL, &sc->sc_brightness) == 0)) { > + if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG", > + 0, NULL, &sc->sc_brightness) == 0) { > ws_get_param = thinkpad_get_param; > ws_set_param = thinkpad_set_param; > } Is this an unrelated diff that snuck in? > @@ -785,6 +793,20 @@ thinkpad_set_param(struct wsdisplay_para > default: > return -1; > } > +} > + > +int > +thinkpad_get_temp(struct acpithinkpad_softc *sc, int idx, int64_t *temp) > +{ > + char sname[5]; > + > + snprintf(sname, sizeof(sname), "TMP%d", idx); > + > + if (aml_evalinteger(sc->sc_acpi, sc->sc_ec->sc_devnode, sname, 0, 0, > + temp) != 0) > + return (1); Looks liks aml_evalnode(9) only returns 0/success or ACPI_E_BADVALUE/failure anyway, so there's no way to distinguish between nonexistent sensors and existent but failing sensors, right? > + > + return (0); > } > > #if NAUDIO > 0 && NWSKBD > 0 >