(unknown charset) a few usb issues related to edge cases

Hans Petter,

I think that I see some issues in the USB code that could cause problems in some
edge cases.
>From easiest to hardest:

1.  I think that currently there is a LOR in usb_bus_shutdown.  I think that the
following patch should fix it:
===========================================================================
--- a/sys/dev/usb/controller/usb_controller.c
+++ b/sys/dev/usb/controller/usb_controller.c
_at__at_ -479,6 +481,7 _at__at_ usb_bus_shutdown(struct usb_proc_msg *pm)

 	bus_generic_shutdown(bus->bdev);

+	USB_BUS_UNLOCK(bus);
 	usbd_enum_lock(udev);

 	err = usbd_set_config_index(udev, USB_UNCONFIG_INDEX);
_at__at_ -497,6 +500,7 _at__at_ usb_bus_shutdown(struct usb_proc_msg *pm)
 		(bus->methods->set_hw_power_sleep) (bus, USB_HW_POWER_SHUTDOWN);

 	usbd_enum_unlock(udev);
+	USB_BUS_LOCK(bus);
 }

 static void
===========================================================================
Otherwise there are a lot of nasty reports like:
lock order reversal: (sleepable after non-sleepable)
 1st 0xffffff80006b0688 ohci0 (ohci0) _at_
/usr/src/sys/dev/usb/controller/usb_controller.c:336
 2nd 0xfffffe00023cf070 USB config SX lock (USB config SX lock) _at_
/usr/src/sys/dev/usb/usb_device.c:2643

usbd_transfer_unsetup can sleep! with the following non-sleepable locks held:
exclusive sleep mutex ohci0 (ohci0) r = 0 (0xffffff80006b0688) locked _at_
/usr/src/sys/dev/usb/controller/usb_controller.c:336

2.  Somewhat related to the above.  I think that because the USB subsystem
implements the shutdown method and detaches all its drivers, then the ukbd
driver won't be able to properly handle the 'shutdown -h' case where the kernel
asks to "press any key to reboot" at the end.  Depending on which thread wins
the race (the one that executes the mainline shutdown code or the USB explore
thread that detaches USB devices) there will either an immediate reboot or a
later crash when any key is pressed.
This is not critical, but OTOH perhaps the USB subsystem doesn't have to do the
shutdown.  As far as I can see a lot of the drivers just do nothing for the
shutdown, for better or for worth.

A side note: perhaps it would be a good idea to pass the 'how' value as an
additional parameter to device_shutdown.

3.  Looking at usbd_transfer_poll I see that it touches a lot of locks,
including taking the bus lock.  As we've discussed before, this is not safe in
a particular context where the polling is supposed to be used - in the kdb/ddb
context.  If the lock is already taken by another thread, then instead of being
able to use a USB keyboard a user would get even less debug-able crash.  Also,
it seems that usbd_transfer_poll calls into the usual state machine with various
callbacks and dynamically made decisions about whether to execute some actions
directly or defer their execution to a different thread.  That code also touches
locks in various places.  I think that it would be more preferable to have a
method that does the job in a more straight-forward way, without touching any
locks, ignoring the usual code paths and assuming that no other treads are
running in parallel.  Ditto for the method to submit a request.

As a side note: we probably need a flag to mark certain things such as e.g. the
ukbd driver as "non recoverable", meaning that once those are used in the kdb
context then there is no safe way to go back to normal system operation.

What do you think?
Thank you.
-- 
Andriy Gapon