On Sat, Jan 25 2020, Jeremie Courreges-Anglas <[email protected]> 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
