On Fri, 14 Nov 2003, Duncan Sands wrote: > Hi Alan, > > > Usbfs is full of locking errors. In particular, there's no excuse for it > > holding the BKL. > > I'm afraid there is. > > > In this case, the simplest solution is simply to insert a test for > > actconfig != NULL before it gets dereferenced. To make certain that > > actconfig won't change out from under you, be sure to hold dev->serialize > > during that test. > > No, it is wrong to do that because it may deadlock (the existing use of > dev->serialize in releaseintf is wrong because of this): driver_disconnect > is called with dev->serialize held and takes ps->devsem, as it must. > However almost all routines in devio.c are called with ps->devsem taken, > so if they take dev->serialize then they may deadlock with driver_disconnect > which holds dev->serialize and is waiting for ps->devsem. > > > The real problem is that usbfs is trying to unbind a driver after usbcore > > has already done so on its behalf. A better fix would be to repair > > driver_disconnect() and the USBDEVFS_DISCONNECT case in proc_ioctl() > > to make them use the > > > > test_and_clear_bit(intf, &ps->ifclaimed) > > > > test as they should. > > Sure, but that is not enough. In fact the whole thing is a mess. It seems to > me that devio.c routines need to be able to take dev->serialize. The only > reason for ps->devsem is to protect against disconnect (or being bumped > off the interface by releaseintf - in fact they are not protected against this > because proc_releaseinterface is called with only down_read(&ps->devsem) > and not down_write!). I think the simplest solution is to throw away > ps->devsem, optionally convert dev->serialize to a rw semaphore for > efficiency reasons (probably doesn't gain you much), and use dev->serialize > where ps->devsem was used before. There are a few special cases in > devio.c which will need to be tweaked in order for this not to cause deadlocks. > > Duncan.
I'm not nearly as familiar with the details of devio.c as you are, so I can't answer the points you've raised. However, you have done a very good job of confirming my initial statement, that locking in usbfs is full of errors. (And I still think we should never be using the BKL -- if everything else was done properly there would no need for it.) So I won't tell you what to do; I'll just confirm what the rest of usbcore should expect. Basically, changes to the device configuration or state (connected vs. disconnected) may happen at any time but only under the protection of usbdev->serialize. In addition, creation/removal of interfaces (as would be caused by a configuration change) and binding/unbinding of drivers to interfaces require holding the driver model's usb subsystem rwsem. During probe and disconnect processing this is done automatically by the driver model core. Unfortunately these things are only incompletely implemented in usbcore, and practically not at all in usbfs. There's still a long way to go. Alan Stern ------------------------------------------------------- This SF.Net email sponsored by: ApacheCon 2003, 16-19 November in Las Vegas. Learn firsthand the latest developments in Apache, PHP, Perl, XML, Java, MySQL, WebDAV, and more! http://www.apachecon.com/ _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
