Re: Hang/reboot with -current and umidi on i386

[Tom Ivar Helbekkmo <tih%hamartun.priv.no@localhost>]

I wrote:

> I've got it fixed, now, and will write a description tomorrow (time
> for sleep at the moment), and file a PR with the patches.

PR sent.  Here's the text from my send-pr (oh, and the Roland UM-ONE
interfaces I mention will be supported if my patch in kern/45908 is
applied):

There are problems with locking in the current version of
/sys/dev/usb/umidi.c, causing NetBSD to crash if an attempt is made to
read from a USB MIDI device.  The reason turns out to be that
assumptions about the kernel lock being taken before certain
operations, and about the taking and releasing of mutex spin locks,
are not being correctly upheld by umidi.c.

Specifically, there are four problems:

1: the kernel lock needs to be held while an input transfer is
   initiated (the KASSERT(KERNEL_LOCKED_P()) is found in the function
   usbd_transfer() in /sys/dev/usb/usbdi.c).  I added code to grab and
   release the kernel lock to open_in_jack() in /sys/dev/usb/umidi.c,
   around the call to start_input_transfer().

2: the kernel lock must, again, be held while a transfer is aborted
   (by calling usbd_abort_xfer()).  This happens while closing a MIDI
   interface that was open for reading, and midiclose(), in
   /sys/dev/midi.c, does not itself grab the lock.  Again, a KASSERT()
   fails for this situation.  I added code to grab and release the
   kernel lock to close_in_jack() in /sys/dev/usb/umidi.c.

3: The softc spinlock mutex must *not* be held during the above
   mentioned call to usbd_abort_xfer(), but midiclose() does hold it
   while calling the umidi driver's closing function.  This fails
   because usbd_abort_xfer() wants to relinquish the CPU to allow the
   hardware time to catch up, and a context switch is then attempted
   with the mutex held.  I added code to release and re-grab the mutex
   around the call to usbd_abort_xfer() in /sys/dev/usb/umidi.c.

4: After a MIDI device that was open for read has been closed (with
   the above fixes in place), reopening it for read fails because of
   the state it is left in by usbd_abort_xfer().  Specifically, the
   first attempt to start an input transfer on it returns
   USBD_CANCELLED, reflecting the aborted transfer.  This is benign,
   and I simply added that return code to the set of those that are
   accepted as non-errors by the umidi driver.

>How-To-Repeat:

Attempt to use a USB MIDI interface on -current, reading and writing
it both directly (/dev/rmidi?) and through the sequencer (/dev/music).
Observe that writing works, but reading crashes NetBSD.

>Fix:

Apply the following patch (thoroughly tested with two Roland UM-ONE):

Index: umidi.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/umidi.c,v
retrieving revision 1.55
diff -u -r1.55 umidi.c
--- umidi.c     23 Dec 2011 00:51:47 -0000      1.55
+++ umidi.c     1 Feb 2012 13:31:11 -0000
@@ -64,7 +64,7 @@
 #define DPRINTFN(n,x)  if (umididebug >= (n)) printf x
 #include <sys/time.h>
 static struct timeval umidi_tv;
-int    umididebug = 0;
+int    umididebug = 1;
 #else
 #define DPRINTF(x)
 #define DPRINTFN(n,x)
@@ -255,6 +255,7 @@
 error:
        aprint_error_dev(self, "disabled.\n");
        sc->sc_dying = 1;
+       KERNEL_UNLOCK_ONE(curlwp);
        return;
 }
 
@@ -349,7 +350,8 @@
        if ((mididev->flags & FREAD) && mididev->in_jack) {
                err = open_in_jack(mididev->in_jack, arg, iintr);
                if ( err != USBD_NORMAL_COMPLETION
-               &&   err != USBD_IN_PROGRESS )
+               &&   err != USBD_IN_PROGRESS
+               &&   err != USBD_CANCELLED )
                        goto bad;
        }
 
@@ -1125,11 +1127,14 @@
        jack->u.in.intr = intr;
        jack->opened = 1;
        if (ep->num_open++ == 0 && UE_GET_DIR(ep->addr)==UE_DIR_IN) {
+               KERNEL_LOCK(1, curlwp);
                err = start_input_transfer(ep);
                if (err != USBD_NORMAL_COMPLETION &&
-                   err != USBD_IN_PROGRESS) {
+                   err != USBD_IN_PROGRESS &&
+                   err != USBD_CANCELLED) {
                        ep->num_open--;
                }
+               KERNEL_UNLOCK_ONE(curlwp);
        }
 
        return err;
@@ -1165,10 +1170,20 @@
 static void
 close_in_jack(struct umidi_jack *jack)
 {
+       struct umidi_endpoint *ep;
+       struct umidi_softc *sc;
+
        if (jack->opened) {
+               ep = jack->endpoint;
+               sc = ep->sc;
+               KASSERT(mutex_owned(&sc->sc_lock));
                jack->opened = 0;
                if (--jack->endpoint->num_open == 0) {
-                   usbd_abort_pipe(jack->endpoint->pipe);
+                       KERNEL_LOCK(1, curlwp);
+                       mutex_exit(&sc->sc_lock);
+                       usbd_abort_pipe(ep->pipe);
+                       mutex_enter(&sc->sc_lock);
+                       KERNEL_UNLOCK_ONE(curlwp);
                }
        }
 }
@@ -1415,12 +1430,17 @@
 static usbd_status
 start_input_transfer(struct umidi_endpoint *ep)
 {
+       usbd_status rv;
+
+       DPRINTFN(200,("umidi in transfer: start %p length %u\n",
+           ep->buffer, ep->buffer_size));
        usbd_setup_xfer(ep->xfer, ep->pipe,
                        (usbd_private_handle)ep,
                        ep->buffer, ep->buffer_size,
                        USBD_SHORT_XFER_OK | USBD_NO_COPY,
                         USBD_NO_TIMEOUT, in_intr);
-       return usbd_transfer(ep->xfer);
+       rv = usbd_transfer(ep->xfer);
+       return rv;
 }
 
 static usbd_status


-tih
-- 
"The market" is a bunch of 28-year-olds who don't know anything. --Paul Krugman