Re: acpithinkpad: don't setup non-existent temp sensors
On Sat, Apr 08, 2023 at 10:09:11AM -0500, joshua stein wrote: > The sensor nodes (TMP0, TMP1, etc.) are hard-coded into the DSDT, so > the EC failing to read the actual sensor should still allow it to be > found. Understood, thanks. > Yes, here is a new version without that chunk. Works for me, looks sane, OK kn
Re: acpithinkpad: don't setup non-existent temp sensors
On Fri, 07 Apr 2023 at 19:00:43 +, Klemens Nanni wrote:
> > + sc->sc_ntempsens = 0;
> > + for (i = 0; i < THINKPAD_NSENSORS - THINKPAD_SENSOR_TMP0; i++) {
> > + if (thinkpad_get_temp(sc, i, ) != 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.
The sensor nodes (TMP0, TMP1, etc.) are hard-coded into the DSDT, so
the EC failing to read the actual sensor should still allow it to be
found.
> > - /* 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_brightness) == 0)) {
> > + if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "PBLG",
> > + 0, NULL, >sc_brightness) == 0) {
> > ws_get_param = thinkpad_get_param;
> > ws_set_param = thinkpad_set_param;
> > }
>
> Is this an unrelated diff that snuck in?
Yes, here is a new version without that chunk.
> > + 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?
Correct, this is just reporting whether the node exists and can be
read, not whether the sensor is broken or reporting bogus values.
Index: 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
--- acpithinkpad.c 6 Apr 2022 18:59:27 - 1.70
+++ acpithinkpad.c 8 Apr 2023 15:03:09 -
@@ -112,11 +112,10 @@
#defineTHINKPAD_TABLET_SCREEN_CHANGED 0x60c0
#defineTHINKPAD_SWITCH_WIRELESS0x7000
-#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 ksensordevsc_sensdev;
+ int sc_ntempsens;
uint64_t sc_hkey_version;
@@ -178,6 +178,7 @@ int thinkpad_get_brightness(struct acpit
intthinkpad_set_brightness(void *, int);
intthinkpad_get_param(struct wsdisplay_param *);
intthinkpad_set_param(struct wsdisplay_param *);
+intthinkpad_get_temp(struct acpithinkpad_softc *, int, int64_t *);
voidthinkpad_sensor_attach(struct acpithinkpad_softc *sc);
voidthinkpad_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; isc_sens[i].type = SENSOR_TEMP;
- sensor_attach(>sc_sensdev, >sc_sens[i]);
- }
+ sc->sc_ntempsens = 0;
+ for (i = 0; i < THINKPAD_NSENSORS - THINKPAD_SENSOR_TMP0; i++) {
+ if (thinkpad_get_temp(sc, i, ) != 0)
+ break;
+
+ sc->sc_sens[THINKPAD_SENSOR_TMP0 + i].type = SENSOR_TEMP;
+ sensor_attach(>sc_sensdev,
+ >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; isc_acpi, sc->sc_ec->sc_devnode,
- sname, 0, 0, );
- sc->sc_sens[i].value = (tmp * 100) + 27315;
- 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, ) != 0) {
+ sc->sc_sens[i].flags = SENSOR_FINVALID;
+ continue;
+ }
+
+
Re: acpithinkpad: don't setup non-existent temp sensors
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 - 1.70
> +++ sys/dev/acpi/acpithinkpad.c 5 Apr 2023 03:43:59 -
> @@ -112,11 +112,10 @@
> #define THINKPAD_TABLET_SCREEN_CHANGED 0x60c0
> #define THINKPAD_SWITCH_WIRELESS0x7000
>
> -#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_NSENSORS10
>
> #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 ksensordevsc_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 *);
>
> voidthinkpad_sensor_attach(struct acpithinkpad_softc *sc);
> voidthinkpad_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 - sc->sc_sens[i].type = SENSOR_TEMP;
> - sensor_attach(>sc_sensdev, >sc_sens[i]);
> - }
> + sc->sc_ntempsens = 0;
> + for (i = 0; i < THINKPAD_NSENSORS - THINKPAD_SENSOR_TMP0; i++) {
> + if (thinkpad_get_temp(sc, i, ) != 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_sensdev,
> + >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 - snprintf(sname, sizeof(sname), "TMP%d", i);
> - aml_evalinteger(sc->sc_acpi, sc->sc_ec->sc_devnode,
> - sname, 0, 0, );
> - sc->sc_sens[i].value = (tmp * 100) + 27315;
> - 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, ) != 0) {
> + sc->sc_sens[i].flags = SENSOR_FINVALID;
> + continue;
> +
Re: acpithinkpad: don't setup non-existent temp sensors
On Tue, Apr 04, 2023 at 10:45:53PM -0500, joshua stein wrote: > 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): I see this on a X230 with coreboot: hw.sensors.acpithinkpad0.temp0=66.00 degC hw.sensors.acpithinkpad0.temp1=0.00 degC hw.sensors.acpithinkpad0.temp2=0.00 degC hw.sensors.acpithinkpad0.temp3=0.00 degC hw.sensors.acpithinkpad0.temp4=0.00 degC hw.sensors.acpithinkpad0.temp5=0.00 degC hw.sensors.acpithinkpad0.temp6=0.00 degC hw.sensors.acpithinkpad0.temp7=0.00 degC hw.sensors.acpithinkpad0.fan0=4326 RPM hw.sensors.acpithinkpad0.indicator0=Off (port replicator), UNKNOWN (It's in a proper dock, but .indicator0 always says "Off"; I think this used to work...) > 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. hw.sensors.acpithinkpad0.temp0=65.00 degC hw.sensors.acpithinkpad0.temp1=0.00 degC hw.sensors.acpithinkpad0.fan0=4326 RPM hw.sensors.acpithinkpad0.indicator0=Off (port replicator), UNKNOWN Works for me, but looks like temp1 is actually a second sensor which doesn't work? Definitely warmer around here. Do you want me to test anything?
