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
>
