From 60ff2f5371f8efd2071a72d9e0de741ae983de64 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 17 Jul 2023 23:40:20 +0000 Subject: [PATCH 01/21] acpiec(4): Record device_t self. Not used yet, to be used soon for device_printf and to allow making some of the internal functions a little more type-safe later. --- sys/dev/acpi/acpi_ec.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 16f852a53028..02294166f41a 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -119,6 +119,8 @@ enum ec_state_t { }; struct acpiec_softc { + device_t sc_dev; + ACPI_HANDLE sc_ech; ACPI_HANDLE sc_gpeh; @@ -313,6 +315,8 @@ acpiec_common_attach(device_t parent, device_t self, ACPI_STATUS rv; ACPI_INTEGER val; + sc->sc_dev = self; + sc->sc_csr_st = cmdt; sc->sc_data_st = datat; From 71ec6a63973c5ffe15d090358b6f150d48559b6d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 17 Jul 2023 23:49:42 +0000 Subject: [PATCH 02/21] acpiec(4): New ACPIEC_DEBUG option. Value is bit mask of debug messages to enable. Enable in x86/ALL kernels. No functional change intended when the option is off. --- sys/arch/amd64/conf/ALL | 1 + sys/arch/i386/conf/ALL | 1 + sys/dev/acpi/acpi_ec.c | 161 ++++++++++++++++++++++++++++++++++++---- sys/dev/acpi/files.acpi | 1 + 4 files changed, 151 insertions(+), 13 deletions(-) diff --git a/sys/arch/amd64/conf/ALL b/sys/arch/amd64/conf/ALL index 34f277d86585..76c16c8e3c80 100644 --- a/sys/arch/amd64/conf/ALL +++ b/sys/arch/amd64/conf/ALL @@ -370,6 +370,7 @@ acpibut* at acpi? # ACPI Button acpidalb* at acpi? # ACPI Direct Application Launch Button acpiec* at acpi? # ACPI Embedded Controller (late) acpiecdt* at acpi? # ACPI Embedded Controller (early) +options ACPIEC_DEBUG=-1 acpifan* at acpi? # ACPI Fan acpilid* at acpi? # ACPI Lid Switch acpipmtr* at acpi? # ACPI Power Meter (experimental) diff --git a/sys/arch/i386/conf/ALL b/sys/arch/i386/conf/ALL index 03659409a343..9a7a158cdca2 100644 --- a/sys/arch/i386/conf/ALL +++ b/sys/arch/i386/conf/ALL @@ -357,6 +357,7 @@ acpibut* at acpi? # ACPI Button acpidalb* at acpi? # ACPI Direct Application Launch Button acpiec* at acpi? # ACPI Embedded Controller (late) acpiecdt* at acpi? # ACPI Embedded Controller (early) +options ACPIEC_DEBUG=-1 acpifan* at acpi? # ACPI Fan acpilid* at acpi? # ACPI Lid Switch acpipmtr* at acpi? # ACPI Power Meter (experimental) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 02294166f41a..05c55d039134 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -61,6 +61,10 @@ #include __KERNEL_RCSID(0, "$NetBSD: acpi_ec.c,v 1.86 2021/12/31 17:22:25 riastradh Exp $"); +#ifdef _KERNEL_OPT +#include "opt_acpi_ec.h" +#endif + #include #include #include @@ -101,22 +105,38 @@ ACPI_MODULE_NAME ("acpi_ec") #define EC_STATUS_SCI 0x20 #define EC_STATUS_SMI 0x40 +#define EC_STATUS_FMT \ + "\x10\10IGN7\7SMI\6SCI\5BURST\4CMD\3IGN2\2IBF\1OBF" + static const struct device_compatible_entry compat_data[] = { { .compat = "PNP0C09" }, DEVICE_COMPAT_EOL }; +#define EC_STATE_ENUM(F) \ + F(EC_STATE_QUERY, "QUERY") \ + F(EC_STATE_QUERY_VAL, "QUERY_VAL") \ + F(EC_STATE_READ, "READ") \ + F(EC_STATE_READ_ADDR, "READ_ADDR") \ + F(EC_STATE_READ_VAL, "READ_VAL") \ + F(EC_STATE_WRITE, "WRITE") \ + F(EC_STATE_WRITE_ADDR, "WRITE_ADDR") \ + F(EC_STATE_WRITE_VAL, "WRITE_VAL") \ + F(EC_STATE_FREE, "FREE") \ + enum ec_state_t { - EC_STATE_QUERY, - EC_STATE_QUERY_VAL, - EC_STATE_READ, - EC_STATE_READ_ADDR, - EC_STATE_READ_VAL, - EC_STATE_WRITE, - EC_STATE_WRITE_ADDR, - EC_STATE_WRITE_VAL, - EC_STATE_FREE +#define F(N, S) N, + EC_STATE_ENUM(F) +#undef F +}; + +#ifdef ACPIEC_DEBUG +static const char *const acpiec_state_names[] = { +#define F(N, S) [N] = S, + EC_STATE_ENUM(F) +#undef F }; +#endif struct acpiec_softc { device_t sc_dev; @@ -144,6 +164,55 @@ struct acpiec_softc { uint8_t sc_cur_addr, sc_cur_val; }; +#ifdef ACPIEC_DEBUG + +#define ACPIEC_DEBUG_ENUM(F) \ + F(ACPIEC_DEBUG_REG, "REG") \ + F(ACPIEC_DEBUG_RW, "RW") \ + F(ACPIEC_DEBUG_QUERY, "QUERY") \ + F(ACPIEC_DEBUG_TRANSITION, "TRANSITION") \ + F(ACPIEC_DEBUG_INTR, "INTR") \ + +enum { +#define F(N, S) N, + ACPIEC_DEBUG_ENUM(F) +#undef F +}; + +static const char *const acpiec_debug_names[] = { +#define F(N, S) [N] = S, + ACPIEC_DEBUG_ENUM(F) +#undef F +}; + +int acpiec_debug = ACPIEC_DEBUG; + +#define DPRINTF(n, sc, fmt, ...) do \ +{ \ + if (acpiec_debug & __BIT(n)) { \ + char dprintbuf[16]; \ + const char *state; \ + \ + /* paranoia */ \ + if ((sc)->sc_state < __arraycount(acpiec_state_names)) { \ + state = acpiec_state_names[(sc)->sc_state]; \ + } else { \ + snprintf(dprintbuf, sizeof(dprintbuf), "0x%x", \ + (sc)->sc_state); \ + state = dprintbuf; \ + } \ + \ + device_printf((sc)->sc_dev, "(%s) [%s] "fmt, \ + acpiec_debug_names[n], state, ##__VA_ARGS__); \ + } \ +} while (0) + +#else + +#define DPRINTF(n, sc, fmt, ...) __nothing + +#endif + static int acpiecdt_match(device_t, cfdata_t, void *); static void acpiecdt_attach(device_t, device_t, void *); @@ -495,24 +564,38 @@ acpiec_parse_gpe_package(device_t self, ACPI_HANDLE ec_handle, static uint8_t acpiec_read_data(struct acpiec_softc *sc) { - return bus_space_read_1(sc->sc_data_st, sc->sc_data_sh, 0); + uint8_t x; + + x = bus_space_read_1(sc->sc_data_st, sc->sc_data_sh, 0); + DPRINTF(ACPIEC_DEBUG_REG, sc, "read data=0x%"PRIx8"\n", x); + + return x; } static void acpiec_write_data(struct acpiec_softc *sc, uint8_t val) { + + DPRINTF(ACPIEC_DEBUG_REG, sc, "write data=0x%"PRIx8"\n", val); bus_space_write_1(sc->sc_data_st, sc->sc_data_sh, 0, val); } static uint8_t acpiec_read_status(struct acpiec_softc *sc) { - return bus_space_read_1(sc->sc_csr_st, sc->sc_csr_sh, 0); + uint8_t x; + + x = bus_space_read_1(sc->sc_csr_st, sc->sc_csr_sh, 0); + DPRINTF(ACPIEC_DEBUG_REG, sc, "read status=0x%"PRIx8"\n", x); + + return x; } static void acpiec_write_command(struct acpiec_softc *sc, uint8_t cmd) { + + DPRINTF(ACPIEC_DEBUG_REG, sc, "write command=0x%"PRIx8"\n", cmd); bus_space_write_1(sc->sc_csr_st, sc->sc_csr_sh, 0, cmd); } @@ -575,6 +658,13 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) acpiec_lock(dv); mutex_enter(&sc->sc_mtx); + DPRINTF(ACPIEC_DEBUG_RW, sc, + "pid %ld %s, lid %ld%s%s: read addr 0x%"PRIx8"\n", + (long)curproc->p_pid, curproc->p_comm, + (long)curlwp->l_lid, curlwp->l_name ? " " : "", + curlwp->l_name ? curlwp->l_name : "", + addr); + sc->sc_cur_addr = addr; sc->sc_state = EC_STATE_READ; @@ -606,6 +696,13 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) } done: + DPRINTF(ACPIEC_DEBUG_RW, sc, + "pid %ld %s, lid %ld%s%s: read addr 0x%"PRIx8": 0x%"PRIx8"\n", + (long)curproc->p_pid, curproc->p_comm, + (long)curlwp->l_lid, curlwp->l_name ? " " : "", + curlwp->l_name ? curlwp->l_name : "", + addr, sc->sc_cur_val); + *val = sc->sc_cur_val; mutex_exit(&sc->sc_mtx); @@ -622,6 +719,13 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) acpiec_lock(dv); mutex_enter(&sc->sc_mtx); + DPRINTF(ACPIEC_DEBUG_RW, sc, + "pid %ld %s, lid %ld%s%s write addr 0x%"PRIx8": 0x%"PRIx8"\n", + (long)curproc->p_pid, curproc->p_comm, + (long)curlwp->l_lid, curlwp->l_name ? " " : "", + curlwp->l_name ? curlwp->l_name : "", + addr, val); + sc->sc_cur_addr = addr; sc->sc_cur_val = val; sc->sc_state = EC_STATE_WRITE; @@ -654,6 +758,14 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) } done: + DPRINTF(ACPIEC_DEBUG_RW, sc, + "pid %ld %s, lid %ld%s%s: write addr 0x%"PRIx8": 0x%"PRIx8 + " done\n", + (long)curproc->p_pid, curproc->p_comm, + (long)curlwp->l_lid, curlwp->l_name ? " " : "", + curlwp->l_name ? curlwp->l_name : "", + addr, val); + mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); return AE_OK; @@ -755,11 +867,14 @@ loop: if (sc->sc_got_sci == false) cv_wait(&sc->sc_cv_sci, &sc->sc_mtx); + DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query requested\n"); mutex_exit(&sc->sc_mtx); acpiec_lock(dv); mutex_enter(&sc->sc_mtx); + DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query\n"); + /* The Query command can always be issued, so be defensive here. */ sc->sc_got_sci = false; sc->sc_state = EC_STATE_QUERY; @@ -771,10 +886,12 @@ loop: delay(1); } + DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI polling timeout\n"); cv_wait(&sc->sc_cv, &sc->sc_mtx); done: reg = sc->sc_cur_val; + DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query: 0x%"PRIx8"\n", reg); mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); @@ -803,6 +920,15 @@ acpiec_gpe_state_machine(device_t dv) reg = acpiec_read_status(sc); +#ifdef ACPIEC_DEBUG + if (acpiec_debug & __BIT(ACPIEC_DEBUG_TRANSITION)) { + char buf[128]; + + snprintb(buf, sizeof(buf), EC_STATUS_FMT, reg); + DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, "%s\n", buf); + } +#endif + if (reg & EC_STATUS_SCI) sc->sc_got_sci = true; @@ -874,15 +1000,22 @@ acpiec_gpe_state_machine(device_t dv) break; case EC_STATE_FREE: - if (sc->sc_got_sci) + if (sc->sc_got_sci) { + DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, + "wake SCI thread\n"); cv_signal(&sc->sc_cv_sci); + } break; default: panic("invalid state"); } - if (sc->sc_state != EC_STATE_FREE) + if (sc->sc_state != EC_STATE_FREE) { + DPRINTF(ACPIEC_DEBUG_INTR, sc, "schedule callout\n"); callout_schedule(&sc->sc_pseudo_intr, 1); + } + + DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, "return\n"); } static void @@ -892,6 +1025,7 @@ acpiec_callout(void *arg) struct acpiec_softc *sc = device_private(dv); mutex_enter(&sc->sc_mtx); + DPRINTF(ACPIEC_DEBUG_INTR, sc, "callout\n"); acpiec_gpe_state_machine(dv); mutex_exit(&sc->sc_mtx); } @@ -903,6 +1037,7 @@ acpiec_gpe_handler(ACPI_HANDLE hdl, uint32_t gpebit, void *arg) struct acpiec_softc *sc = device_private(dv); mutex_enter(&sc->sc_mtx); + DPRINTF(ACPIEC_DEBUG_INTR, sc, "GPE\n"); acpiec_gpe_state_machine(dv); mutex_exit(&sc->sc_mtx); diff --git a/sys/dev/acpi/files.acpi b/sys/dev/acpi/files.acpi index 84df38c63b7e..db2fee6da88c 100644 --- a/sys/dev/acpi/files.acpi +++ b/sys/dev/acpi/files.acpi @@ -45,6 +45,7 @@ device acpiec attach acpiec at acpinodebus device acpiecdt attach acpiecdt at acpiecdtbus +defparam opt_acpi_ec.h ACPIEC_DEBUG file dev/acpi/acpi_ec.c acpiec|acpiecdt # ACPI Lid Switch From 10a20e592cf5c13ba066f632fdebf775a3b0c19f Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 17 Jul 2023 23:53:18 +0000 Subject: [PATCH 03/21] acpiec(4): Clarify lock order and sprinkle lock assertions. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 05c55d039134..f5eb0474d63c 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -34,14 +34,12 @@ * - read and write access from ASL, e.g. to read battery state * - notification of ASL of System Control Interrupts. * - * Access to the EC is serialised by sc_access_mtx and optionally the - * ACPI global mutex. Both locks are held until the request is fulfilled. - * All access to the softc has to hold sc_mtx to serialise against the GPE - * handler and the callout. sc_mtx is also used for wakeup conditions. + * Lock order: + * sc_access_mtx (serializes EC transactions -- read, write, or SCI) + * -> ACPI global lock (excludes other ACPI access during EC transaction) + * -> sc_mtx (serializes state machine transitions and waits) * - * SCIs are processed in a kernel thread. Handling gets a bit complicated - * by the lock order (sc_mtx must be acquired after sc_access_mtx and the - * ACPI global mutex). + * SCIs are processed in a kernel thread. * * Read and write requests spin around for a short time as many requests * can be handled instantly by the EC. During normal processing interrupt @@ -566,6 +564,8 @@ acpiec_read_data(struct acpiec_softc *sc) { uint8_t x; + KASSERT(mutex_owned(&sc->sc_mtx)); + x = bus_space_read_1(sc->sc_data_st, sc->sc_data_sh, 0); DPRINTF(ACPIEC_DEBUG_REG, sc, "read data=0x%"PRIx8"\n", x); @@ -576,6 +576,8 @@ static void acpiec_write_data(struct acpiec_softc *sc, uint8_t val) { + KASSERT(mutex_owned(&sc->sc_mtx)); + DPRINTF(ACPIEC_DEBUG_REG, sc, "write data=0x%"PRIx8"\n", val); bus_space_write_1(sc->sc_data_st, sc->sc_data_sh, 0, val); } @@ -585,6 +587,8 @@ acpiec_read_status(struct acpiec_softc *sc) { uint8_t x; + KASSERT(mutex_owned(&sc->sc_mtx)); + x = bus_space_read_1(sc->sc_csr_st, sc->sc_csr_sh, 0); DPRINTF(ACPIEC_DEBUG_REG, sc, "read status=0x%"PRIx8"\n", x); @@ -595,6 +599,8 @@ static void acpiec_write_command(struct acpiec_softc *sc, uint8_t cmd) { + KASSERT(mutex_owned(&sc->sc_mtx)); + DPRINTF(ACPIEC_DEBUG_REG, sc, "write command=0x%"PRIx8"\n", cmd); bus_space_write_1(sc->sc_csr_st, sc->sc_csr_sh, 0, cmd); } @@ -918,6 +924,8 @@ acpiec_gpe_state_machine(device_t dv) struct acpiec_softc *sc = device_private(dv); uint8_t reg; + KASSERT(mutex_owned(&sc->sc_mtx)); + reg = acpiec_read_status(sc); #ifdef ACPIEC_DEBUG From 520f76ef5124010bbf863baa1c26fac7b0e88431 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:06:51 +0000 Subject: [PATCH 04/21] acpiec(4): Sprinkle comments. Note where this code is abusing cv_wait and needs a loop to handle spurious wakeups. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index f5eb0474d63c..8eb2b83466d8 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -694,6 +694,11 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) return AE_ERROR; } } else if (cv_timedwait(&sc->sc_cv, &sc->sc_mtx, EC_CMD_TIMEOUT * hz)) { + /* + * XXX while (sc->sc_state != EC_STATE_FREE) + * cv_timedwait(...), + * plus deadline + */ mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); aprint_error_dev(dv, @@ -756,6 +761,11 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) return AE_ERROR; } } else if (cv_timedwait(&sc->sc_cv, &sc->sc_mtx, EC_CMD_TIMEOUT * hz)) { + /* + * XXX while (sc->sc_state != EC_STATE_FREE) + * cv_timedwait(...), + * plus deadline + */ mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); aprint_error_dev(dv, @@ -869,13 +879,21 @@ acpiec_gpe_query(void *arg) int i; loop: + /* + * Wait until the EC sends an SCI requesting a query. + * + * XXX This needs to be `while', not `if'. + */ mutex_enter(&sc->sc_mtx); - if (sc->sc_got_sci == false) cv_wait(&sc->sc_cv_sci, &sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query requested\n"); mutex_exit(&sc->sc_mtx); + /* + * EC wants to submit a query to us. Exclude concurrent reads + * and writes while we handle it. + */ acpiec_lock(dv); mutex_enter(&sc->sc_mtx); @@ -893,6 +911,7 @@ loop: } DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI polling timeout\n"); + /* XXX while (sc->sc_state != EC_STATE_FREE) cv_wait(...) */ cv_wait(&sc->sc_cv, &sc->sc_mtx); done: @@ -951,17 +970,14 @@ acpiec_gpe_state_machine(device_t dv) case EC_STATE_QUERY_VAL: if ((reg & EC_STATUS_OBF) == 0) break; /* Nothing of interest here. */ - sc->sc_cur_val = acpiec_read_data(sc); sc->sc_state = EC_STATE_FREE; - cv_signal(&sc->sc_cv); break; case EC_STATE_READ: if ((reg & EC_STATUS_IBF) != 0) break; /* Nothing of interest here. */ - acpiec_write_command(sc, EC_COMMAND_READ); sc->sc_state = EC_STATE_READ_ADDR; break; @@ -969,7 +985,6 @@ acpiec_gpe_state_machine(device_t dv) case EC_STATE_READ_ADDR: if ((reg & EC_STATUS_IBF) != 0) break; /* Nothing of interest here. */ - acpiec_write_data(sc, sc->sc_cur_addr); sc->sc_state = EC_STATE_READ_VAL; break; @@ -979,14 +994,12 @@ acpiec_gpe_state_machine(device_t dv) break; /* Nothing of interest here. */ sc->sc_cur_val = acpiec_read_data(sc); sc->sc_state = EC_STATE_FREE; - cv_signal(&sc->sc_cv); break; case EC_STATE_WRITE: if ((reg & EC_STATUS_IBF) != 0) break; /* Nothing of interest here. */ - acpiec_write_command(sc, EC_COMMAND_WRITE); sc->sc_state = EC_STATE_WRITE_ADDR; break; @@ -1003,7 +1016,6 @@ acpiec_gpe_state_machine(device_t dv) break; /* Nothing of interest here. */ sc->sc_state = EC_STATE_FREE; cv_signal(&sc->sc_cv); - acpiec_write_data(sc, sc->sc_cur_val); break; @@ -1014,10 +1026,15 @@ acpiec_gpe_state_machine(device_t dv) cv_signal(&sc->sc_cv_sci); } break; + default: panic("invalid state"); } + /* + * In case GPE interrupts are broken, poll once per tick for EC + * status updates while a transaction is still pending. + */ if (sc->sc_state != EC_STATE_FREE) { DPRINTF(ACPIEC_DEBUG_INTR, sc, "schedule callout\n"); callout_schedule(&sc->sc_pseudo_intr, 1); From bbbfaac138cafcab92d56cb58c50c4d88bf719d8 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:08:50 +0000 Subject: [PATCH 05/21] acpiec(4): Assert state is free when we start a transaction. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 8eb2b83466d8..f08fa371d01d 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -671,6 +671,8 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) curlwp->l_name ? curlwp->l_name : "", addr); + KASSERT(sc->sc_state == EC_STATE_FREE); + sc->sc_cur_addr = addr; sc->sc_state = EC_STATE_READ; @@ -737,6 +739,8 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) curlwp->l_name ? curlwp->l_name : "", addr, val); + KASSERT(sc->sc_state == EC_STATE_FREE); + sc->sc_cur_addr = addr; sc->sc_cur_val = val; sc->sc_state = EC_STATE_WRITE; @@ -899,6 +903,8 @@ loop: DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query\n"); + KASSERT(sc->sc_state == EC_STATE_FREE); + /* The Query command can always be issued, so be defensive here. */ sc->sc_got_sci = false; sc->sc_state = EC_STATE_QUERY; From b92f80824a8be3dd841256e8eac6710f2dd6eff7 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:11:46 +0000 Subject: [PATCH 06/21] acpiec(4): Set sc_got_sci only when a transaction is over. Before, when the acpiec thread noticed an SCI had been requested and entered acpiec_gpe_state_machine to send the query command, it would see the SCI is still requested -- because it had yet to acknowledge it by setting the query command! -- and think the EC was asking for a _second_ SCI. So once the first SCI transaction was over, it would start a second one, even though the EC hadn't asked for another -- and this would wedge on some ECs. Now, acpiec_gpe_state_machine waits to see what state we transition to before taking the SCI bit to mean we need to notify the acpiec thread to handle another query. That way, when the acpiec thread enters acpiec_gpe_state_machine with EC_STATE_QUERY, it can send the query command first, with the side effect of clearing the SCI bit in subsequent reads of the status register, and it won't think another SCI has been requested until it returns to EC_STATE_FREE and sees the SCI bit set again in the status register. Possibly relevant PRs: PR kern/53135 PR kern/52763 PR kern/57162 --- sys/dev/acpi/acpi_ec.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index f08fa371d01d..62944220778a 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -962,9 +962,6 @@ acpiec_gpe_state_machine(device_t dv) } #endif - if (reg & EC_STATUS_SCI) - sc->sc_got_sci = true; - switch (sc->sc_state) { case EC_STATE_QUERY: if ((reg & EC_STATUS_IBF) != 0) @@ -1026,17 +1023,25 @@ acpiec_gpe_state_machine(device_t dv) break; case EC_STATE_FREE: - if (sc->sc_got_sci) { - DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, - "wake SCI thread\n"); - cv_signal(&sc->sc_cv_sci); - } break; default: panic("invalid state"); } + /* + * If we just ended a transaction, and an SCI was requested, + * notify the SCI thread. + */ + if (sc->sc_state == EC_STATE_FREE) { + if (reg & EC_STATUS_SCI) { + DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, + "wake SCI thread\n"); + sc->sc_got_sci = true; + cv_signal(&sc->sc_cv_sci); + } + } + /* * In case GPE interrupts are broken, poll once per tick for EC * status updates while a transaction is still pending. From ce75e9b0e6a2f09c79492f9ee3e29a6dbe59bd50 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:24:46 +0000 Subject: [PATCH 07/21] acpiec(4): Fix cv_wait loop around sc->sc_got_sci. That is, make it actually loop as required, so it gracefully handles spurious wakeups instead of barging into invalid states. --- sys/dev/acpi/acpi_ec.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 62944220778a..2faf5d1be81b 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -885,11 +885,9 @@ acpiec_gpe_query(void *arg) loop: /* * Wait until the EC sends an SCI requesting a query. - * - * XXX This needs to be `while', not `if'. */ mutex_enter(&sc->sc_mtx); - if (sc->sc_got_sci == false) + while (!sc->sc_got_sci) cv_wait(&sc->sc_cv_sci, &sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query requested\n"); mutex_exit(&sc->sc_mtx); @@ -906,6 +904,7 @@ loop: KASSERT(sc->sc_state == EC_STATE_FREE); /* The Query command can always be issued, so be defensive here. */ + KASSERT(sc->sc_got_sci); sc->sc_got_sci = false; sc->sc_state = EC_STATE_QUERY; From 5957f6a70e01398cb1d3df46731e7198fd029bea Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:29:01 +0000 Subject: [PATCH 08/21] acpiec(4): Fix interrupt wait loop in acpiec_gpe_query thread. --- sys/dev/acpi/acpi_ec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 2faf5d1be81b..7ce241e1fe73 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -908,6 +908,9 @@ loop: sc->sc_got_sci = false; sc->sc_state = EC_STATE_QUERY; + /* + * First, attempt to get the query by polling. + */ for (i = 0; i < EC_POLL_TIMEOUT; ++i) { acpiec_gpe_state_machine(dv); if (sc->sc_state == EC_STATE_FREE) @@ -915,9 +918,14 @@ loop: delay(1); } + /* + * Polling timed out. Try waiting for interrupts -- either GPE + * interrupts, or periodic callouts in case GPE interrupts are + * broken. + */ DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI polling timeout\n"); - /* XXX while (sc->sc_state != EC_STATE_FREE) cv_wait(...) */ - cv_wait(&sc->sc_cv, &sc->sc_mtx); + while (sc->sc_state != EC_STATE_FREE) + cv_wait(&sc->sc_cv, &sc->sc_mtx); done: reg = sc->sc_cur_val; From 4594b0950704e09ee6c6f44b1395cab14bbe1cef Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:39:59 +0000 Subject: [PATCH 09/21] acpiec(4): Fix cv_timedwait abuse in acpiec_read/write. --- sys/dev/acpi/acpi_ec.c | 52 ++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 7ce241e1fe73..6f6359082263 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -695,17 +695,21 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) sc->sc_state); return AE_ERROR; } - } else if (cv_timedwait(&sc->sc_cv, &sc->sc_mtx, EC_CMD_TIMEOUT * hz)) { - /* - * XXX while (sc->sc_state != EC_STATE_FREE) - * cv_timedwait(...), - * plus deadline - */ - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); - aprint_error_dev(dv, - "command takes over %d sec...\n", EC_CMD_TIMEOUT); - return AE_ERROR; + } else { + const unsigned deadline = getticks() + EC_CMD_TIMEOUT*hz; + unsigned delta; + + while (sc->sc_state != EC_STATE_FREE && + (delta = deadline - getticks()) < INT_MAX) + (void)cv_timedwait(&sc->sc_cv, &sc->sc_mtx, delta); + if (sc->sc_state != EC_STATE_FREE) { + mutex_exit(&sc->sc_mtx); + acpiec_unlock(dv); + aprint_error_dev(dv, + "command takes over %d sec...\n", + EC_CMD_TIMEOUT); + return AE_ERROR; + } } done: @@ -764,17 +768,21 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) sc->sc_state); return AE_ERROR; } - } else if (cv_timedwait(&sc->sc_cv, &sc->sc_mtx, EC_CMD_TIMEOUT * hz)) { - /* - * XXX while (sc->sc_state != EC_STATE_FREE) - * cv_timedwait(...), - * plus deadline - */ - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); - aprint_error_dev(dv, - "command takes over %d sec...\n", EC_CMD_TIMEOUT); - return AE_ERROR; + } else { + const unsigned deadline = getticks() + EC_CMD_TIMEOUT*hz; + unsigned delta; + + while (sc->sc_state != EC_STATE_FREE && + (delta = deadline - getticks()) < INT_MAX) + (void)cv_timedwait(&sc->sc_cv, &sc->sc_mtx, delta); + if (sc->sc_state != EC_STATE_FREE) { + mutex_exit(&sc->sc_mtx); + acpiec_unlock(dv); + aprint_error_dev(dv, + "command takes over %d sec...\n", + EC_CMD_TIMEOUT); + return AE_ERROR; + } } done: From f6f4d572d8bbc95f2206297d01a37c535d40bdb7 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:43:00 +0000 Subject: [PATCH 10/21] acpiec(4): Don't touch sc->sc_state outside sc->sc_mtx. --- sys/dev/acpi/acpi_ec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 6f6359082263..c5965aed26ba 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -689,10 +689,10 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) acpiec_gpe_state_machine(dv); } if (sc->sc_state != EC_STATE_FREE) { - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); aprint_error_dev(dv, "command timed out, state %d\n", sc->sc_state); + mutex_exit(&sc->sc_mtx); + acpiec_unlock(dv); return AE_ERROR; } } else { @@ -762,10 +762,10 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) acpiec_gpe_state_machine(dv); } if (sc->sc_state != EC_STATE_FREE) { - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); aprint_error_dev(dv, "command timed out, state %d\n", sc->sc_state); + mutex_exit(&sc->sc_mtx); + acpiec_unlock(dv); return AE_ERROR; } } else { From c61cd2a85114874b24bbd4e43eb23396b031c904 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:45:39 +0000 Subject: [PATCH 11/21] acpiec(4): Merge returns in acpiec_read/write. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index c5965aed26ba..6b20efa13d2b 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -660,6 +660,7 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) { struct acpiec_softc *sc = device_private(dv); int i, timeo = 1000 * EC_CMD_TIMEOUT; + ACPI_STATUS rv; acpiec_lock(dv); mutex_enter(&sc->sc_mtx); @@ -691,9 +692,8 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) if (sc->sc_state != EC_STATE_FREE) { aprint_error_dev(dv, "command timed out, state %d\n", sc->sc_state); - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); - return AE_ERROR; + rv = AE_ERROR; + goto out; } } else { const unsigned deadline = getticks() + EC_CMD_TIMEOUT*hz; @@ -703,12 +703,11 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) (delta = deadline - getticks()) < INT_MAX) (void)cv_timedwait(&sc->sc_cv, &sc->sc_mtx, delta); if (sc->sc_state != EC_STATE_FREE) { - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); aprint_error_dev(dv, "command takes over %d sec...\n", EC_CMD_TIMEOUT); - return AE_ERROR; + rv = AE_ERROR; + goto out; } } @@ -721,10 +720,11 @@ done: addr, sc->sc_cur_val); *val = sc->sc_cur_val; - + rv = AE_OK; +out: mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); - return AE_OK; + return rv; } static ACPI_STATUS @@ -732,6 +732,7 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) { struct acpiec_softc *sc = device_private(dv); int i, timeo = 1000 * EC_CMD_TIMEOUT; + ACPI_STATUS rv; acpiec_lock(dv); mutex_enter(&sc->sc_mtx); @@ -764,9 +765,8 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) if (sc->sc_state != EC_STATE_FREE) { aprint_error_dev(dv, "command timed out, state %d\n", sc->sc_state); - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); - return AE_ERROR; + rv = AE_ERROR; + goto out; } } else { const unsigned deadline = getticks() + EC_CMD_TIMEOUT*hz; @@ -776,12 +776,11 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) (delta = deadline - getticks()) < INT_MAX) (void)cv_timedwait(&sc->sc_cv, &sc->sc_mtx, delta); if (sc->sc_state != EC_STATE_FREE) { - mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); aprint_error_dev(dv, "command takes over %d sec...\n", EC_CMD_TIMEOUT); - return AE_ERROR; + rv = AE_ERROR; + goto out; } } @@ -793,10 +792,11 @@ done: (long)curlwp->l_lid, curlwp->l_name ? " " : "", curlwp->l_name ? curlwp->l_name : "", addr, val); - + rv = AE_OK; +out: mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); - return AE_OK; + return rv; } /* From aac4b7523db0b7d92cd5c99888a7b37ebbf28972 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:53:33 +0000 Subject: [PATCH 12/21] acpiec(4): Factor wait logic out. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 156 ++++++++++++++++++----------------------- 1 file changed, 70 insertions(+), 86 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 6b20efa13d2b..4e43a4f01e38 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -656,35 +656,21 @@ acpiec_unlock(device_t dv) } static ACPI_STATUS -acpiec_read(device_t dv, uint8_t addr, uint8_t *val) +acpiec_wait_timeout(struct acpiec_softc *sc) { - struct acpiec_softc *sc = device_private(dv); - int i, timeo = 1000 * EC_CMD_TIMEOUT; - ACPI_STATUS rv; - - acpiec_lock(dv); - mutex_enter(&sc->sc_mtx); - - DPRINTF(ACPIEC_DEBUG_RW, sc, - "pid %ld %s, lid %ld%s%s: read addr 0x%"PRIx8"\n", - (long)curproc->p_pid, curproc->p_comm, - (long)curlwp->l_lid, curlwp->l_name ? " " : "", - curlwp->l_name ? curlwp->l_name : "", - addr); - - KASSERT(sc->sc_state == EC_STATE_FREE); - - sc->sc_cur_addr = addr; - sc->sc_state = EC_STATE_READ; + device_t dv = sc->sc_dev; + int i; for (i = 0; i < EC_POLL_TIMEOUT; ++i) { acpiec_gpe_state_machine(dv); if (sc->sc_state == EC_STATE_FREE) - goto done; + return AE_OK; delay(1); } if (cold || acpiec_cold) { + int timeo = 1000 * EC_CMD_TIMEOUT; + while (sc->sc_state != EC_STATE_FREE && timeo-- > 0) { delay(1000); acpiec_gpe_state_machine(dv); @@ -692,8 +678,7 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) if (sc->sc_state != EC_STATE_FREE) { aprint_error_dev(dv, "command timed out, state %d\n", sc->sc_state); - rv = AE_ERROR; - goto out; + return AE_ERROR; } } else { const unsigned deadline = getticks() + EC_CMD_TIMEOUT*hz; @@ -706,12 +691,38 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) aprint_error_dev(dv, "command takes over %d sec...\n", EC_CMD_TIMEOUT); - rv = AE_ERROR; - goto out; + return AE_ERROR; } } -done: + return AE_OK; +} + +static ACPI_STATUS +acpiec_read(device_t dv, uint8_t addr, uint8_t *val) +{ + struct acpiec_softc *sc = device_private(dv); + ACPI_STATUS rv; + + acpiec_lock(dv); + mutex_enter(&sc->sc_mtx); + + DPRINTF(ACPIEC_DEBUG_RW, sc, + "pid %ld %s, lid %ld%s%s: read addr 0x%"PRIx8"\n", + (long)curproc->p_pid, curproc->p_comm, + (long)curlwp->l_lid, curlwp->l_name ? " " : "", + curlwp->l_name ? curlwp->l_name : "", + addr); + + KASSERT(sc->sc_state == EC_STATE_FREE); + + sc->sc_cur_addr = addr; + sc->sc_state = EC_STATE_READ; + + rv = acpiec_wait_timeout(sc); + if (ACPI_FAILURE(rv)) + goto out; + DPRINTF(ACPIEC_DEBUG_RW, sc, "pid %ld %s, lid %ld%s%s: read addr 0x%"PRIx8": 0x%"PRIx8"\n", (long)curproc->p_pid, curproc->p_comm, @@ -720,9 +731,8 @@ done: addr, sc->sc_cur_val); *val = sc->sc_cur_val; - rv = AE_OK; -out: - mutex_exit(&sc->sc_mtx); + +out: mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); return rv; } @@ -731,7 +741,6 @@ static ACPI_STATUS acpiec_write(device_t dv, uint8_t addr, uint8_t val) { struct acpiec_softc *sc = device_private(dv); - int i, timeo = 1000 * EC_CMD_TIMEOUT; ACPI_STATUS rv; acpiec_lock(dv); @@ -750,41 +759,10 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) sc->sc_cur_val = val; sc->sc_state = EC_STATE_WRITE; - for (i = 0; i < EC_POLL_TIMEOUT; ++i) { - acpiec_gpe_state_machine(dv); - if (sc->sc_state == EC_STATE_FREE) - goto done; - delay(1); - } - - if (cold || acpiec_cold) { - while (sc->sc_state != EC_STATE_FREE && timeo-- > 0) { - delay(1000); - acpiec_gpe_state_machine(dv); - } - if (sc->sc_state != EC_STATE_FREE) { - aprint_error_dev(dv, "command timed out, state %d\n", - sc->sc_state); - rv = AE_ERROR; - goto out; - } - } else { - const unsigned deadline = getticks() + EC_CMD_TIMEOUT*hz; - unsigned delta; - - while (sc->sc_state != EC_STATE_FREE && - (delta = deadline - getticks()) < INT_MAX) - (void)cv_timedwait(&sc->sc_cv, &sc->sc_mtx, delta); - if (sc->sc_state != EC_STATE_FREE) { - aprint_error_dev(dv, - "command takes over %d sec...\n", - EC_CMD_TIMEOUT); - rv = AE_ERROR; - goto out; - } - } + rv = acpiec_wait_timeout(sc); + if (ACPI_FAILURE(rv)) + goto out; -done: DPRINTF(ACPIEC_DEBUG_RW, sc, "pid %ld %s, lid %ld%s%s: write addr 0x%"PRIx8": 0x%"PRIx8 " done\n", @@ -792,9 +770,8 @@ done: (long)curlwp->l_lid, curlwp->l_name ? " " : "", curlwp->l_name ? curlwp->l_name : "", addr, val); - rv = AE_OK; -out: - mutex_exit(&sc->sc_mtx); + +out: mutex_exit(&sc->sc_mtx); acpiec_unlock(dv); return rv; } @@ -880,6 +857,32 @@ acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr, return rv; } +static void +acpiec_wait(struct acpiec_softc *sc) +{ + device_t dv = sc->sc_dev; + int i; + + /* + * First, attempt to get the query by polling. + */ + for (i = 0; i < EC_POLL_TIMEOUT; ++i) { + acpiec_gpe_state_machine(dv); + if (sc->sc_state == EC_STATE_FREE) + return; + delay(1); + } + + /* + * Polling timed out. Try waiting for interrupts -- either GPE + * interrupts, or periodic callouts in case GPE interrupts are + * broken. + */ + DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI polling timeout\n"); + while (sc->sc_state != EC_STATE_FREE) + cv_wait(&sc->sc_cv, &sc->sc_mtx); +} + static void acpiec_gpe_query(void *arg) { @@ -888,7 +891,6 @@ acpiec_gpe_query(void *arg) uint8_t reg; char qxx[5]; ACPI_STATUS rv; - int i; loop: /* @@ -916,26 +918,8 @@ loop: sc->sc_got_sci = false; sc->sc_state = EC_STATE_QUERY; - /* - * First, attempt to get the query by polling. - */ - for (i = 0; i < EC_POLL_TIMEOUT; ++i) { - acpiec_gpe_state_machine(dv); - if (sc->sc_state == EC_STATE_FREE) - goto done; - delay(1); - } - - /* - * Polling timed out. Try waiting for interrupts -- either GPE - * interrupts, or periodic callouts in case GPE interrupts are - * broken. - */ - DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI polling timeout\n"); - while (sc->sc_state != EC_STATE_FREE) - cv_wait(&sc->sc_cv, &sc->sc_mtx); + acpiec_wait(sc); -done: reg = sc->sc_cur_val; DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query: 0x%"PRIx8"\n", reg); From 74d47f1b22e0de7a6543bd000403e578694fb362 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:55:32 +0000 Subject: [PATCH 13/21] acpiec(4): Pass softc, not device_t, to acpiec_gpe_state_machine. Simpler, type-safer. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 4e43a4f01e38..0f58a6ccd9af 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -235,7 +235,7 @@ static ACPI_STATUS acpiec_space_setup(ACPI_HANDLE, uint32_t, void *, void **); static ACPI_STATUS acpiec_space_handler(uint32_t, ACPI_PHYSICAL_ADDRESS, uint32_t, ACPI_INTEGER *, void *, void *); -static void acpiec_gpe_state_machine(device_t); +static void acpiec_gpe_state_machine(struct acpiec_softc *); CFATTACH_DECL_NEW(acpiec, sizeof(struct acpiec_softc), acpiec_match, acpiec_attach, NULL, NULL); @@ -662,7 +662,7 @@ acpiec_wait_timeout(struct acpiec_softc *sc) int i; for (i = 0; i < EC_POLL_TIMEOUT; ++i) { - acpiec_gpe_state_machine(dv); + acpiec_gpe_state_machine(sc); if (sc->sc_state == EC_STATE_FREE) return AE_OK; delay(1); @@ -673,7 +673,7 @@ acpiec_wait_timeout(struct acpiec_softc *sc) while (sc->sc_state != EC_STATE_FREE && timeo-- > 0) { delay(1000); - acpiec_gpe_state_machine(dv); + acpiec_gpe_state_machine(sc); } if (sc->sc_state != EC_STATE_FREE) { aprint_error_dev(dv, "command timed out, state %d\n", @@ -860,14 +860,13 @@ acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr, static void acpiec_wait(struct acpiec_softc *sc) { - device_t dv = sc->sc_dev; int i; /* * First, attempt to get the query by polling. */ for (i = 0; i < EC_POLL_TIMEOUT; ++i) { - acpiec_gpe_state_machine(dv); + acpiec_gpe_state_machine(sc); if (sc->sc_state == EC_STATE_FREE) return; delay(1); @@ -943,9 +942,8 @@ loop: } static void -acpiec_gpe_state_machine(device_t dv) +acpiec_gpe_state_machine(struct acpiec_softc *sc) { - struct acpiec_softc *sc = device_private(dv); uint8_t reg; KASSERT(mutex_owned(&sc->sc_mtx)); @@ -1061,7 +1059,7 @@ acpiec_callout(void *arg) mutex_enter(&sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_INTR, sc, "callout\n"); - acpiec_gpe_state_machine(dv); + acpiec_gpe_state_machine(sc); mutex_exit(&sc->sc_mtx); } @@ -1073,7 +1071,7 @@ acpiec_gpe_handler(ACPI_HANDLE hdl, uint32_t gpebit, void *arg) mutex_enter(&sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_INTR, sc, "GPE\n"); - acpiec_gpe_state_machine(dv); + acpiec_gpe_state_machine(sc); mutex_exit(&sc->sc_mtx); return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE; From 36c3907219d54930c9f7b824911ce6dfd9c20cff Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:57:13 +0000 Subject: [PATCH 14/21] acpiec(4): Pass softc, not device_t, to acpiec_callout. Simpler. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 0f58a6ccd9af..dd66d889df47 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -422,7 +422,7 @@ acpiec_common_attach(device_t parent, device_t self, aprint_normal_dev(self, "using global ACPI lock\n"); callout_init(&sc->sc_pseudo_intr, CALLOUT_MPSAFE); - callout_setfunc(&sc->sc_pseudo_intr, acpiec_callout, self); + callout_setfunc(&sc->sc_pseudo_intr, acpiec_callout, sc); rv = AcpiInstallAddressSpaceHandler(sc->sc_ech, ACPI_ADR_SPACE_EC, acpiec_space_handler, acpiec_space_setup, self); @@ -1054,8 +1054,7 @@ acpiec_gpe_state_machine(struct acpiec_softc *sc) static void acpiec_callout(void *arg) { - device_t dv = arg; - struct acpiec_softc *sc = device_private(dv); + struct acpiec_softc *sc = arg; mutex_enter(&sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_INTR, sc, "callout\n"); From 9b3659282efdf806a2172615bf7b60abd5860bf6 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 00:58:18 +0000 Subject: [PATCH 15/21] acpiec(4): Pass softc, not device_t, to acpiec_gpe_handler. Simpler. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index dd66d889df47..3d03d8585277 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -434,7 +434,7 @@ acpiec_common_attach(device_t parent, device_t self, } rv = AcpiInstallGpeHandler(sc->sc_gpeh, sc->sc_gpebit, - ACPI_GPE_EDGE_TRIGGERED, acpiec_gpe_handler, self); + ACPI_GPE_EDGE_TRIGGERED, acpiec_gpe_handler, sc); if (rv != AE_OK) { aprint_error_dev(self, "unable to install GPE handler: %s\n", AcpiFormatException(rv)); @@ -1065,8 +1065,7 @@ acpiec_callout(void *arg) static uint32_t acpiec_gpe_handler(ACPI_HANDLE hdl, uint32_t gpebit, void *arg) { - device_t dv = arg; - struct acpiec_softc *sc = device_private(dv); + struct acpiec_softc *sc = arg; mutex_enter(&sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_INTR, sc, "GPE\n"); From 62f5141486d79cfacad6dd476fcd0d066beb8520 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 01:00:10 +0000 Subject: [PATCH 16/21] acpiec(4): Pass softc, not device_t, to acpiec_lock/unlock. Simpler, type-safer. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 3d03d8585277..aa4e438861ae 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -619,9 +619,8 @@ acpiec_space_setup(ACPI_HANDLE region, uint32_t func, void *arg, } static void -acpiec_lock(device_t dv) +acpiec_lock(struct acpiec_softc *sc) { - struct acpiec_softc *sc = device_private(dv); ACPI_STATUS rv; mutex_enter(&sc->sc_access_mtx); @@ -630,7 +629,7 @@ acpiec_lock(device_t dv) rv = AcpiAcquireGlobalLock(EC_LOCK_TIMEOUT, &sc->sc_global_lock); if (rv != AE_OK) { - aprint_error_dev(dv, + aprint_error_dev(sc->sc_dev, "failed to acquire global lock: %s\n", AcpiFormatException(rv)); return; @@ -639,15 +638,14 @@ acpiec_lock(device_t dv) } static void -acpiec_unlock(device_t dv) +acpiec_unlock(struct acpiec_softc *sc) { - struct acpiec_softc *sc = device_private(dv); ACPI_STATUS rv; if (sc->sc_need_global_lock) { rv = AcpiReleaseGlobalLock(sc->sc_global_lock); if (rv != AE_OK) { - aprint_error_dev(dv, + aprint_error_dev(sc->sc_dev, "failed to release global lock: %s\n", AcpiFormatException(rv)); } @@ -704,7 +702,7 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) struct acpiec_softc *sc = device_private(dv); ACPI_STATUS rv; - acpiec_lock(dv); + acpiec_lock(sc); mutex_enter(&sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_RW, sc, @@ -733,7 +731,7 @@ acpiec_read(device_t dv, uint8_t addr, uint8_t *val) *val = sc->sc_cur_val; out: mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); + acpiec_unlock(sc); return rv; } @@ -743,7 +741,7 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) struct acpiec_softc *sc = device_private(dv); ACPI_STATUS rv; - acpiec_lock(dv); + acpiec_lock(sc); mutex_enter(&sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_RW, sc, @@ -772,7 +770,7 @@ acpiec_write(device_t dv, uint8_t addr, uint8_t val) addr, val); out: mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); + acpiec_unlock(sc); return rv; } @@ -905,7 +903,7 @@ loop: * EC wants to submit a query to us. Exclude concurrent reads * and writes while we handle it. */ - acpiec_lock(dv); + acpiec_lock(sc); mutex_enter(&sc->sc_mtx); DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query\n"); @@ -923,7 +921,7 @@ loop: DPRINTF(ACPIEC_DEBUG_QUERY, sc, "SCI query: 0x%"PRIx8"\n", reg); mutex_exit(&sc->sc_mtx); - acpiec_unlock(dv); + acpiec_unlock(sc); if (reg == 0) goto loop; /* Spurious query result */ From c565ac38285e353367614bfbedfc80b2e2ae5781 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 01:02:49 +0000 Subject: [PATCH 17/21] acpiec(4): Pass softc, not device_t, to acpiec_read/write. Simpler, type-safer. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index aa4e438861ae..de8dca5c7c0f 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -697,9 +697,8 @@ acpiec_wait_timeout(struct acpiec_softc *sc) } static ACPI_STATUS -acpiec_read(device_t dv, uint8_t addr, uint8_t *val) +acpiec_read(struct acpiec_softc *sc, uint8_t addr, uint8_t *val) { - struct acpiec_softc *sc = device_private(dv); ACPI_STATUS rv; acpiec_lock(sc); @@ -736,9 +735,8 @@ out: mutex_exit(&sc->sc_mtx); } static ACPI_STATUS -acpiec_write(device_t dv, uint8_t addr, uint8_t val) +acpiec_write(struct acpiec_softc *sc, uint8_t addr, uint8_t val) { - struct acpiec_softc *sc = device_private(dv); ACPI_STATUS rv; acpiec_lock(sc); @@ -810,7 +808,8 @@ static ACPI_STATUS acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr, uint32_t width, ACPI_INTEGER *value, void *arg, void *region_arg) { - device_t dv; + device_t dv = arg; + struct acpiec_softc *sc = device_private(dv); ACPI_STATUS rv; uint8_t addr, *buf; unsigned int i; @@ -820,7 +819,6 @@ acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr, return AE_BAD_PARAMETER; addr = paddr; - dv = arg; buf = (uint8_t *)value; rv = AE_OK; @@ -828,7 +826,7 @@ acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr, switch (func) { case ACPI_READ: for (i = 0; i < width; i += 8, ++addr, ++buf) { - rv = acpiec_read(dv, addr, buf); + rv = acpiec_read(sc, addr, buf); if (rv != AE_OK) break; } @@ -841,14 +839,15 @@ acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr, break; case ACPI_WRITE: for (i = 0; i < width; i += 8, ++addr, ++buf) { - rv = acpiec_write(dv, addr, *buf); + rv = acpiec_write(sc, addr, *buf); if (rv != AE_OK) break; } break; default: - aprint_error("%s: invalid Address Space function called: %x\n", - device_xname(dv), (unsigned int)func); + aprint_error_dev(sc->sc_dev, + "invalid Address Space function called: %x\n", + (unsigned int)func); return AE_BAD_PARAMETER; } From ea2a7f4d633d41fe45d18900c56f11c55723894d Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 01:05:11 +0000 Subject: [PATCH 18/21] acpiec(4): Pass softc, not device_t, to acpiec_gpe_query thread. Simpler. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index de8dca5c7c0f..7fc03140527a 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -449,7 +449,7 @@ acpiec_common_attach(device_t parent, device_t self, } if (kthread_create(PRI_NONE, KTHREAD_MPSAFE, NULL, acpiec_gpe_query, - self, NULL, "acpiec sci thread")) { + sc, NULL, "acpiec sci thread")) { aprint_error_dev(self, "unable to create query kthread\n"); goto post_csr_map; } @@ -882,8 +882,7 @@ acpiec_wait(struct acpiec_softc *sc) static void acpiec_gpe_query(void *arg) { - device_t dv = arg; - struct acpiec_softc *sc = device_private(dv); + struct acpiec_softc *sc = arg; uint8_t reg; char qxx[5]; ACPI_STATUS rv; @@ -931,7 +930,7 @@ loop: snprintf(qxx, sizeof(qxx), "_Q%02X", (unsigned int)reg); rv = AcpiEvaluateObject(sc->sc_ech, qxx, NULL, NULL); if (rv != AE_OK && rv != AE_NOT_FOUND) { - aprint_error_dev(dv, "GPE query method %s failed: %s", + aprint_error_dev(sc->sc_dev, "GPE query method %s failed: %s", qxx, AcpiFormatException(rv)); } From 45bbd2d583d6a317f1c2c605852e0dcfa0e571c2 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 01:06:15 +0000 Subject: [PATCH 19/21] acpiec(4): Pass softc, not device_t, to acpiec_space_handler. Better to keep the device_t isolated to public interfaces. Simpler internally this way. No functional change intended. --- sys/dev/acpi/acpi_ec.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 7fc03140527a..914f05710086 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -425,7 +425,7 @@ acpiec_common_attach(device_t parent, device_t self, callout_setfunc(&sc->sc_pseudo_intr, acpiec_callout, sc); rv = AcpiInstallAddressSpaceHandler(sc->sc_ech, ACPI_ADR_SPACE_EC, - acpiec_space_handler, acpiec_space_setup, self); + acpiec_space_handler, acpiec_space_setup, sc); if (rv != AE_OK) { aprint_error_dev(self, "unable to install address space handler: %s\n", @@ -777,8 +777,8 @@ out: mutex_exit(&sc->sc_mtx); * * Transfer bitwidth/8 bytes of data between paddr and *value: * from paddr to *value when func is ACPI_READ, and the other way - * when func is ACPI_WRITE. arg is the acpiec(4) or acpiecdt(4) - * device. region_arg is ignored (XXX why? determined by + * when func is ACPI_WRITE. arg is the acpiec_softc pointer. + * region_arg is ignored (XXX why? determined by * acpiec_space_setup but never used by anything that I can see). * * The caller always provides storage at *value large enough for @@ -808,8 +808,7 @@ static ACPI_STATUS acpiec_space_handler(uint32_t func, ACPI_PHYSICAL_ADDRESS paddr, uint32_t width, ACPI_INTEGER *value, void *arg, void *region_arg) { - device_t dv = arg; - struct acpiec_softc *sc = device_private(dv); + struct acpiec_softc *sc = arg; ACPI_STATUS rv; uint8_t addr, *buf; unsigned int i; @@ -1074,13 +1073,17 @@ acpiec_gpe_handler(ACPI_HANDLE hdl, uint32_t gpebit, void *arg) ACPI_STATUS acpiec_bus_read(device_t dv, u_int addr, ACPI_INTEGER *val, int width) { - return acpiec_space_handler(ACPI_READ, addr, width * 8, val, dv, NULL); + struct acpiec_softc *sc = device_private(dv); + + return acpiec_space_handler(ACPI_READ, addr, width * 8, val, sc, NULL); } ACPI_STATUS acpiec_bus_write(device_t dv, u_int addr, ACPI_INTEGER val, int width) { - return acpiec_space_handler(ACPI_WRITE, addr, width * 8, &val, dv, + struct acpiec_softc *sc = device_private(dv); + + return acpiec_space_handler(ACPI_WRITE, addr, width * 8, &val, sc, NULL); } From a279c195d794235564e75c54d7cefe3dfddd7af3 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 01:59:57 +0000 Subject: [PATCH 20/21] acpiec(4): Factor out if (state == FREE) cv_signal(sc_cv). No functional change intended. --- sys/dev/acpi/acpi_ec.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 914f05710086..7125396776e5 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -967,7 +967,6 @@ acpiec_gpe_state_machine(struct acpiec_softc *sc) break; /* Nothing of interest here. */ sc->sc_cur_val = acpiec_read_data(sc); sc->sc_state = EC_STATE_FREE; - cv_signal(&sc->sc_cv); break; case EC_STATE_READ: @@ -989,7 +988,6 @@ acpiec_gpe_state_machine(struct acpiec_softc *sc) break; /* Nothing of interest here. */ sc->sc_cur_val = acpiec_read_data(sc); sc->sc_state = EC_STATE_FREE; - cv_signal(&sc->sc_cv); break; case EC_STATE_WRITE: @@ -1009,9 +1007,8 @@ acpiec_gpe_state_machine(struct acpiec_softc *sc) case EC_STATE_WRITE_VAL: if ((reg & EC_STATUS_IBF) != 0) break; /* Nothing of interest here. */ - sc->sc_state = EC_STATE_FREE; - cv_signal(&sc->sc_cv); acpiec_write_data(sc, sc->sc_cur_val); + sc->sc_state = EC_STATE_FREE; break; case EC_STATE_FREE: @@ -1022,10 +1019,12 @@ acpiec_gpe_state_machine(struct acpiec_softc *sc) } /* - * If we just ended a transaction, and an SCI was requested, - * notify the SCI thread. + * If we are not in a transaction, wake anyone waiting to start + * one. If an SCI was requested, notify the SCI thread that it + * needs to handle the SCI. */ if (sc->sc_state == EC_STATE_FREE) { + cv_signal(&sc->sc_cv); if (reg & EC_STATUS_SCI) { DPRINTF(ACPIEC_DEBUG_TRANSITION, sc, "wake SCI thread\n"); From 1f5c7491730ecc4a8ac21c74dedfc49443592741 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Tue, 18 Jul 2023 01:10:02 +0000 Subject: [PATCH 21/21] acpiec(4): Take a lock around acpiec_cold updates. Otherwise we race with readers -- probably harmlessly, but let's avoid the appearance of problems. XXX Maybe acpiec_suspend and acpiec_shutdown should interrupt transactions and force them to fail promptly? XXX This looks bad because acpiec_cold is global and sc->sc_mtx doesn't look like it's global, but we expect to have only one acpiec(4) device anyway from what I understand. Maybe we should move acpiec_cold into the softc? --- sys/dev/acpi/acpi_ec.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sys/dev/acpi/acpi_ec.c b/sys/dev/acpi/acpi_ec.c index 7125396776e5..29b524d74b69 100644 --- a/sys/dev/acpi/acpi_ec.c +++ b/sys/dev/acpi/acpi_ec.c @@ -477,8 +477,11 @@ post_data_map: static bool acpiec_suspend(device_t dv, const pmf_qual_t *qual) { + struct acpiec_softc *sc = device_private(dv); + mutex_enter(&sc->sc_mtx); acpiec_cold = true; + mutex_exit(&sc->sc_mtx); return true; } @@ -486,8 +489,11 @@ acpiec_suspend(device_t dv, const pmf_qual_t *qual) static bool acpiec_resume(device_t dv, const pmf_qual_t *qual) { + struct acpiec_softc *sc = device_private(dv); + mutex_enter(&sc->sc_mtx); acpiec_cold = false; + mutex_exit(&sc->sc_mtx); return true; } @@ -495,8 +501,12 @@ acpiec_resume(device_t dv, const pmf_qual_t *qual) static bool acpiec_shutdown(device_t dv, int how) { + struct acpiec_softc *sc = device_private(dv); + mutex_enter(&sc->sc_mtx); acpiec_cold = true; + mutex_exit(&sc->sc_mtx); + return true; }