On Tue, 8 Feb 2022, [email protected] wrote:
> >Synopsis: C2 state not recognized on Thinkpad T420s when on AC
> >Category: system
> >Environment:
> System : OpenBSD 7.0
> Details : OpenBSD 7.0-current (GENERIC.MP) #0: Sun Feb 6 21:10:48
> CET 2022
>
> [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>
> Architecture: OpenBSD.amd64
> Machine : amd64
> >Description:
> The booting kernel on a ThinkpadT420s does not recognize the C2 state
> when booting on AC power, while it does when booting on battery;
>
>
> -acpiac0 at acpi0: AC unit online
> +acpiac0 at acpi0: AC unit offline
> acpithinkpad0 at acpi0: version 1.0
> "PNP0C14" at acpi0 not configured
> "PNP0C14" at acpi0 not configured
> -acpicpu0 at acpi0: C3(350@104 io@0x415), C1(1000@1 halt), PSS
> -acpicpu1 at acpi0: C3(350@104 io@0x415), C1(1000@1 halt), PSS
> -acpicpu2 at acpi0: C3(350@104 io@0x415), C1(1000@1 halt), PSS
> -acpicpu3 at acpi0: C3(350@104 io@0x415), C1(1000@1 halt), PSS
> +acpicpu0 at acpi0: C3(200@109 io@0x416), C2(500@80 io@0x414), C1(1000@1
> halt), PSS
> +acpicpu1 at acpi0: C3(200@109 io@0x416), C2(500@80 io@0x414), C1(1000@1
> halt), PSS
> +acpicpu2 at acpi0: C3(200@109 io@0x416), C2(500@80 io@0x414), C1(1000@1
> halt), PSS
> +acpicpu3 at acpi0: C3(200@109 io@0x416), C2(500@80 io@0x414), C1(1000@1
> halt), PSS
>
> >How-To-Repeat:
> Boot the Thnikpad T420s on AC and on battery.
I think there's a defect in how we decide whether to register the CPU node
notification and need to lift it out of the block it's in so it can be
registered for _CST changes even if we're not going to listen for _PPC
changes. The diff below attempts to address that. I suspect it's not
100% safe: AIUI, notifications may arrive while a CPU is trying to use the
data to go idle, so this might make some setups less stable (definitely
not snapshot material) but would be useful test change to see if it
results in notifications being reported on this T420 that affect the
presence of C2.
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.
Philip Guenther
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 9 Feb 2022 18:18:49 -0000
@@ -85,6 +85,7 @@ void acpicpu_setperf_ppc_change(struct a
#define FLAGS_NOTHROTTLE 0x08
#define FLAGS_NOPSS 0x10
#define FLAGS_NOPCT 0x20
+#define FLAGS_NOCST 0x40
#define CPU_THT_EN (1L << 4)
#define CPU_MAXSTATE(sc) (1L << (sc)->sc_duty_wid)
@@ -734,9 +735,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 +796,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,
@@ -812,6 +811,20 @@ acpicpu_attach(struct device *parent, st
acpi_hasprocfvs = 1;
}
}
+ }
+
+ /*
+ * 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);
}
/*
