Date: Mon, 19 Dec 2011 14:16:17 +0200 From: Andriy Gapon <avg@FreeBSD.org> To: Hans Petter Selasky <hselasky@FreeBSD.org> Cc: FreeBSD current <freebsd-current@FreeBSD.org>, freebsd-usb@FreeBSD.org Subject: a few usb issues related to edge cases Message-ID: <4EEF2B11.6080802@FreeBSD.org>
next in thread | raw e-mail | index | archive | help
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 @@ -479,6 +481,7 @@ 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); @@ -497,6 +500,7 @@ 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) @ /usr/src/sys/dev/usb/controller/usb_controller.c:336 2nd 0xfffffe00023cf070 USB config SX lock (USB config SX lock) @ /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 @ /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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4EEF2B11.6080802>