Module Name: src Committed By: riastradh Date: Fri Jan 14 22:25:49 UTC 2022
Modified Files: src/sys/dev/i2c: ihidev.c ihidev.h Log Message: ihidev(4): Fix locking and interrupt handler. - Can't run iic_exec in softint because it does cv_wait, at least on some i2c controllers -- defer to workqueue instead. - Fix violations of locking rules: . Do not take a lock at higher IPL than it is defined at! . Do not sleep under a lock! . Definitely do not sleep under a spin lock! In this case, sc_intr_lock was defined at IPL_VM but used at IPL_TTY, and i2c transactions -- possibly causing sleep for cv_wait -- were issued under it. But in this case, the interrupt handler needs only a single bit to mark whether the work is pending, so just use atomic_swap for that. - Use an adaptive lock (IPL_NONE) for i2c transactions. - Detach children, and do so before freeing anything. To generate a diff of this commit: cvs rdiff -u -r1.20 -r1.21 src/sys/dev/i2c/ihidev.c cvs rdiff -u -r1.5 -r1.6 src/sys/dev/i2c/ihidev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
Modified files: Index: src/sys/dev/i2c/ihidev.c diff -u src/sys/dev/i2c/ihidev.c:1.20 src/sys/dev/i2c/ihidev.c:1.21 --- src/sys/dev/i2c/ihidev.c:1.20 Sat Aug 7 16:19:11 2021 +++ src/sys/dev/i2c/ihidev.c Fri Jan 14 22:25:49 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: ihidev.c,v 1.20 2021/08/07 16:19:11 thorpej Exp $ */ +/* $NetBSD: ihidev.c,v 1.21 2022/01/14 22:25:49 riastradh Exp $ */ /* $OpenBSD ihidev.c,v 1.13 2017/04/08 02:57:23 deraadt Exp $ */ /*- @@ -54,13 +54,13 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: ihidev.c,v 1.20 2021/08/07 16:19:11 thorpej Exp $"); +__KERNEL_RCSID(0, "$NetBSD: ihidev.c,v 1.21 2022/01/14 22:25:49 riastradh Exp $"); #include <sys/param.h> #include <sys/systm.h> #include <sys/device.h> #include <sys/kmem.h> - +#include <sys/workqueue.h> #include <dev/i2c/i2cvar.h> #include <dev/i2c/ihidev.h> @@ -110,14 +110,14 @@ static int ihidev_detach(device_t, int); CFATTACH_DECL_NEW(ihidev, sizeof(struct ihidev_softc), ihidev_match, ihidev_attach, ihidev_detach, NULL); -static bool ihiddev_intr_init(struct ihidev_softc *); -static void ihiddev_intr_fini(struct ihidev_softc *); +static bool ihidev_intr_init(struct ihidev_softc *); +static void ihidev_intr_fini(struct ihidev_softc *); static bool ihidev_suspend(device_t, const pmf_qual_t *); static bool ihidev_resume(device_t, const pmf_qual_t *); static int ihidev_hid_command(struct ihidev_softc *, int, void *, bool); static int ihidev_intr(void *); -static void ihidev_softintr(void *); +static void ihidev_work(struct work *, void *); static int ihidev_reset(struct ihidev_softc *, bool); static int ihidev_hid_desc_parse(struct ihidev_softc *); @@ -160,14 +160,14 @@ ihidev_attach(device_t parent, device_t sc->sc_dev = self; sc->sc_tag = ia->ia_tag; sc->sc_addr = ia->ia_addr; - mutex_init(&sc->sc_intr_lock, MUTEX_DEFAULT, IPL_VM); + mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_NONE); sc->sc_phandle = ia->ia_cookie; if (ia->ia_cookietype != I2C_COOKIE_ACPI) { aprint_error(": unsupported device tree type\n"); return; } - if (! ihidev_acpi_get_info(sc)) { + if (!ihidev_acpi_get_info(sc)) { return; } @@ -208,7 +208,7 @@ ihidev_attach(device_t parent, device_t repsz)); } sc->sc_ibuf = kmem_zalloc(sc->sc_isize, KM_SLEEP); - if (! ihiddev_intr_init(sc)) { + if (!ihidev_intr_init(sc)) { return; } @@ -245,7 +245,8 @@ ihidev_attach(device_t parent, device_t } /* power down until we're opened */ - if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, &I2C_HID_POWER_OFF, false)) { + if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, &I2C_HID_POWER_OFF, + false)) { aprint_error_dev(sc->sc_dev, "failed to power down\n"); return; } @@ -257,13 +258,19 @@ static int ihidev_detach(device_t self, int flags) { struct ihidev_softc *sc = device_private(self); + int error; + + error = config_detach_children(self, flags); + if (error) + return error; + + pmf_device_deregister(self); + ihidev_intr_fini(sc); + + if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, &I2C_HID_POWER_OFF, + true)) + aprint_error_dev(sc->sc_dev, "failed to power down\n"); - mutex_enter(&sc->sc_intr_lock); - ihiddev_intr_fini(sc); - if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, - &I2C_HID_POWER_OFF, true)) - aprint_error_dev(sc->sc_dev, "failed to power down\n"); - mutex_exit(&sc->sc_intr_lock); if (sc->sc_ibuf != NULL) { kmem_free(sc->sc_ibuf, sc->sc_isize); sc->sc_ibuf = NULL; @@ -272,8 +279,9 @@ ihidev_detach(device_t self, int flags) if (sc->sc_report != NULL) kmem_free(sc->sc_report, sc->sc_reportlen); - pmf_device_deregister(self); - return (0); + mutex_destroy(&sc->sc_lock); + + return 0; } static bool @@ -281,14 +289,14 @@ ihidev_suspend(device_t self, const pmf_ { struct ihidev_softc *sc = device_private(self); - mutex_enter(&sc->sc_intr_lock); + mutex_enter(&sc->sc_lock); if (sc->sc_refcnt > 0) { printf("ihidev power off\n"); if (ihidev_hid_command(sc, I2C_HID_CMD_SET_POWER, &I2C_HID_POWER_OFF, true)) aprint_error_dev(sc->sc_dev, "failed to power down\n"); } - mutex_exit(&sc->sc_intr_lock); + mutex_exit(&sc->sc_lock); return true; } @@ -297,12 +305,12 @@ ihidev_resume(device_t self, const pmf_q { struct ihidev_softc *sc = device_private(self); - mutex_enter(&sc->sc_intr_lock); + mutex_enter(&sc->sc_lock); if (sc->sc_refcnt > 0) { printf("ihidev power reset\n"); ihidev_reset(sc, true); } - mutex_exit(&sc->sc_intr_lock); + mutex_exit(&sc->sc_lock); return true; } @@ -646,7 +654,7 @@ ihidev_hid_desc_parse(struct ihidev_soft } static bool -ihiddev_intr_init(struct ihidev_softc *sc) +ihidev_intr_init(struct ihidev_softc *sc) { #if NACPICA > 0 ACPI_HANDLE hdl = (void *)(uintptr_t)sc->sc_phandle; @@ -682,12 +690,13 @@ ihiddev_intr_init(struct ihidev_softc *s aprint_normal_dev(sc->sc_dev, "interrupting at %s\n", acpi_intr_string(sc->sc_ih, buf, sizeof(buf))); - sc->sc_sih = softint_establish(SOFTINT_SERIAL, ihidev_softintr, sc); - if (sc->sc_sih == NULL) { + if (workqueue_create(&sc->sc_wq, device_xname(sc->sc_dev), ihidev_work, + sc, PRI_NONE, IPL_TTY, WQ_MPSAFE)) { aprint_error_dev(sc->sc_dev, - "can't establish soft interrupt\n"); + "can't establish workqueue\n"); return false; } + sc->sc_work_pending = 0; return true; #else @@ -697,14 +706,14 @@ ihiddev_intr_init(struct ihidev_softc *s } static void -ihiddev_intr_fini(struct ihidev_softc *sc) +ihidev_intr_fini(struct ihidev_softc *sc) { #if NACPICA > 0 if (sc->sc_ih != NULL) { acpi_intr_disestablish(sc->sc_ih); } - if (sc->sc_sih != NULL) { - softint_disestablish(sc->sc_sih); + if (sc->sc_wq != NULL) { + workqueue_destroy(sc->sc_wq); } #endif } @@ -736,23 +745,19 @@ ihidev_intr(void *arg) { struct ihidev_softc * const sc = arg; - mutex_enter(&sc->sc_intr_lock); - /* - * Schedule our soft interrupt handler. If we're using a level- - * triggered interrupt, we have to mask it off while we wait - * for service. + * Schedule our work. If we're using a level-triggered + * interrupt, we have to mask it off while we wait for service. */ - softint_schedule(sc->sc_sih); + if (atomic_swap_uint(&sc->sc_work_pending, 1) == 0) + workqueue_enqueue(sc->sc_wq, &sc->sc_work, NULL); ihidev_intr_mask(sc); - mutex_exit(&sc->sc_intr_lock); - return 1; } static void -ihidev_softintr(void *arg) +ihidev_work(struct work *wk, void *arg) { struct ihidev_softc * const sc = arg; struct ihidev *scd; @@ -761,13 +766,14 @@ ihidev_softintr(void *arg) u_char *p; u_int rep = 0; - mutex_enter(&sc->sc_intr_lock); + atomic_store_relaxed(&sc->sc_work_pending, 0); + + mutex_enter(&sc->sc_lock); + iic_acquire_bus(sc->sc_tag, 0); res = iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr, NULL, 0, sc->sc_ibuf, sc->sc_isize, 0); iic_release_bus(sc->sc_tag, 0); - mutex_exit(&sc->sc_intr_lock); - if (res != 0) goto out; @@ -807,6 +813,8 @@ ihidev_softintr(void *arg) scd->sc_intr(scd, p, psize); out: + mutex_exit(&sc->sc_lock); + /* * If our interrupt is level-triggered, re-enable it now. */ @@ -837,7 +845,7 @@ ihidev_print(void *aux, const char *pnp) if (iha->reportid == IHIDEV_CLAIM_ALLREPORTID) return (QUIET); - + if (pnp) aprint_normal("hid at %s", pnp); @@ -974,7 +982,7 @@ ihidev_set_report(struct device *dev, in static bool ihidev_acpi_get_info(struct ihidev_softc *sc) -{ +{ ACPI_HANDLE hdl = (void *)(uintptr_t)sc->sc_phandle; ACPI_STATUS status; ACPI_INTEGER val; Index: src/sys/dev/i2c/ihidev.h diff -u src/sys/dev/i2c/ihidev.h:1.5 src/sys/dev/i2c/ihidev.h:1.6 --- src/sys/dev/i2c/ihidev.h:1.5 Fri Jan 14 21:32:27 2022 +++ src/sys/dev/i2c/ihidev.h Fri Jan 14 22:25:49 2022 @@ -1,4 +1,4 @@ -/* $NetBSD: ihidev.h,v 1.5 2022/01/14 21:32:27 riastradh Exp $ */ +/* $NetBSD: ihidev.h,v 1.6 2022/01/14 22:25:49 riastradh Exp $ */ /* $OpenBSD ihidev.h,v 1.4 2016/01/31 18:24:35 jcs Exp $ */ /*- @@ -106,6 +106,7 @@ struct i2c_hid_desc { #include <sys/device.h> #include <sys/mutex.h> +#include <sys/workqueue.h> #include <dev/i2c/i2cvar.h> @@ -114,10 +115,12 @@ struct ihidev_softc { i2c_tag_t sc_tag; i2c_addr_t sc_addr; uint64_t sc_phandle; + kmutex_t sc_lock; void * sc_ih; - void * sc_sih; - kmutex_t sc_intr_lock; + struct workqueue *sc_wq; + struct work sc_work; + volatile unsigned sc_work_pending; int sc_intr_type; u_int sc_hid_desc_addr;