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

[Iain Hibbert <plunky%rya-online.net@localhost>]

On Sat, 4 Feb 2012, Tom Ivar Helbekkmo wrote:

> 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...?

Hm, not sure.. presumably it takes the lock so it can't be interrupted
while closing the device, and if it released it before calling the close
routine, that could conflict with another open call.  Probably it needs to
set some other "hands off" flag to prevent userland calling in..

> > 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.  :)

which KASSERT triggers, is it the one in usbd_transfer_complete() ?

> >  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()?

what I did (in ubt.c, but this method seems to be used throughout dev/usb)
is to open pipes and allocate transfers when the device is enabled, rather
than when the device is attached.. For umidi then, that would mean during
umidi_open() rather than umidi_attach(), and abort/close from
umidi_close()

> >  > -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.

except that having USB_DEBUG generally sets the other UXX_DEBUG flags in
usb.h (I see it doesn't for UMIDI, probably a separate issue)

iain