On Sat, Jan 25 2020, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote: > So I have this diff for apmd -z/-Z that uses APM_POWER_CHANGE events to > trigger autosuspend. It works fine except for one glitch: if I plug the > AC cable and then resume, apmd will receive another APM_POWER_CHANGE > event and read the power info only to find that the AC is *unplugged*. > So apmd happily suspends my system again. If I resume once more, the AC > status is correctly detected and apmd(8) doesn't suspend the system again. > > What happens is that acpibat_notify runs because of ACPIDEV_POLL, it > grabs battery data and sends an APM_POWER_CHANGE event to apmd. At this > point apmd may see out of date data from acpiac(4). > > acpi_sleep_task later forces a refresh of (in order) acpiac(4), > acpibat(4) and acpisbs(4), but in my use case it's too late, the window > with stale acpiac(4) data is too long.
Even though apmd(8) -z/-Z isn't negatively affected any more, this problem holds. acpibat(4) can send APM_POWER_CHANGE events to userland before the acpi thread refreshes acpiac(4) status. You can easily check this by running apmd -d and going through zzz cycles with A/C plugged/unplugged. I think we should show a consistent state to userland, even if the apm emulation in acpi(4) is implemented using several drivers, > The diff below refreshes AC status with notifications at DVACT_WAKEUP > time. Is is correct that DVACT_WAKEUP handlers will always run before > the ACPI task queue is processed? "No" seems unlikely but I'd rather > ask... The answer was "yes"... > I hear we prefer running ACPI code in its dedicated thread but there are > other drivers in dev/acpi that run ACPI code in DVACT_WAKEUP, so I'm > assuming it is fine. :) Indeed it's fine. The acpi thread runs acpi_sleep_state() and thus the DVACT_WAKEUP handlers. I guess that stuff looked like pure magic to me at that time. The diff below teaches acpiac(4), acpibat(4) and acpisbs(4) to refresh their state using DVACT_WAKEUP handlers instead of using the *_notify handlers in acpi_sleep_task(). This ensures a consistent state right after resume, and makes those drivers less special. I'm reusing the *_refresh functions in the new handlers, so send APM_POWER_CHANGE events from the *_notify functions instead. Finally, at the end of acpi_sleep_task() queue an APM_POWER_CHANGE event so that userland can look at fresh power info as soon as possible. Thoughts? oks? Index: acpi.c =================================================================== RCS file: /d/cvs/src/sys/dev/acpi/acpi.c,v retrieving revision 1.385 diff -u -p -r1.385 acpi.c --- acpi.c 14 May 2020 13:07:10 -0000 1.385 +++ acpi.c 17 May 2020 12:08:11 -0000 @@ -1942,20 +1942,11 @@ void acpi_sleep_task(void *arg0, int sleepmode) { struct acpi_softc *sc = arg0; - struct acpi_ac *ac; - struct acpi_bat *bat; - struct acpi_sbs *sbs; /* System goes to sleep here.. */ acpi_sleep_state(sc, sleepmode); - - /* AC and battery information needs refreshing */ - SLIST_FOREACH(ac, &sc->sc_ac, aac_link) - aml_notify(ac->aac_softc->sc_devnode, 0x80); - SLIST_FOREACH(bat, &sc->sc_bat, aba_link) - aml_notify(bat->aba_softc->sc_devnode, 0x80); - SLIST_FOREACH(sbs, &sc->sc_sbs, asbs_link) - aml_notify(sbs->asbs_softc->sc_devnode, 0x80); + /* Tell userland to recheck A/C and battery status */ + acpi_record_event(sc, APM_POWER_CHANGE); } #endif /* SMALL_KERNEL */ Index: acpiac.c =================================================================== RCS file: /d/cvs/src/sys/dev/acpi/acpiac.c,v retrieving revision 1.32 diff -u -p -r1.32 acpiac.c --- acpiac.c 9 May 2020 00:40:48 -0000 1.32 +++ acpiac.c 9 May 2020 00:55:46 -0000 @@ -33,13 +33,18 @@ int acpiac_match(struct device *, void *, void *); void acpiac_attach(struct device *, struct device *, void *); +int acpiac_activate(struct device *, int); int acpiac_notify(struct aml_node *, int, void *); void acpiac_refresh(void *); int acpiac_getpsr(struct acpiac_softc *); struct cfattach acpiac_ca = { - sizeof(struct acpiac_softc), acpiac_match, acpiac_attach + sizeof(struct acpiac_softc), + acpiac_match, + acpiac_attach, + NULL, + acpiac_activate, }; struct cfdriver acpiac_cd = { @@ -92,6 +97,21 @@ acpiac_attach(struct device *parent, str acpiac_notify, sc, ACPIDEV_NOPOLL); } +int +acpiac_activate(struct device *self, int act) +{ + struct acpiac_softc *sc = (struct acpiac_softc *)self; + + switch (act) { + case DVACT_WAKEUP: + acpiac_refresh(sc); + dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat); + break; + } + + return (0); +} + void acpiac_refresh(void *arg) { @@ -99,7 +119,6 @@ acpiac_refresh(void *arg) acpiac_getpsr(sc); sc->sc_sens[0].value = sc->sc_ac_stat; - acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE); } int @@ -136,6 +155,7 @@ acpiac_notify(struct aml_node *node, int /* FALLTHROUGH */ case 0x80: acpiac_refresh(sc); + acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE); dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat); break; } Index: acpibat.c =================================================================== RCS file: /d/cvs/src/sys/dev/acpi/acpibat.c,v retrieving revision 1.67 diff -u -p -r1.67 acpibat.c --- acpibat.c 1 Jul 2018 19:40:49 -0000 1.67 +++ acpibat.c 17 May 2020 12:30:19 -0000 @@ -31,9 +31,14 @@ int acpibat_match(struct device *, void *, void *); void acpibat_attach(struct device *, struct device *, void *); +int acpibat_activate(struct device *, int); struct cfattach acpibat_ca = { - sizeof(struct acpibat_softc), acpibat_match, acpibat_attach + sizeof(struct acpibat_softc), + acpibat_match, + acpibat_attach, + NULL, + acpibat_activate, }; struct cfdriver acpibat_cd = { @@ -110,6 +115,31 @@ acpibat_attach(struct device *parent, st acpibat_notify, sc, ACPIDEV_POLL); } +int +acpibat_activate(struct device *self, int act) +{ + struct acpibat_softc *sc = (struct acpibat_softc *)self; + int64_t sta; + + switch (act) { + case DVACT_WAKEUP: + /* Check if installed state of battery has changed */ + if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode, "_STA", 0, NULL, &sta) + == 0) { + if (sta & STA_BATTERY) + sc->sc_bat_present = 1; + else + sc->sc_bat_present = 0; + } + acpibat_getbix(sc); + acpibat_getbst(sc); + acpibat_refresh(sc); + break; + } + + return (0); +} + void acpibat_monitor(struct acpibat_softc *sc) { @@ -315,8 +345,6 @@ acpibat_refresh(void *arg) sc->sc_sens[9].flags = 0; } } - - acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE); } int @@ -505,6 +533,7 @@ acpibat_notify(struct aml_node *node, in } acpibat_refresh(sc); + acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE); return (0); } Index: acpisbs.c =================================================================== RCS file: /d/cvs/src/sys/dev/acpi/acpisbs.c,v retrieving revision 1.9 diff -u -p -r1.9 acpisbs.c --- acpisbs.c 27 Jan 2020 11:04:18 -0000 1.9 +++ acpisbs.c 17 May 2020 11:28:09 -0000 @@ -128,6 +128,7 @@ extern void acpiec_write(struct acpiec_s int acpisbs_match(struct device *, void *, void *); void acpisbs_attach(struct device *, struct device *, void *); +int acpisbs_activate(struct device *, int); void acpisbs_setup_sensors(struct acpisbs_softc *); void acpisbs_refresh_sensors(struct acpisbs_softc *); void acpisbs_read(struct acpisbs_softc *); @@ -139,6 +140,8 @@ const struct cfattach acpisbs_ca = { sizeof(struct acpisbs_softc), acpisbs_match, acpisbs_attach, + NULL, + acpisbs_activate, }; struct cfdriver acpisbs_cd = { @@ -355,6 +358,21 @@ acpisbs_refresh_sensors(struct acpisbs_s sc->sc_sensors[i].flags = SENSOR_FUNKNOWN; } } +} + +int +acpisbs_activate(struct device *self, int act) +{ + struct acpisbs_softc *sc = (struct acpisbs_softc *)self; + + switch (act) { + case DVACT_WAKEUP: + acpisbs_read(sc); + acpisbs_refresh_sensors(sc); + break; + } + + return 0; } int -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE