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.
This commit is contained in:
riastradh
parent
06a7b095fe
commit
be765157c3
|
|
@ -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 self, void *aux)
|
|||
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 self, void *aux)
|
|||
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 self, void *aux)
|
|||
}
|
||||
|
||||
/* 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;
|
||||
|
||||
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))
|
||||
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_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_qual_t *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 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_qual_t *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_softc *sc)
|
|||
}
|
||||
|
||||
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 *sc)
|
|||
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 *sc)
|
|||
}
|
||||
|
||||
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.
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in New Issue