On Friday 14 November 2003 16:14, Alan Stern wrote: > 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.
Hi Alan, thanks for replying. > 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.) A quick fix involving minimal changes requires the BKL. I think that a proper fix also requires a clear idea of the future of usbfs. After all, at the bottom of it, many of the locking problems come from a mal-adapted API. One thought I had was that the usbfs files (001 etc) could become directories. The directory would contain a subdirectory for each interface (for the current configuration), and each interface directory would contain an endpoint file for each endpoint of the current altsetting. Sending an urb to an endpoint would require opening that endpoint and writing to it (or doing an ioctl). Configuration and altsetting changes would cause directory contents to change. What happens if you have an open endpoint when a change deletes that endpoint? Exactly the same thing that happens with other file systems when you have an open file that someone else deletes: you get to keep your copy until you close it, at which point it evaporates. The whole interface claiming/releasing business could be eliminated by saying (like for many device files) that endpoints can't be opened more than once. That way, if you open an endpoint you get exclusive access to it. To "claim an interface", you could just open each endpoint in it. If other types of locking are required, maybe a more standard file locking method could be used. Backwards compatibility could be maintained by allowing the traditional ioctl's on the top-level directory (001 etc). > 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. Any objection to usbdev->serialize becoming a rwsem? > Unfortunately these things are only incompletely implemented in usbcore, > and practically not at all in usbfs. There's still a long way to go. Yes. In the meantime I will try to find a fix suitable for the current 2.6 freeze. All the best, Duncan. ------------------------------------------------------- This SF. Net email is sponsored by: GoToMyPC GoToMyPC is the fast, easy and secure way to access your computer from any Web browser or wireless device. Click here to Try it Free! https://www.gotomypc.com/tr/OSDN/AW/Q4_2003/t/g22lp?Target=mm/g22lp.tmpl _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
