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