On Tue, Apr 19, 2016 at 11:18:33AM +0200, Sebastien Marie wrote:
> Hi,
>
> As I got some feedback for this old thread, I would like to ressurect it
> and ask for wider testing: does your fans still works correctly with the
> patch ?
>
> I personnally ran with it since September 2014.
>
>
> The acpitz problem was with active cooling (the ACPI subsystem that
> plays with fans depending of the temperature), more precisely with
> hysterisis (the fact that the temperature limit changes automatically
> when the limit is reach).
>
> The current problem is (on my host which use hysterisis for active
> cooling) that once the fan starts to spin, it never go down (keeping
> the noise at high level).
>
>
> With the current code:
>
> - when notify 0x81 is receive (operating points changed - due to
> hysterisis), sc->sc_ac_stat[i] is set inconditionnally to -1 (state
> of fans is unknown/ON/OFF)
>
> - but the current code readed _ALx (_STA) to update sc->sc_ac_stat[i]
> only when it sets fan ON or OFF.
>
> - there is no code path for set a fan OFF when sc->sc_ac_stat[i] == -1
> (whereas it is possible to set a fan ON)
>
> - so when the fan is ON and notify 0x81 received, the fan never go
> back to OFF (and sc->sc_ac_stat[i] keep -1 value).
>
>
> The diff allows fan to turn OFF if the state is unknown.
>
> Thanks for testing it.
> --
> Sebastien Marie
>
I have tested this on an x230 and observed no regressions, and I think
the diff does the right thing (it basically says "if the temperature is below
the active cooling level for a tz, turn the fan off regardless of what state
it is currently in"). As Sebastien points out, we would only turn off the
fan today if we knew definitively that it was on, and this may not be the
case if the thermal trip values were changed via an aml notify (in which
case acpitz_init gets called again resulting in the fan state being set
back to UNKNOWN).
ok mlarkin@ if it causes no breakage elsewhere.
-ml
>
> Index: sys/dev/acpi/acpitz.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpitz.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 acpitz.c
> --- sys/dev/acpi/acpitz.c 6 May 2015 01:41:55 -0000 1.49
> +++ sys/dev/acpi/acpitz.c 19 Apr 2016 08:50:12 -0000
> @@ -37,6 +37,7 @@
> #define KTOC(k) ((k - 2732) / 10)
> #define ACPITZ_MAX_AC (10)
> #define ACPITZ_TMP_RETRY (3)
> +#define ACPITZ_UNKNOWN (-1)
>
> struct acpitz_softc {
> struct device sc_dev;
> @@ -139,7 +140,6 @@ acpitz_init(struct acpitz_softc *sc, int
> for (i = 0; i < ACPITZ_MAX_AC; i++) {
> snprintf(name, sizeof(name), "_AC%d", i);
> sc->sc_ac[i] = acpitz_getreading(sc, name);
> - sc->sc_ac_stat[i] = -1;
> }
> }
>
> @@ -160,6 +160,8 @@ acpitz_init(struct acpitz_softc *sc, int
> sc->sc_devnode, &res, 0);
> aml_freevalue(&res);
> }
> + /* initialize current state to unknown */
> + sc->sc_ac_stat[i] = ACPITZ_UNKNOWN;
> }
> }
> }
> @@ -423,7 +425,8 @@ acpitz_refresh(void *arg)
> acpitz_setfan(sc, i, "_ON_");
> } else if (sc->sc_ac[i] != -1) {
> /* turn off fan i */
> - if (sc->sc_ac_stat[i] > 0)
> + if ((sc->sc_ac_stat[i] == ACPITZ_UNKNOWN) ||
> + (sc->sc_ac_stat[i] > 0))
> acpitz_setfan(sc, i, "_OFF");
> }
> }
>
