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;
