NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: PR/45909: Use of MIDI over USB interfaces crashes NetBSD
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
Home |
Main Index |
Thread Index |
Old Index