On Thu, 10 Feb 2022, Jan Stary wrote:
> > > When you build a kernel with this, do please add ACPI_DEBUG to your
> > > kernel
> > > config, so we can see more details about what the firmware is telling us.
> >
> > Full dmesg below, without ACPI_DEBUG.
> >
> > Also below, full /var/log/messages with ACPI_DEBUG,
> > as it spams dmesg so much that /var/run/dmesg.boot
> > does not really contain the booting kernel device messages,
> > being rolled off by the storm of ACPI_DEBUG messages.
> > (Is there a way to increase that buffer,
> > so that dmesg.boot would hold everything?)
> > Of course, this is only after syslogd has started;
> > hopefully the acpicpu events are there.
> >
> > Both contain a log of the same scenario: cold start the machine on AC,
> > plug AC out, in, out, in; shutdown with the power button.
>
> With MSGBUFSIZE cranked up,
> here is a dmesg containing all,
> up to before the shutdown.
Uh, wow, I had forgotten how horrifically verbose ACPI_DEBUG was. I'm
half inclined to delete all the uses of ACPI_DEBUG from acpicpu.c and use
a different #define for them.
That said, the data shows the expected 0x81 notifications (and no 0x80
notifications) on the CPU objects, and the values appear to be accurately
parsed the acpicpu.c. Whew.
So here's a revised diff that tries to make it safe for ACPI to notify us
that a CPU's _CST has changed while that cpu is entering idle. Revert the
previous diff before trying to apply this one. Please give it a shot; no
need for ACPI_DEBUG now!
Philip
Index: sys/dev/acpi/acpicpu.c
===================================================================
RCS file: /data/src/openbsd/src/sys/dev/acpi/acpicpu.c,v
retrieving revision 1.91
diff -u -p -r1.91 acpicpu.c
--- sys/dev/acpi/acpicpu.c 9 Jan 2022 05:42:37 -0000 1.91
+++ sys/dev/acpi/acpicpu.c 11 Feb 2022 07:19:11 -0000
@@ -25,6 +25,7 @@
#include <sys/malloc.h>
#include <sys/queue.h>
#include <sys/atomic.h>
+#include <sys/mutex.h>
#include <machine/bus.h>
#include <machine/cpu.h>
@@ -80,6 +81,7 @@ void acpicpu_setperf_ppc_change(struct a
#define CST_FLAG_FALLBACK 0x4000 /* fallback for broken _CST */
#define CST_FLAG_SKIP 0x8000 /* state is worse choice */
+#define FLAGS_NOCST 0x01
#define FLAGS_MWAIT_ONLY 0x02
#define FLAGS_BMCHECK 0x04
#define FLAGS_NOTHROTTLE 0x08
@@ -130,6 +132,11 @@ struct acpicpu_softc {
struct cpu_info *sc_ci;
SLIST_HEAD(,acpi_cstate) sc_cstates;
+ /* sc_mtx protects sc_cstates_active and sc_mwait_only */
+ struct mutex sc_mtx;
+ struct acpi_cstate *sc_cstates_active;
+ int sc_mwait_only;
+
bus_space_tag_t sc_iot;
bus_space_handle_t sc_ioh;
@@ -161,10 +168,12 @@ struct acpicpu_softc {
void acpicpu_add_cstatepkg(struct aml_value *, void *);
void acpicpu_add_cdeppkg(struct aml_value *, void *);
+void acpicpu_cst_activate(struct acpicpu_softc *);
int acpicpu_getppc(struct acpicpu_softc *);
int acpicpu_getpct(struct acpicpu_softc *);
int acpicpu_getpss(struct acpicpu_softc *);
int acpicpu_getcst(struct acpicpu_softc *);
+void acpicpu_free_states(struct acpi_cstate *);
void acpicpu_getcst_from_fadt(struct acpicpu_softc *);
void acpicpu_print_one_cst(struct acpi_cstate *_cx);
void acpicpu_print_cst(struct acpicpu_softc *_sc);
@@ -511,10 +520,10 @@ acpicpu_getcst(struct acpicpu_softc *sc)
int use_nonmwait;
/* delete the existing list */
- while ((cx = SLIST_FIRST(&sc->sc_cstates)) != NULL) {
- SLIST_REMOVE_HEAD(&sc->sc_cstates, link);
- free(cx, M_DEVBUF, sizeof(*cx));
- }
+ cx = SLIST_FIRST(&sc->sc_cstates);
+ SLIST_INIT(&sc->sc_cstates);
+ if (cx != sc->sc_cstates_active)
+ acpicpu_free_states(cx);
/* provide a fallback C1-via-halt in case _CST's C1 is bogus */
acpicpu_add_cstate(sc, ACPI_STATE_C1, CST_METH_HALT,
@@ -526,17 +535,18 @@ acpicpu_getcst(struct acpicpu_softc *sc)
aml_foreachpkg(&res, 1, acpicpu_add_cstatepkg, sc);
aml_freevalue(&res);
+ use_nonmwait = 0;
+
/* only have fallback state? then no _CST objects were understood */
cx = SLIST_FIRST(&sc->sc_cstates);
if (cx->flags & CST_FLAG_FALLBACK)
- return (1);
+ goto done;
/*
* Skip states >= C2 if the CPU's LAPIC timer stops in deep
* states (i.e., it doesn't have the 'ARAT' bit set).
* Also keep track if all the states we'll use use mwait.
*/
- use_nonmwait = 0;
while ((next_cx = SLIST_NEXT(cx, link)) != NULL) {
if (cx->state > 1 &&
(sc->sc_ci->ci_feature_tpmflags & TPM_ARAT) == 0)
@@ -545,6 +555,7 @@ acpicpu_getcst(struct acpicpu_softc *sc)
use_nonmwait = 1;
cx = next_cx;
}
+done:
if (use_nonmwait)
sc->sc_flags &= ~FLAGS_MWAIT_ONLY;
else
@@ -558,6 +569,16 @@ acpicpu_getcst(struct acpicpu_softc *sc)
return (0);
}
+void
+acpicpu_free_states(struct acpi_cstate *cx)
+{
+ while (cx != NULL) {
+ struct acpi_cstate *next_cx = SLIST_NEXT(cx, link);
+ free(cx, M_DEVBUF, sizeof(*cx));
+ cx = next_cx;
+ }
+}
+
/*
* old-style fixed C-state info in the FADT.
* Note that this has extra restrictions on values and flags.
@@ -689,7 +710,9 @@ acpicpu_attach(struct device *parent, st
sc->sc_acpi = (struct acpi_softc *)parent;
sc->sc_devnode = aa->aaa_node;
+ mtx_init(&sc->sc_mtx, IPL_NONE);
SLIST_INIT(&sc->sc_cstates);
+ sc->sc_cstates_active = NULL;
if (aml_evalinteger(sc->sc_acpi, sc->sc_devnode,
"_UID", 0, NULL, &uid) == 0)
@@ -734,9 +757,10 @@ acpicpu_attach(struct device *parent, st
#endif
/* Get C-States from _CST or FADT */
- if (acpicpu_getcst(sc) || SLIST_EMPTY(&sc->sc_cstates))
+ if (acpicpu_getcst(sc) || SLIST_EMPTY(&sc->sc_cstates)) {
acpicpu_getcst_from_fadt(sc);
- else {
+ sc->sc_flags |= FLAGS_NOCST;
+ } else {
/* Notify BIOS we use _CST objects */
if (sc->sc_acpi->sc_fadt->cst_cnt) {
acpi_write_pmreg(sc->sc_acpi, ACPIREG_SMICMD, 0,
@@ -794,9 +818,6 @@ acpicpu_attach(struct device *parent, st
0, sc->sc_acpi->sc_fadt->pstate_cnt);
}
- aml_register_notify(sc->sc_devnode, NULL,
- acpicpu_notify, sc, ACPIDEV_NOPOLL);
-
acpi_gasio(sc->sc_acpi, ACPI_IOREAD,
sc->sc_pct.pct_status.grd_gas.address_space_id,
sc->sc_pct.pct_status.grd_gas.address,
@@ -838,6 +859,39 @@ acpicpu_attach(struct device *parent, st
}
printf("\n");
+ acpicpu_cst_activate(sc);
+
+ /*
+ * If we understood either the provided _CST or _PPC
+ * resources, register for notification of changes.
+ */
+ if ((sc->sc_flags & FLAGS_NOCST) == 0 ||
+ (sc->sc_flags & (FLAGS_NOPSS | FLAGS_NOPCT)) == 0)
+ {
+#ifdef ACPI_DEBUG
+ printf(": %s: enabling notification\n", sc->sc_devnode->name);
+#endif
+ aml_register_notify(sc->sc_devnode, NULL,
+ acpicpu_notify, sc, ACPIDEV_NOPOLL);
+ }
+}
+
+void
+acpicpu_cst_activate(struct acpicpu_softc *sc)
+{
+ struct acpi_cstate *cx, *old_states;
+
+ mtx_enter(&sc->sc_mtx);
+ cx = SLIST_FIRST(&sc->sc_cstates);
+ old_states = sc->sc_cstates_active;
+ if (cx != old_states)
+ sc->sc_cstates_active = cx;
+ else
+ old_states = NULL;
+ sc->sc_mwait_only = (sc->sc_flags & FLAGS_MWAIT_ONLY) != 0;
+ mtx_leave(&sc->sc_mtx);
+
+ acpicpu_free_states(old_states);
}
int
@@ -1023,10 +1077,13 @@ acpicpu_notify(struct aml_node *node, in
break;
case 0x81: /* _CST changed, retrieve new values */
+ if (sc->sc_flags & FLAGS_NOCST)
+ break;
acpicpu_getcst(sc);
printf("%s: notify", DEVNAME(sc));
acpicpu_print_cst(sc);
printf("\n");
+ acpicpu_cst_activate(sc);
break;
default:
@@ -1151,18 +1208,23 @@ acpicpu_setperf(int level)
void
acpicpu_idle(void)
{
- struct cpu_info *ci = curcpu();
- struct acpicpu_softc *sc = (struct acpicpu_softc *)ci->ci_acpicpudev;
- struct acpi_cstate *best, *cx;
- unsigned long itime;
+ struct cpu_info *ci = curcpu();
+ struct acpicpu_softc *sc = (struct acpicpu_softc *)ci->ci_acpicpudev;
+ struct acpi_cstate *best, *cx;
+ unsigned long itime;
+ u_short state;
+ short method;
+ u_int address;
if (sc == NULL) {
__asm volatile("sti");
panic("null acpicpu");
}
+ mtx_enter(&sc->sc_mtx);
+
/* possibly update the MWAIT_ONLY flag in cpu_info */
- if (sc->sc_flags & FLAGS_MWAIT_ONLY) {
+ if (sc->sc_mwait_only) {
if ((ci->ci_mwait & MWAIT_ONLY) == 0)
atomic_setbits_int(&ci->ci_mwait, MWAIT_ONLY);
} else if (ci->ci_mwait & MWAIT_ONLY)
@@ -1172,7 +1234,7 @@ acpicpu_idle(void)
* Find the first state with a latency we'll accept, ignoring
* states marked skippable
*/
- best = cx = SLIST_FIRST(&sc->sc_cstates);
+ best = cx = sc->sc_cstates_active;
while ((cx->flags & CST_FLAG_SKIP) ||
cx->latency * 3 > sc->sc_prev_sleep) {
if ((cx = SLIST_NEXT(cx, link)) == NULL)
@@ -1196,18 +1258,23 @@ acpicpu_idle(void)
best = cx;
}
+ /* made our choice; save the values we need and unlock */
+ state = best->state;
+ method = best->method;
+ address = best->address;
+ mtx_leave(&sc->sc_mtx);
- atomic_inc_long(&cst_stats[best->state]);
+ atomic_inc_long(&cst_stats[state]);
itime = tick / 2;
- switch (best->method) {
+ switch (method) {
default:
case CST_METH_HALT:
__asm volatile("sti; hlt");
break;
case CST_METH_IO_HALT:
- inb((u_short)best->address);
+ inb((u_short)address);
__asm volatile("sti; hlt");
break;
@@ -1238,7 +1305,7 @@ acpicpu_idle(void)
* something to the queue and called cpu_unidle() between
* the check in sched_idle() and here.
*/
- hints = (unsigned)best->address;
+ hints = (unsigned)address;
microuptime(&start);
atomic_setbits_int(&ci->ci_mwait, MWAIT_IDLING);
if (cpu_is_idle(ci)) {
@@ -1265,7 +1332,7 @@ acpicpu_idle(void)
}
case CST_METH_GAS_IO:
- inb((u_short)best->address);
+ inb((u_short)address);
/* something harmless to give system time to change state */
acpi_read_pmreg(acpi_softc, ACPIREG_PM1_STS, 0);
break;
