Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/sys/dev/usb usb: Overhaul uhid(4) and uhidev(4) locking.
details: https://anonhg.NetBSD.org/src/rev/713caafae1e7
branches: trunk
changeset: 946526:713caafae1e7
user: riastradh <riastradh%NetBSD.org@localhost>
date: Sun Nov 29 22:54:51 2020 +0000
description:
usb: Overhaul uhid(4) and uhidev(4) locking.
- uhidev API rules:
1. Call uhidev_open when you want exclusive use of a report id.
After it succeeds, you will get interrupts.
2. Call uhidev_close when done with exclusive use of a report id.
After it returns, you will no longer get interrupts.
=> uhidev_open/close do not nest.
3. uhidev_write no longer requires the caller to have exclusive
access -- if there is a write in progress, it will block
interruptibly until done. This way drivers for individual
report ids need not work separately to coordinate their writes.
4. You must uhidev_stop to abort any pending writes on the same
report id. (uhidev_stop no longer does anything else -- to
ensure no more interrupts, just use uhidev_close.)
- Fix uhidev_open/close locking -- uhidev now has an interruptible
config lock held only on first open and last close by any report id
in the device, to serialize the transition between zero and nonzero
numbers of references which requires opening/closing pipes and
allocating/freeing buffers.
- Make /dev/uhidN selnotify(POLLHUP) when the device is yanked.
- Factor uhid device lookup and reference counting and dying
detection and so on into uhid_enter/exit.
- Nix struct uhid_softc::sc_access_lock. This served no purpose but
to confuse me when trying to understand the logic of this beast
(and to ensure uhidev_write exclusion, but it was uninterruptible,
which is wrong for something that implements userland operations,
and didn't actually work because uhidev_write did nothing to
coordinate between different report ids).
- Fix locking in select/poll.
- Use atomics to manage UHID_IMMED to keep it simple. (sc_lock would
be fine too but it makes the code more verbose.)
- Omit needless UHID_ASLP -- cv_broadcast already has this
micro-optimization.
With these changes, my Pinebook survives
for i in `jot 100`; do
echo '###' $i
for j in `jot 16`; do
usbhidctl -rf /dev/uhid$j >/dev/null &
done
wait
done
while plugging and unplugging uhid(4) devices (U2F keys), and the U2F
keys still work as U2F keys.
ok nick, mrg
XXX pullup-9
XXX pullup-8?
Note on ABI and pullups: This changes the layout of struct
uhidev_softc, but with the sole exception of ucycom(4) -- which at
the moment is completely broken and unusable -- the only members that
USB HID drivers use are sc_udev and sc_iface, which haven't changed.
The layout of struct uhidev, which is allocated by each USB HID
driver in its own softc structure, is unchanged.
diffstat:
sys/dev/usb/uhid.c | 351 ++++++++++++++++++++++++----------------
sys/dev/usb/uhidev.c | 432 ++++++++++++++++++++++++++++++++++++++++----------
sys/dev/usb/uhidev.h | 35 ++-
3 files changed, 578 insertions(+), 240 deletions(-)
diffs (truncated from 1212 to 300 lines):
diff -r aaf8ad837941 -r 713caafae1e7 sys/dev/usb/uhid.c
--- a/sys/dev/usb/uhid.c Sun Nov 29 21:50:50 2020 +0000
+++ b/sys/dev/usb/uhid.c Sun Nov 29 22:54:51 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: uhid.c,v 1.114 2020/05/23 23:42:42 ad Exp $ */
+/* $NetBSD: uhid.c,v 1.115 2020/11/29 22:54:51 riastradh Exp $ */
/*
* Copyright (c) 1998, 2004, 2008, 2012 The NetBSD Foundation, Inc.
@@ -35,7 +35,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: uhid.c,v 1.114 2020/05/23 23:42:42 ad Exp $");
+__KERNEL_RCSID(0, "$NetBSD: uhid.c,v 1.115 2020/11/29 22:54:51 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_compat_netbsd.h"
@@ -43,21 +43,24 @@
#endif
#include <sys/param.h>
-#include <sys/systm.h>
+#include <sys/types.h>
+
+#include <sys/atomic.h>
+#include <sys/compat_stub.h>
+#include <sys/conf.h>
+#include <sys/device.h>
+#include <sys/file.h>
+#include <sys/intr.h>
+#include <sys/ioctl.h>
#include <sys/kernel.h>
#include <sys/kmem.h>
+#include <sys/poll.h>
+#include <sys/proc.h>
+#include <sys/select.h>
#include <sys/signalvar.h>
-#include <sys/device.h>
-#include <sys/ioctl.h>
-#include <sys/conf.h>
+#include <sys/systm.h>
#include <sys/tty.h>
-#include <sys/file.h>
-#include <sys/select.h>
-#include <sys/proc.h>
#include <sys/vnode.h>
-#include <sys/poll.h>
-#include <sys/intr.h>
-#include <sys/compat_stub.h>
#include <dev/usb/usb.h>
#include <dev/usb/usbhid.h>
@@ -84,8 +87,7 @@
struct uhid_softc {
struct uhidev sc_hdev;
- kmutex_t sc_access_lock; /* serialises syscall accesses */
- kmutex_t sc_lock; /* protects refcnt, others */
+ kmutex_t sc_lock;
kcondvar_t sc_cv;
kcondvar_t sc_detach_cv;
@@ -99,12 +101,12 @@
struct selinfo sc_rsel;
proc_t *sc_async; /* process that wants SIGIO */
void *sc_sih;
- u_char sc_state; /* driver state */
-#define UHID_ASLP 0x01 /* waiting for device data */
+ volatile uint32_t sc_state; /* driver state */
#define UHID_IMMED 0x02 /* return read data immediately */
int sc_refcnt;
int sc_raw;
+ u_char sc_open;
u_char sc_dying;
};
@@ -192,7 +194,6 @@
aprint_normal(": input=%d, output=%d, feature=%d\n",
sc->sc_isize, sc->sc_osize, sc->sc_fsize);
- mutex_init(&sc->sc_access_lock, MUTEX_DEFAULT, IPL_NONE);
mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_SOFTUSB);
cv_init(&sc->sc_cv, "uhidrea");
cv_init(&sc->sc_detach_cv, "uhiddet");
@@ -225,22 +226,26 @@
DPRINTF(("uhid_detach: sc=%p flags=%d\n", sc, flags));
+ /* Prevent new I/O operations, and interrupt any pending reads. */
+ mutex_enter(&sc->sc_lock);
sc->sc_dying = 1;
+ cv_broadcast(&sc->sc_cv);
+ mutex_exit(&sc->sc_lock);
+
+ /* Interrupt any pending uhidev_write. */
+ uhidev_stop(&sc->sc_hdev);
+
+ /* Wait for I/O operations to complete. */
+ mutex_enter(&sc->sc_lock);
+ while (sc->sc_refcnt) {
+ DPRINTF(("%s: open=%d refcnt=%d\n", __func__,
+ sc->sc_open, sc->sc_refcnt));
+ cv_wait(&sc->sc_detach_cv, &sc->sc_lock);
+ }
+ mutex_exit(&sc->sc_lock);
pmf_device_deregister(self);
- mutex_enter(&sc->sc_lock);
- if (sc->sc_hdev.sc_state & UHIDEV_OPEN) {
- if (--sc->sc_refcnt >= 0) {
- /* Wake everyone */
- cv_broadcast(&sc->sc_cv);
- /* Wait for processes to go away. */
- if (cv_timedwait(&sc->sc_detach_cv, &sc->sc_lock, hz * 60))
- aprint_error_dev(self, ": didn't detach\n");
- }
- }
- mutex_exit(&sc->sc_lock);
-
/* locate the major number */
maj = cdevsw_lookup_major(&uhid_cdevsw);
@@ -248,14 +253,29 @@
mn = device_unit(self);
vdevgone(maj, mn, mn, VCHR);
-#if 0
- usbd_add_drv_event(USB_EVENT_DRIVER_DETACH,
- sc->sc_hdev.sc_parent->sc_udev, sc->sc_hdev.sc_dev);
-#endif
+ /*
+ * Wait for close to finish.
+ *
+ * XXX I assumed that vdevgone would synchronously call close,
+ * and not return before it has completed, but empirically the
+ * assertion of sc->sc_open == 0 below fires if we don't wait
+ * here. Someone^TM should carefully examine vdevgone to
+ * ascertain what it guarantees, and audit all other users of
+ * it accordingly.
+ */
+ mutex_enter(&sc->sc_lock);
+ while (sc->sc_open) {
+ DPRINTF(("%s: open=%d\n", __func__, sc->sc_open));
+ cv_wait(&sc->sc_detach_cv, &sc->sc_lock);
+ }
+ mutex_exit(&sc->sc_lock);
+
+ KASSERT(sc->sc_open == 0);
+ KASSERT(sc->sc_refcnt == 0);
+
cv_destroy(&sc->sc_cv);
cv_destroy(&sc->sc_detach_cv);
mutex_destroy(&sc->sc_lock);
- mutex_destroy(&sc->sc_access_lock);
seldestroy(&sc->sc_rsel);
softint_disestablish(sc->sc_sih);
@@ -281,12 +301,9 @@
mutex_enter(&sc->sc_lock);
(void)b_to_q(data, len, &sc->sc_q);
- if (sc->sc_state & UHID_ASLP) {
- sc->sc_state &= ~UHID_ASLP;
- DPRINTFN(5, ("uhid_intr: waking %p\n", &sc->sc_q));
- cv_broadcast(&sc->sc_cv);
- }
- selnotify(&sc->sc_rsel, 0, 0);
+ DPRINTFN(5, ("uhid_intr: waking %p\n", &sc->sc_q));
+ cv_broadcast(&sc->sc_cv);
+ selnotify(&sc->sc_rsel, 0, NOTE_SUBMIT);
if (sc->sc_async != NULL) {
DPRINTFN(3, ("uhid_intr: sending SIGIO %p\n", sc->sc_async));
softint_schedule(sc->sc_sih);
@@ -319,45 +336,74 @@
DPRINTF(("uhidopen: sc=%p\n", sc));
- if (sc->sc_dying)
- return ENXIO;
-
+ /*
+ * Try to open. If dying, or if already open (or opening),
+ * fail -- opens are exclusive.
+ */
mutex_enter(&sc->sc_lock);
-
- /*
- * uhid interrupts aren't enabled yet, so setup sc_q now, as
- * long as they're not already allocated.
- */
- if (sc->sc_hdev.sc_state & UHIDEV_OPEN) {
+ if (sc->sc_dying) {
+ mutex_exit(&sc->sc_lock);
+ return ENXIO;
+ }
+ if (sc->sc_open) {
mutex_exit(&sc->sc_lock);
return EBUSY;
}
+ sc->sc_open = 1;
+ atomic_store_relaxed(&sc->sc_state, 0);
mutex_exit(&sc->sc_lock);
+ /* uhid interrupts aren't enabled yet, so setup sc_q now */
if (clalloc(&sc->sc_q, UHID_BSIZE, 0) == -1) {
- return ENOMEM;
+ error = ENOMEM;
+ goto fail0;
}
- mutex_enter(&sc->sc_access_lock);
- error = uhidev_open(&sc->sc_hdev);
- if (error) {
- clfree(&sc->sc_q);
- mutex_exit(&sc->sc_access_lock);
- return error;
- }
- mutex_exit(&sc->sc_access_lock);
-
+ /* Allocate an output buffer if needed. */
if (sc->sc_osize > 0)
sc->sc_obuf = kmem_alloc(sc->sc_osize, KM_SLEEP);
else
sc->sc_obuf = NULL;
- sc->sc_state &= ~UHID_IMMED;
+ /* Paranoia: reset SIGIO before enabling interrputs. */
mutex_enter(&proc_lock);
sc->sc_async = NULL;
mutex_exit(&proc_lock);
+ /* Open the uhidev -- after this point we can get interrupts. */
+ error = uhidev_open(&sc->sc_hdev);
+ if (error)
+ goto fail1;
+
+ /* We are open for business. */
+ mutex_enter(&sc->sc_lock);
+ sc->sc_open = 2;
+ mutex_exit(&sc->sc_lock);
+
return 0;
+
+fail2: __unused
+ mutex_enter(&sc->sc_lock);
+ KASSERT(sc->sc_open == 2);
+ sc->sc_open = 1;
+ mutex_exit(&sc->sc_lock);
+ uhidev_close(&sc->sc_hdev);
+fail1: selnotify(&sc->sc_rsel, POLLHUP, 0);
+ mutex_enter(&proc_lock);
+ sc->sc_async = NULL;
+ mutex_exit(&proc_lock);
+ if (sc->sc_osize > 0) {
+ kmem_free(sc->sc_obuf, sc->sc_osize);
+ sc->sc_obuf = NULL;
+ }
+ clfree(&sc->sc_q);
+fail0: mutex_enter(&sc->sc_lock);
+ KASSERT(sc->sc_open == 1);
+ sc->sc_open = 0;
+ cv_broadcast(&sc->sc_detach_cv);
+ atomic_store_relaxed(&sc->sc_state, 0);
+ mutex_exit(&sc->sc_lock);
+ return error;
}
static int
@@ -369,25 +415,80 @@
DPRINTF(("uhidclose: sc=%p\n", sc));
+ /* We are closing up shop. Prevent new opens until we're done. */
+ mutex_enter(&sc->sc_lock);
+ KASSERT(sc->sc_open == 2);
+ sc->sc_open = 1;
+ mutex_exit(&sc->sc_lock);
+
+ /* Prevent further interrupts. */
+ uhidev_close(&sc->sc_hdev);
+
+ /* Hang up all select/poll. */
+ selnotify(&sc->sc_rsel, POLLHUP, 0);
+
+ /* Reset SIGIO. */
mutex_enter(&proc_lock);
sc->sc_async = NULL;
mutex_exit(&proc_lock);
- mutex_enter(&sc->sc_access_lock);
-
- uhidev_stop(&sc->sc_hdev);
-
+ /* Free the buffer and queue. */
+ if (sc->sc_osize > 0) {
+ kmem_free(sc->sc_obuf, sc->sc_osize);
+ sc->sc_obuf = NULL;
Home |
Main Index |
Thread Index |
Old Index