[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
On Wed, 1 Feb 2012, Tom Ivar Helbekkmo wrote:
(I changed the subject btw and Cc'd gnats bugs so this should get attached
to the PR)
> 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().
this bit seems ok to me
> 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.
I think you meant usbd_abort_pipe() btw? but given that, seems ok..
> 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.
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..
and if usbd_abort_pipe() is relinquishing the CPU, what about the kernel
> 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.
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..
> -int umididebug = 0;
> +int umididebug = 1;
that is unrelated :)
> #define DPRINTF(x)
> #define DPRINTFN(n,x)
> @@ -255,6 +255,7 @@
> aprint_error_dev(self, "disabled.\n");
> sc->sc_dying = 1;
> + KERNEL_UNLOCK_ONE(curlwp);
and you didn't mention that (but it is needed :)
> @@ -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,
> 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?
(I don't have any MIDI hardware btw, no way to test any of this :)
Main Index |
Thread Index |