Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/dev/usb properly protect uhid's sc_q member with sc_lock...



details:   https://anonhg.NetBSD.org/src/rev/7c22aadb8689
branches:  trunk
changeset: 806753:7c22aadb8689
user:      mrg <mrg%NetBSD.org@localhost>
date:      Sat Mar 07 20:20:55 2015 +0000

description:
properly protect uhid's sc_q member with sc_lock.  should fix PR#49728.
while here, remove D_MPSAFE from uhid* and all uhid users, as it really
needs all the callers to be safe and they're not.

XXX: pullup-7

diffstat:

 sys/dev/usb/TODO.usbmp |    8 +-
 sys/dev/usb/uatp.c     |    9 +--
 sys/dev/usb/ucycom.c   |   13 +++-
 sys/dev/usb/uhid.c     |  119 ++++++++++++++++++++++++++++---------------------
 sys/dev/usb/uhidev.c   |   47 +++++++++++-------
 sys/dev/usb/uhidev.h   |    3 +-
 sys/dev/usb/ukbd.c     |    7 +-
 sys/dev/usb/uyurex.c   |    8 ++-
 8 files changed, 124 insertions(+), 90 deletions(-)

diffs (truncated from 630 to 300 lines):

diff -r 60b72dad0df1 -r 7c22aadb8689 sys/dev/usb/TODO.usbmp
--- a/sys/dev/usb/TODO.usbmp    Sat Mar 07 18:54:57 2015 +0000
+++ b/sys/dev/usb/TODO.usbmp    Sat Mar 07 20:20:55 2015 +0000
@@ -1,4 +1,4 @@
-$NetBSD: TODO.usbmp,v 1.8 2014/08/02 15:50:16 skrll Exp $
+$NetBSD: TODO.usbmp,v 1.9 2015/03/07 20:20:55 mrg Exp $
 
 
 the majority of the USB MP device interface is documented in usbdivar.h.
@@ -41,8 +41,8 @@
   - own cdevsw that isn't D_MPSAFE; need to check intr handlers
 
   uhid(4)
-  - needs some locking here (not completely tested changes)
-  - done
+  - D_MPSAFE not set as all users need it first.
+  - mostly done
 
   ukbd(4)
   ums(4)
@@ -135,7 +135,7 @@
   - ral
   - rum
   - run
-  - urtw
+  - urtw               working
   - urtwn
   - upgt
   - zyd
diff -r 60b72dad0df1 -r 7c22aadb8689 sys/dev/usb/uatp.c
--- a/sys/dev/usb/uatp.c        Sat Mar 07 18:54:57 2015 +0000
+++ b/sys/dev/usb/uatp.c        Sat Mar 07 20:20:55 2015 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: uatp.c,v 1.10 2014/07/17 17:11:12 riastradh Exp $      */
+/*     $NetBSD: uatp.c,v 1.11 2015/03/07 20:20:55 mrg Exp $    */
 
 /*-
  * Copyright (c) 2011-2014 The NetBSD Foundation, Inc.
@@ -146,7 +146,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: uatp.c,v 1.10 2014/07/17 17:11:12 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: uatp.c,v 1.11 2015/03/07 20:20:55 mrg Exp $");
 
 #include <sys/types.h>
 #include <sys/param.h>
@@ -1350,8 +1350,7 @@
 
        DPRINTF(sc, UATP_DEBUG_MISC, ("initializing\n"));
        geyser34_enable_raw_mode(sc);
-       usb_init_task(&sc->sc_reset_task, &geyser34_reset_task, sc,
-           USB_TASKQ_MPSAFE);
+       usb_init_task(&sc->sc_reset_task, &geyser34_reset_task, sc, 0);
 }
 
 static int
@@ -2012,7 +2011,7 @@
 static void
 tap_initialize(struct uatp_softc *sc)
 {
-       callout_init(&sc->sc_untap_callout, CALLOUT_MPSAFE);
+       callout_init(&sc->sc_untap_callout, 0);
        callout_setfunc(&sc->sc_untap_callout, untap_callout, sc);
        mutex_init(&sc->sc_tap_mutex, MUTEX_DEFAULT, IPL_USB);
        cv_init(&sc->sc_tap_cv, "uatptap");
diff -r 60b72dad0df1 -r 7c22aadb8689 sys/dev/usb/ucycom.c
--- a/sys/dev/usb/ucycom.c      Sat Mar 07 18:54:57 2015 +0000
+++ b/sys/dev/usb/ucycom.c      Sat Mar 07 20:20:55 2015 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: ucycom.c,v 1.41 2014/11/15 19:26:37 christos Exp $     */
+/*     $NetBSD: ucycom.c,v 1.42 2015/03/07 20:20:55 mrg Exp $  */
 
 /*
  * Copyright (c) 2005 The NetBSD Foundation, Inc.
@@ -38,7 +38,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ucycom.c,v 1.41 2014/11/15 19:26:37 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ucycom.c,v 1.42 2015/03/07 20:20:55 mrg Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -1124,9 +1124,14 @@
 Static void
 ucycom_cleanup(struct ucycom_softc *sc)
 {
+       uint8_t *obuf;
+
        DPRINTF(("ucycom_cleanup: closing uhidev\n"));
 
-       if (sc->sc_obuf !=NULL)
-               free (sc->sc_obuf, M_USBDEV);
+       obuf = sc->sc_obuf;
+       sc->sc_obuf = NULL;
        uhidev_close(&sc->sc_hdev);
+
+       if (obuf != NULL)
+               free (obuf, M_USBDEV);
 }
diff -r 60b72dad0df1 -r 7c22aadb8689 sys/dev/usb/uhid.c
--- a/sys/dev/usb/uhid.c        Sat Mar 07 18:54:57 2015 +0000
+++ b/sys/dev/usb/uhid.c        Sat Mar 07 20:20:55 2015 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: uhid.c,v 1.92 2014/07/25 08:10:39 dholland Exp $       */
+/*     $NetBSD: uhid.c,v 1.93 2015/03/07 20:20:55 mrg 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.92 2014/07/25 08:10:39 dholland Exp $");
+__KERNEL_RCSID(0, "$NetBSD: uhid.c,v 1.93 2015/03/07 20:20:55 mrg Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_compat_netbsd.h"
@@ -91,7 +91,7 @@
 
        u_char *sc_obuf;
 
-       struct clist sc_q;
+       struct clist sc_q;      /* protected by sc_lock */
        struct selinfo sc_rsel;
        proc_t *sc_async;       /* process that wants SIGIO */
        void *sc_sih;
@@ -127,7 +127,7 @@
        .d_mmap = nommap,
        .d_kqfilter = uhidkqfilter,
        .d_discard = nodiscard,
-       .d_flag = D_OTHER | D_MPSAFE
+       .d_flag = D_OTHER
 };
 
 Static void uhid_intr(struct uhidev *, void *, u_int len);
@@ -154,9 +154,9 @@
        DPRINTF(("uhid_match: report=%d\n", uha->reportid));
 
        if (match->cf_flags & 1)
-               return (UMATCH_HIGHEST);
+               return UMATCH_HIGHEST;
        else
-               return (UMATCH_IFACECLASS_GENERIC);
+               return UMATCH_IFACECLASS_GENERIC;
 }
 
 void
@@ -172,7 +172,7 @@
        sc->sc_hdev.sc_intr = uhid_intr;
        sc->sc_hdev.sc_parent = uha->parent;
        sc->sc_hdev.sc_report_id = uha->reportid;
-       sc->sc_sih = softint_establish(SOFTINT_MPSAFE | SOFTINT_CLOCK,
+       sc->sc_sih = softint_establish(SOFTINT_CLOCK,
            uhid_softintr, sc);
 
        uhidev_get_report_desc(uha->parent, &desc, &size);
@@ -253,7 +253,7 @@
        seldestroy(&sc->sc_rsel);
        softint_disestablish(sc->sc_sih);
 
-       return (0);
+       return 0;
 }
 
 void
@@ -314,22 +314,31 @@
        DPRINTF(("uhidopen: sc=%p\n", sc));
 
        if (sc->sc_dying)
-               return (ENXIO);
+               return ENXIO;
 
        mutex_enter(&sc->sc_access_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) {
+               mutex_exit(&sc->sc_access_lock);
+               return EBUSY;
+       }
+       if (clalloc(&sc->sc_q, UHID_BSIZE, 0) == -1) {
+               mutex_exit(&sc->sc_access_lock);
+               return ENOMEM;
+       }
+
        error = uhidev_open(&sc->sc_hdev);
        if (error) {
+               clfree(&sc->sc_q);
                mutex_exit(&sc->sc_access_lock);
-               return (error);
+               return error;
        }
        mutex_exit(&sc->sc_access_lock);
 
-       if (clalloc(&sc->sc_q, UHID_BSIZE, 0) == -1) {
-               mutex_enter(&sc->sc_access_lock);
-               uhidev_close(&sc->sc_hdev);
-               mutex_exit(&sc->sc_access_lock);
-               return (ENOMEM);
-       }
        sc->sc_obuf = malloc(sc->sc_osize, M_USBDEV, M_WAITOK);
        sc->sc_state &= ~UHID_IMMED;
 
@@ -337,7 +346,7 @@
        sc->sc_async = NULL;
        mutex_exit(proc_lock);
 
-       return (0);
+       return 0;
 }
 
 int
@@ -349,18 +358,24 @@
 
        DPRINTF(("uhidclose: sc=%p\n", sc));
 
-       clfree(&sc->sc_q);
-       free(sc->sc_obuf, M_USBDEV);
-
        mutex_enter(proc_lock);
        sc->sc_async = NULL;
        mutex_exit(proc_lock);
 
        mutex_enter(&sc->sc_access_lock);
+
+       mutex_enter(&sc->sc_lock);
+       uhidev_stop(&sc->sc_hdev);
+       mutex_exit(&sc->sc_lock);
+
+       clfree(&sc->sc_q);
+       free(sc->sc_obuf, M_USBDEV);
+
        uhidev_close(&sc->sc_hdev);
+
        mutex_exit(&sc->sc_access_lock);
 
-       return (0);
+       return 0;
 }
 
 int
@@ -379,15 +394,15 @@
                err = uhidev_get_report(&sc->sc_hdev, UHID_INPUT_REPORT,
                                        buffer, sc->sc_isize + extra);
                if (err)
-                       return (EIO);
-               return (uiomove(buffer+extra, sc->sc_isize, uio));
+                       return EIO;
+               return uiomove(buffer+extra, sc->sc_isize, uio);
        }
 
        mutex_enter(&sc->sc_lock);
        while (sc->sc_q.c_cc == 0) {
                if (flag & IO_NDELAY) {
                        mutex_exit(&sc->sc_lock);
-                       return (EWOULDBLOCK);
+                       return EWOULDBLOCK;
                }
                sc->sc_state |= UHID_ASLP;
                DPRINTFN(5, ("uhidread: sleep on %p\n", &sc->sc_q));
@@ -400,7 +415,6 @@
                        break;
                }
        }
-       mutex_exit(&sc->sc_lock);
 
        /* Transfer as many chunks as possible. */
        while (sc->sc_q.c_cc > 0 && uio->uio_resid > 0 && !error) {
@@ -413,11 +427,14 @@
                DPRINTFN(5, ("uhidread: got %lu chars\n", (u_long)length));
 
                /* Copy the data to the user process. */
+               mutex_exit(&sc->sc_lock);
                if ((error = uiomove(buffer, length, uio)) != 0)
-                       break;
+                       return error;
+               mutex_enter(&sc->sc_lock);
        }
 
-       return (error);
+       mutex_exit(&sc->sc_lock);
+       return error;
 }
 
 int
@@ -440,7 +457,7 @@
        if (--sc->sc_refcnt < 0)
                usb_detach_broadcast(sc->sc_hdev.sc_dev, &sc->sc_detach_cv);
        mutex_exit(&sc->sc_lock);
-       return (error);
+       return error;
 }
 
 int
@@ -453,12 +470,12 @@
        DPRINTFN(1, ("uhidwrite\n"));



Home | Main Index | Thread Index | Old Index