# HG changeset patch # User Taylor R Campbell # Date 1606677771 0 # Sun Nov 29 19:22:51 2020 +0000 # Branch trunk # Node ID 4bb398e561d22546f3f9f68aed770c650dbc67f9 # Parent 9d1047357993956144c3fca33f3de53e09c0a4c7 # EXP-Topic riastradh-uhidev 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_io_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. diff -r 9d1047357993 -r 4bb398e561d2 sys/dev/usb/uhid.c --- a/sys/dev/usb/uhid.c Thu Nov 26 02:06:01 2020 +0000 +++ b/sys/dev/usb/uhid.c Sun Nov 29 19:22:51 2020 +0000 @@ -43,21 +43,24 @@ #endif #include -#include +#include + +#include +#include +#include +#include +#include +#include +#include #include #include +#include +#include +#include #include -#include -#include -#include +#include #include -#include -#include -#include #include -#include -#include -#include #include #include @@ -84,8 +87,7 @@ int uhiddebug = 0; 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 uhid_softc { 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 @@ uhid_attach(device_t parent, device_t se 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 @@ uhid_detach(device_t self, int flags) 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 @@ uhid_detach(device_t self, int flags) 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 @@ uhid_intr(struct uhidev *addr, void *dat 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,72 @@ uhidopen(dev_t dev, int flag, int mode, 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; + mutex_exit(&sc->sc_lock); + return error; } static int @@ -369,25 +413,80 @@ uhidclose(dev_t dev, int flag, int mode, 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; + } clfree(&sc->sc_q); - if (sc->sc_osize > 0) - kmem_free(sc->sc_obuf, sc->sc_osize); - uhidev_close(&sc->sc_hdev); - - mutex_exit(&sc->sc_access_lock); + /* All set. We are now closed. */ + 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 0; } +static int +uhid_io_enter(dev_t dev, struct uhid_softc **scp) +{ + struct uhid_softc *sc; + int error; + + /* XXX need to hold reference to device */ + sc = device_lookup_private(&uhid_cd, UHIDUNIT(dev)); + if (sc == NULL) + return ENXIO; + + mutex_enter(&sc->sc_lock); + KASSERT(sc->sc_open == 2); + if (sc->sc_dying) { + error = ENXIO; + } else if (sc->sc_refcnt == INT_MAX) { + error = EBUSY; + } else { + *scp = sc; + sc->sc_refcnt++; + error = 0; + } + mutex_exit(&sc->sc_lock); + + return error; +} + +static void +uhid_io_exit(struct uhid_softc *sc) +{ + + mutex_enter(&sc->sc_lock); + KASSERT(sc->sc_open == 2); + KASSERT(sc->sc_refcnt > 0); + if (--sc->sc_refcnt == 0) + cv_broadcast(&sc->sc_detach_cv); + mutex_exit(&sc->sc_lock); +} + Static int uhid_do_read(struct uhid_softc *sc, struct uio *uio, int flag) { @@ -398,7 +497,7 @@ uhid_do_read(struct uhid_softc *sc, stru usbd_status err; DPRINTFN(1, ("uhidread\n")); - if (sc->sc_state & UHID_IMMED) { + if (atomic_load_relaxed(&sc->sc_state) & UHID_IMMED) { DPRINTFN(1, ("uhidread immed\n")); extra = sc->sc_hdev.sc_report_id != 0; if (sc->sc_isize + extra > sizeof(buffer)) @@ -416,14 +515,14 @@ uhid_do_read(struct uhid_softc *sc, stru mutex_exit(&sc->sc_lock); return EWOULDBLOCK; } - sc->sc_state |= UHID_ASLP; + if (sc->sc_dying) { + mutex_exit(&sc->sc_lock); + return EIO; + } DPRINTFN(5, ("uhidread: sleep on %p\n", &sc->sc_q)); error = cv_wait_sig(&sc->sc_cv, &sc->sc_lock); DPRINTFN(5, ("uhidread: woke, error=%d\n", error)); - if (sc->sc_dying) - error = EIO; if (error) { - sc->sc_state &= ~UHID_ASLP; break; } } @@ -455,20 +554,11 @@ uhidread(dev_t dev, struct uio *uio, int struct uhid_softc *sc; int error; - sc = device_lookup_private(&uhid_cd, UHIDUNIT(dev)); - - mutex_enter(&sc->sc_lock); - sc->sc_refcnt++; - mutex_exit(&sc->sc_lock); - - mutex_enter(&sc->sc_access_lock); + error = uhid_io_enter(dev, &sc); + if (error) + return error; error = uhid_do_read(sc, uio, flag); - mutex_exit(&sc->sc_access_lock); - - mutex_enter(&sc->sc_lock); - if (--sc->sc_refcnt < 0) - cv_broadcast(&sc->sc_detach_cv); - mutex_exit(&sc->sc_lock); + uhid_io_exit(sc); return error; } @@ -481,9 +571,6 @@ uhid_do_write(struct uhid_softc *sc, str DPRINTFN(1, ("uhidwrite\n")); - if (sc->sc_dying) - return EIO; - size = sc->sc_osize; if (uio->uio_resid != size || size == 0) return EINVAL; @@ -522,20 +609,11 @@ uhidwrite(dev_t dev, struct uio *uio, in struct uhid_softc *sc; int error; - sc = device_lookup_private(&uhid_cd, UHIDUNIT(dev)); - - mutex_enter(&sc->sc_lock); - sc->sc_refcnt++; - mutex_exit(&sc->sc_lock); - - mutex_enter(&sc->sc_access_lock); + error = uhid_io_enter(dev, &sc); + if (error) + return error; error = uhid_do_write(sc, uio, flag); - mutex_exit(&sc->sc_access_lock); - - mutex_enter(&sc->sc_lock); - if (--sc->sc_refcnt < 0) - cv_broadcast(&sc->sc_detach_cv); - mutex_exit(&sc->sc_lock); + uhid_io_exit(sc); return error; } @@ -552,9 +630,6 @@ uhid_do_ioctl(struct uhid_softc *sc, u_l DPRINTFN(2, ("uhidioctl: cmd=%lx\n", cmd)); - if (sc->sc_dying) - return EIO; - switch (cmd) { case FIONBIO: /* All handled in the upper FS layer. */ @@ -628,9 +703,9 @@ uhid_do_ioctl(struct uhid_softc *sc, u_l if (err) return EOPNOTSUPP; - sc->sc_state |= UHID_IMMED; + atomic_or_32(&sc->sc_state, UHID_IMMED); } else - sc->sc_state &= ~UHID_IMMED; + atomic_and_32(&sc->sc_state, ~UHID_IMMED); break; case USB_GET_REPORT: @@ -727,25 +802,11 @@ uhidioctl(dev_t dev, u_long cmd, void *a struct uhid_softc *sc; int error; - sc = device_lookup_private(&uhid_cd, UHIDUNIT(dev)); - if (sc == NULL) - return ENXIO; - - if (sc->sc_dying) - return EIO; - - mutex_enter(&sc->sc_lock); - sc->sc_refcnt++; - mutex_exit(&sc->sc_lock); - - mutex_enter(&sc->sc_access_lock); + error = uhid_io_enter(dev, &sc); + if (error) + return error; error = uhid_do_ioctl(sc, cmd, addr, flag, l); - mutex_exit(&sc->sc_access_lock); - - mutex_enter(&sc->sc_lock); - if (--sc->sc_refcnt < 0) - cv_broadcast(&sc->sc_detach_cv); - mutex_exit(&sc->sc_lock); + uhid_io_exit(sc); return error; } @@ -755,12 +816,8 @@ uhidpoll(dev_t dev, int events, struct l struct uhid_softc *sc; int revents = 0; - sc = device_lookup_private(&uhid_cd, UHIDUNIT(dev)); - if (sc == NULL) - return ENXIO; - - if (sc->sc_dying) - return EIO; + if (uhid_io_enter(dev, &sc) != 0) + return POLLHUP; mutex_enter(&sc->sc_lock); if (events & (POLLOUT | POLLWRNORM)) @@ -773,6 +830,7 @@ uhidpoll(dev_t dev, int events, struct l } mutex_exit(&sc->sc_lock); + uhid_io_exit(sc); return revents; } @@ -791,7 +849,18 @@ filt_uhidread(struct knote *kn, long hin { struct uhid_softc *sc = kn->kn_hook; + if (hint == NOTE_SUBMIT) + KASSERT(mutex_owned(&sc->sc_lock)); + else + mutex_enter(&sc->sc_lock); + kn->kn_data = sc->sc_q.c_cc; + + if (hint == NOTE_SUBMIT) + KASSERT(mutex_owned(&sc->sc_lock)); + else + mutex_exit(&sc->sc_lock); + return kn->kn_data > 0; } @@ -814,25 +883,24 @@ uhidkqfilter(dev_t dev, struct knote *kn { struct uhid_softc *sc; struct klist *klist; - - sc = device_lookup_private(&uhid_cd, UHIDUNIT(dev)); + int error; - if (sc->sc_dying) - return ENXIO; + error = uhid_io_enter(dev, &sc); + if (error) + return error; switch (kn->kn_filter) { case EVFILT_READ: klist = &sc->sc_rsel.sel_klist; kn->kn_fop = &uhidread_filtops; break; - case EVFILT_WRITE: klist = &sc->sc_rsel.sel_klist; kn->kn_fop = &uhid_seltrue_filtops; break; - default: - return EINVAL; + error = EINVAL; + goto out; } kn->kn_hook = sc; @@ -841,5 +909,6 @@ uhidkqfilter(dev_t dev, struct knote *kn SLIST_INSERT_HEAD(klist, kn, kn_selnext); mutex_exit(&sc->sc_lock); - return 0; +out: uhid_io_exit(sc); + return error; } diff -r 9d1047357993 -r 4bb398e561d2 sys/dev/usb/uhidev.c --- a/sys/dev/usb/uhidev.c Thu Nov 26 02:06:01 2020 +0000 +++ b/sys/dev/usb/uhidev.c Sun Nov 29 19:22:51 2020 +0000 @@ -42,14 +42,17 @@ #endif #include -#include +#include + +#include +#include +#include #include #include +#include +#include #include -#include -#include -#include -#include +#include #include #include @@ -141,6 +144,9 @@ uhidev_attach(device_t parent, device_t aprint_normal("\n"); mutex_init(&sc->sc_lock, MUTEX_DEFAULT, IPL_SOFTUSB); + cv_init(&sc->sc_cv, "uhidev"); + sc->sc_writelock = NULL; + sc->sc_configlock = NULL; id = usbd_get_interface_descriptor(iface); @@ -327,7 +333,7 @@ uhidev_attach(device_t parent, device_t aprint_normal_dev(self, "%d report ids\n", nrepid); nrepid++; repsizes = kmem_alloc(nrepid * sizeof(*repsizes), KM_SLEEP); - sc->sc_subdevs = kmem_zalloc(nrepid * sizeof(device_t), + sc->sc_subdevs = kmem_zalloc(nrepid * sizeof(sc->sc_subdevs[0]), KM_SLEEP); /* Just request max packet size for the interrupt pipe */ @@ -450,25 +456,65 @@ uhidev_detach(device_t self, int flags) DPRINTF(("uhidev_detach: sc=%p flags=%d\n", sc, flags)); + /* Notify that we are going away. */ + mutex_enter(&sc->sc_lock); sc->sc_dying = 1; - if (sc->sc_ipipe != NULL) - usbd_abort_pipe(sc->sc_ipipe); - - if (sc->sc_repdesc != NULL) - kmem_free(sc->sc_repdesc, sc->sc_repdesc_size); + cv_broadcast(&sc->sc_cv); + mutex_exit(&sc->sc_lock); - rv = 0; + /* + * Try to detach all our children. If anything fails, bail. + * Failure can happen if this is from drvctl -d; of course, if + * this is a USB device being yanked, flags will have + * DETACH_FORCE and the children will not have the option of + * refusing detachment. + */ for (i = 0; i < sc->sc_nrepid; i++) { - if (sc->sc_subdevs[i] != NULL) { - csc = device_private(sc->sc_subdevs[i]); - rnd_detach_source(&csc->rnd_source); - rv |= config_detach(sc->sc_subdevs[i], flags); + if (sc->sc_subdevs[i] == NULL) + continue; + /* + * XXX rnd_detach_source should go in uhidev_childdet, + * but the struct krndsource lives in the child's + * softc, which is gone by the time of childdet. The + * parent uhidev_softc should be changed to allocate + * the struct krndsource, not the child. + */ + csc = device_private(sc->sc_subdevs[i]); + rnd_detach_source(&csc->rnd_source); + rv = config_detach(sc->sc_subdevs[i], flags); + if (rv) { + rnd_attach_source(&csc->rnd_source, + device_xname(sc->sc_dev), + RND_TYPE_TTY, RND_FLAG_DEFAULT); + mutex_enter(&sc->sc_lock); + sc->sc_dying = 0; + mutex_exit(&sc->sc_lock); + return rv; } } + KASSERTMSG(sc->sc_refcnt == 0, + "%s: %d refs remain", device_xname(sc->sc_dev), sc->sc_refcnt); + KASSERT(sc->sc_opipe == NULL); + KASSERT(sc->sc_ipipe == NULL); + KASSERT(sc->sc_ibuf == NULL); + + if (sc->sc_repdesc != NULL) { + kmem_free(sc->sc_repdesc, sc->sc_repdesc_size); + sc->sc_repdesc = NULL; + } + if (sc->sc_subdevs != NULL) { + int nrepid = sc->sc_nrepid; + kmem_free(sc->sc_subdevs, nrepid * sizeof(sc->sc_subdevs[0])); + sc->sc_subdevs = NULL; + } + usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc->sc_udev, sc->sc_dev); pmf_device_deregister(self); + KASSERT(sc->sc_configlock == NULL); + KASSERT(sc->sc_writelock == NULL); + cv_destroy(&sc->sc_cv); mutex_destroy(&sc->sc_lock); return rv; @@ -547,34 +593,112 @@ uhidev_get_report_desc(struct uhidev_sof *size = sc->sc_repdesc_size; } -int -uhidev_open(struct uhidev *scd) +static int +uhidev_config_enter(struct uhidev_softc *sc) +{ + int error; + + KASSERT(mutex_owned(&sc->sc_lock)); + + for (;;) { + if (sc->sc_dying) + return ENXIO; + if (sc->sc_configlock == NULL) + break; + error = cv_wait_sig(&sc->sc_cv, &sc->sc_lock); + if (error) + return error; + } + + sc->sc_configlock = curlwp; + return 0; +} + +static void +uhidev_config_enter_nointr(struct uhidev_softc *sc) { - struct uhidev_softc *sc = scd->sc_parent; + + KASSERT(mutex_owned(&sc->sc_lock)); + + while (sc->sc_configlock) + cv_wait(&sc->sc_cv, &sc->sc_lock); + sc->sc_configlock = curlwp; +} + +static void +uhidev_config_exit(struct uhidev_softc *sc) +{ + + KASSERT(mutex_owned(&sc->sc_lock)); + KASSERTMSG(sc->sc_configlock == curlwp, "%s: migrated from %p to %p", + device_xname(sc->sc_dev), curlwp, sc->sc_configlock); + + sc->sc_configlock = NULL; + cv_broadcast(&sc->sc_cv); +} + +/* + * uhidev_open_pipes(sc) + * + * Ensure the pipes of the softc are open. Caller must hold + * sc_lock, which may be released and reacquired. + */ +static int +uhidev_open_pipes(struct uhidev_softc *sc) +{ usbd_status err; int error; - DPRINTF(("uhidev_open: open pipe, state=%d\n", scd->sc_state)); + KASSERT(mutex_owned(&sc->sc_lock)); + + /* If the device is dying, refuse. */ + if (sc->sc_dying) + return ENXIO; + + /* + * If the pipes are already open, just increment the reference + * count, or fail if it would overflow. + */ + if (sc->sc_refcnt) { + if (sc->sc_refcnt == INT_MAX) + return EBUSY; + sc->sc_refcnt++; + return 0; + } - mutex_enter(&sc->sc_lock); - if (scd->sc_state & UHIDEV_OPEN) { - mutex_exit(&sc->sc_lock); - return EBUSY; - } - scd->sc_state |= UHIDEV_OPEN; - if (sc->sc_refcnt++) { - mutex_exit(&sc->sc_lock); + /* + * If there's no input data to prepare, don't bother with the + * pipes. + * + * XXX What about output data? + */ + if (sc->sc_isize == 0) return 0; + + /* + * Lock the configuration and release sc_lock we may sleep to + * allocate. If someone else got in first, we're done; + * otherwise open the pipes. + */ + error = uhidev_config_enter(sc); + if (error) + goto out; + if (sc->sc_refcnt) { + if (sc->sc_refcnt == INT_MAX) { + error = EBUSY; + } else { + sc->sc_refcnt++; + error = 0; + } + goto out0; } mutex_exit(&sc->sc_lock); - if (sc->sc_isize == 0) - return 0; - + /* Allocate an input buffer. */ sc->sc_ibuf = kmem_alloc(sc->sc_isize, KM_SLEEP); /* Set up input interrupt pipe. */ - DPRINTF(("uhidev_open: isize=%d, ep=0x%02x\n", sc->sc_isize, + DPRINTF(("%s: isize=%d, ep=0x%02x\n", __func__, sc->sc_isize, sc->sc_iep_addr)); err = usbd_open_pipe_intr(sc->sc_iface, sc->sc_iep_addr, @@ -626,28 +750,141 @@ uhidev_open(struct uhidev *scd) } } - return 0; -out4: - /* Free output xfer */ - if (sc->sc_oxfer != NULL) + /* Success! */ + mutex_enter(&sc->sc_lock); + KASSERTMSG(sc->sc_refcnt == 0, "%d refs spuriously acquired", + sc->sc_refcnt); + sc->sc_refcnt++; + goto out0; + +out4: if (sc->sc_oxfer) { + usbd_abort_pipe(sc->sc_opipe); usbd_destroy_xfer(sc->sc_oxfer); -out3: - /* Abort output pipe */ - usbd_close_pipe(sc->sc_opipe); -out2: - /* Abort input pipe */ - usbd_close_pipe(sc->sc_ipipe); -out1: - DPRINTF(("uhidev_open: failed in someway")); + sc->sc_oxfer = NULL; + } +out3: if (sc->sc_opipe) { + usbd_close_pipe(sc->sc_opipe); + sc->sc_opipe = NULL; + } +out2: if (sc->sc_ipipe) { + usbd_abort_pipe(sc->sc_ipipe); + usbd_close_pipe(sc->sc_ipipe); + sc->sc_ipipe = NULL; + } +out1: kmem_free(sc->sc_ibuf, sc->sc_isize); + sc->sc_ibuf = NULL; + mutex_enter(&sc->sc_lock); +out0: KASSERT(mutex_owned(&sc->sc_lock)); + uhidev_config_exit(sc); +out: KASSERT(mutex_owned(&sc->sc_lock)); + return error; +} + +static void +uhidev_close_pipes(struct uhidev_softc *sc) +{ + + KASSERT(mutex_owned(&sc->sc_lock)); + KASSERTMSG(sc->sc_refcnt > 0, "%s: refcnt fouled: %d", + device_xname(sc->sc_dev), sc->sc_refcnt); + + /* If this isn't the last reference, just decrement. */ + if (sc->sc_refcnt > 1) { + sc->sc_refcnt--; + return; + } + + /* + * Lock the configuration and release sc_lock so we may sleep + * to free memory. We're not waiting for anyone to allocate or + * free anything. + */ + uhidev_config_enter_nointr(sc); + + /* + * If someone else acquired a reference while we were waiting + * for the config lock, nothing more for us to do. + */ + if (sc->sc_refcnt > 1) { + uhidev_config_exit(sc); + sc->sc_refcnt--; + return; + } + + /* + * We're the last reference and committed to closing the pipes. + * Decrement the reference count before we release the lock -- + * access to the pipes is allowed as long as the reference + * count is positive, so this forces all new opens to wait + * until the config lock is released. + */ + KASSERTMSG(sc->sc_refcnt == 1, "%s: refcnt fouled: %d", + device_xname(sc->sc_dev), sc->sc_refcnt); + sc->sc_refcnt--; + mutex_exit(&sc->sc_lock); + + if (sc->sc_oxfer) { + usbd_abort_pipe(sc->sc_opipe); + usbd_destroy_xfer(sc->sc_oxfer); + sc->sc_oxfer = NULL; + } + if (sc->sc_opipe) { + usbd_close_pipe(sc->sc_opipe); + sc->sc_opipe = NULL; + } + if (sc->sc_ipipe) { + usbd_abort_pipe(sc->sc_ipipe); + usbd_close_pipe(sc->sc_ipipe); + sc->sc_ipipe = NULL; + } kmem_free(sc->sc_ibuf, sc->sc_isize); + sc->sc_ibuf = NULL; + mutex_enter(&sc->sc_lock); + uhidev_config_exit(sc); + KASSERTMSG(sc->sc_refcnt == 0, "%s: refcnt fouled: %d", + device_xname(sc->sc_dev), sc->sc_refcnt); +} + +int +uhidev_open(struct uhidev *scd) +{ + struct uhidev_softc *sc = scd->sc_parent; + int error; + + mutex_enter(&sc->sc_lock); + + DPRINTF(("uhidev_open(%s, report %d = %s): state=%x refcnt=%d\n", + device_xname(sc->sc_dev), + scd->sc_report_id, + device_xname(scd->sc_dev), + scd->sc_state, + sc->sc_refcnt)); + + /* Mark the report id open. This is an exclusive lock. */ + if (scd->sc_state & UHIDEV_OPEN) { + error = EBUSY; + goto fail0; + } + scd->sc_state |= UHIDEV_OPEN; + + /* Open the pipes which are shared by all report ids. */ + error = uhidev_open_pipes(sc); + if (error) + goto fail1; + + mutex_exit(&sc->sc_lock); + + /* Success! */ + return 0; + +fail2: __unused + uhidev_close_pipes(sc); +fail1: KASSERTMSG(scd->sc_state & UHIDEV_OPEN, + "%s: report id %d: closed while opening", + device_xname(sc->sc_dev), scd->sc_report_id); scd->sc_state &= ~UHIDEV_OPEN; - sc->sc_refcnt = 0; - sc->sc_ibuf = NULL; - sc->sc_ipipe = NULL; - sc->sc_opipe = NULL; - sc->sc_oxfer = NULL; - mutex_exit(&sc->sc_lock); +fail0: mutex_exit(&sc->sc_lock); return error; } @@ -655,24 +892,15 @@ void uhidev_stop(struct uhidev *scd) { struct uhidev_softc *sc = scd->sc_parent; - - /* Disable interrupts. */ - if (sc->sc_opipe != NULL) { - usbd_abort_pipe(sc->sc_opipe); - usbd_close_pipe(sc->sc_opipe); - sc->sc_opipe = NULL; - } + bool abort = false; - if (sc->sc_ipipe != NULL) { - usbd_abort_pipe(sc->sc_ipipe); - usbd_close_pipe(sc->sc_ipipe); - sc->sc_ipipe = NULL; - } + mutex_enter(&sc->sc_lock); + if (sc->sc_writereportid == scd->sc_report_id) + abort = true; + mutex_exit(&sc->sc_lock); - if (sc->sc_ibuf != NULL) { - kmem_free(sc->sc_ibuf, sc->sc_isize); - sc->sc_ibuf = NULL; - } + if (abort && sc->sc_opipe) + usbd_abort_pipe(sc->sc_opipe); } void @@ -681,27 +909,24 @@ uhidev_close(struct uhidev *scd) struct uhidev_softc *sc = scd->sc_parent; mutex_enter(&sc->sc_lock); - if (!(scd->sc_state & UHIDEV_OPEN)) { - mutex_exit(&sc->sc_lock); - return; - } - scd->sc_state &= ~UHIDEV_OPEN; - if (--sc->sc_refcnt) { - mutex_exit(&sc->sc_lock); - return; - } - mutex_exit(&sc->sc_lock); + + DPRINTF(("uhidev_close(%s, report %d = %s): state=%x refcnt=%d\n", + device_xname(sc->sc_dev), + scd->sc_report_id, + device_xname(scd->sc_dev), + scd->sc_state, + sc->sc_refcnt)); - DPRINTF(("uhidev_close: close pipe\n")); + KASSERTMSG(scd->sc_state & UHIDEV_OPEN, + "%s: report id %d: unpaired close", + device_xname(sc->sc_dev), scd->sc_report_id); + uhidev_close_pipes(sc); + KASSERTMSG(scd->sc_state & UHIDEV_OPEN, + "%s: report id %d: closed while closing", + device_xname(sc->sc_dev), scd->sc_report_id); + scd->sc_state &= ~UHIDEV_OPEN; - if (sc->sc_oxfer != NULL) { - usbd_destroy_xfer(sc->sc_oxfer); - sc->sc_oxfer = NULL; - } - - - /* Possibly redundant, but properly handled */ - uhidev_stop(scd); + mutex_exit(&sc->sc_lock); } usbd_status @@ -736,12 +961,29 @@ uhidev_get_report(struct uhidev *scd, in usbd_status uhidev_write(struct uhidev_softc *sc, void *data, int len) { + usbd_status err; DPRINTF(("uhidev_write: data=%p, len=%d\n", data, len)); if (sc->sc_opipe == NULL) return USBD_INVAL; + mutex_enter(&sc->sc_lock); + for (;;) { + if (sc->sc_dying) { + err = USBD_IOERROR; + goto out; + } + if (sc->sc_writelock == NULL) + break; + if (cv_wait_sig(&sc->sc_cv, &sc->sc_lock)) { + err = USBD_INTERRUPTED; + goto out; + } + } + sc->sc_writelock = curlwp; + mutex_exit(&sc->sc_lock); + #ifdef UHIDEV_DEBUG if (uhidevdebug > 50) { @@ -754,6 +996,14 @@ uhidev_write(struct uhidev_softc *sc, vo DPRINTF(("\n")); } #endif - return usbd_intr_transfer(sc->sc_oxfer, sc->sc_opipe, 0, USBD_NO_TIMEOUT, - data, &len); + err = usbd_intr_transfer(sc->sc_oxfer, sc->sc_opipe, 0, + USBD_NO_TIMEOUT, data, &len); + + mutex_enter(&sc->sc_lock); + KASSERTMSG(sc->sc_writelock == curlwp, "%s: migrated from %p to %p", + device_xname(sc->sc_dev), curlwp, sc->sc_writelock); + sc->sc_writelock = NULL; + cv_broadcast(&sc->sc_cv); +out: mutex_exit(&sc->sc_lock); + return err; } diff -r 9d1047357993 -r 4bb398e561d2 sys/dev/usb/uhidev.h --- a/sys/dev/usb/uhidev.h Thu Nov 26 02:06:01 2020 +0000 +++ b/sys/dev/usb/uhidev.h Sun Nov 29 19:22:51 2020 +0000 @@ -30,6 +30,8 @@ * POSSIBILITY OF SUCH DAMAGE. */ +#ifndef _DEV_USB_UHIDEV_H_ +#define _DEV_USB_UHIDEV_H_ #include @@ -37,26 +39,35 @@ struct uhidev_softc { device_t sc_dev; /* base device */ struct usbd_device *sc_udev; struct usbd_interface *sc_iface; /* interface */ - struct usbd_pipe *sc_ipipe; /* input interrupt pipe */ int sc_iep_addr; - - u_char *sc_ibuf; + int sc_oep_addr; u_int sc_isize; - struct usbd_pipe *sc_opipe; /* output interrupt pipe */ - struct usbd_xfer *sc_oxfer; /* write request */ - int sc_oep_addr; - + int sc_repdesc_size; void *sc_repdesc; - int sc_repdesc_size; u_int sc_nrepid; device_t *sc_subdevs; + kmutex_t sc_lock; + kcondvar_t sc_cv; + + /* Read/written under sc_lock. */ + struct lwp *sc_writelock; + struct lwp *sc_configlock; int sc_refcnt; + int sc_writereportid; u_char sc_dying; - kmutex_t sc_lock; /* protects writes to sc_state */ + /* + * - Read under sc_lock, provided sc_refcnt > 0. + * - Written under sc_configlock only when transitioning to and + * from sc_refcnt = 0. + */ + u_char *sc_ibuf; + struct usbd_pipe *sc_ipipe; /* input interrupt pipe */ + struct usbd_pipe *sc_opipe; /* output interrupt pipe */ + struct usbd_xfer *sc_oxfer; /* write request */ u_int sc_flags; #define UHIDEV_F_XB1 0x0001 /* Xbox 1 controller */ @@ -66,7 +77,7 @@ struct uhidev { device_t sc_dev; /* base device */ struct uhidev_softc *sc_parent; uByte sc_report_id; - uint8_t sc_state; + uint8_t sc_state; /* read/written under sc_parent->sc_lock */ #define UHIDEV_OPEN 0x01 /* device is open */ int sc_in_rep_size; void (*sc_intr)(struct uhidev *, void *, u_int); @@ -89,3 +100,5 @@ usbd_status uhidev_get_report(struct uhi usbd_status uhidev_write(struct uhidev_softc *, void *, int); #define UHIDEV_OSIZE 64 + +#endif /* _DEV_USB_UHIDEV_H_ */