Re: PR/45909: Use of MIDI over USB interfaces crashes NetBSD

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

The following reply was made to PR kern/45909; it has been noted by GNATS.

From: Tom Ivar Helbekkmo <tih%hamartun.priv.no@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: kern-bug-people%netbsd.org@localhost,  gnats-admin%netbsd.org@localhost,  
netbsd-bugs%netbsd.org@localhost
Subject: Re: PR/45909: Use of MIDI over USB interfaces crashes NetBSD
Date: Sat, 04 Feb 2012 15:45:39 +0100

 Iain Hibbert <plunky%rya-online.net@localhost> writes:
 
 >  I think you meant usbd_abort_pipe() btw? but given that, seems ok..
 
 Yes, of course.  :)
 
 >  I don't think its right to release/aquire a lock that a higher layer is
 >  holding to protect its access to the resource, since the resource can
 >  change from underneath it..
 
 Agreed.  It looks ugly as sin, too.  I just did what I could with
 umidi.c alone - I didn't want to start messing with higher level code,
 in case I broke something for other drivers.  For all I know, it's
 possible that the thread lock could be released in midiclose(), before
 the hw->close() call, since I suspect that it's really only needed for
 the while() { cv_wait() } loop...?
 
 >  and if usbd_abort_pipe() is relinquishing the CPU, what about the kernel
 >  lock?
 
 Yeah, I don't understand that bit at all.  You get a KASSERT failure if
 you call usbd_abort_pipe() without holding the kernel lock, though - and
 another one if you *are* holding the thread lock, when it tries to do a
 context change with a mutex held.  This challenges my understanding of
 what the kernel lock is.  :)
 
 >  is there another way to fix this, so that CANCELLED is not returned? IIRC
 >  when I've used usbd_abort_pipe() I also closed the pipe and reopened it
 >  for the next use, not sure if that is actually required..
 
 You mean call usbd_abort_pipe(), usbd_close_pipe() and usbd_open_pipe()
 in sequence within close_in_jack()?
 
 >  > -int      umididebug = 0;
 >  > +int      umididebug = 1;
 >  
 >  that is unrelated :)
 
 Yeah - but it's silly to have it set to 0, since then just building a
 kernel with the UMIDI_DEBUG option doesn't give you any debug output,
 which IMHO breaks the Principle of Least Astonishment.
 
 >  >  #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;
 >  
 >  and you didn't mention that (but it is needed :)
 
 Ah, yes.  Someone must have forgotten to put it in at some time, and I
 fixed it when I noticed it.
 
 >  > @@ -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;
 >  >  }
 >  
 >  and this probably not needed?
 
 Heh, no.  Forgot about that.  It's left over from an experiment that
 didn't go anywhere useful.  :)
 
 -tih
 -- 
 "The market" is a bunch of 28-year-olds who don't know anything. --Paul Krugman