> Date: Thu, 10 Feb 2022 23:46:43 -0800
> From: guent...@openbsd.org
>
> 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.
Go for it.
> 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;
>
>
