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

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

(I responded to gnats-bugs, but left out current-users.  Fixing that now.)

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