Messages in this thread | | | From | Duncan Sands <> | Subject | Re: [linux-usb-devel] Re: [OOPS, usbcore, releaseintf] 2.6.0-test10-mm1 | Date | Mon, 8 Dec 2003 22:55:43 +0100 |
| |
> > You can't keep the ps->devsem lock and use ps->dev->serialize, because it > > leads to deadlock. > > How so? Remember that I am almost totally unfamiliar with the details of > the usbfs code. Are you saying there are places where the driver holds > one lock and needs to acquire the other and vice versa?
Yes. ps->devsem is used to protect against disconnection: all top level routines take it (as a read lock), and in driver_disconnect it is taken as a write lock. Top level routines call lower level routines which sometimes need to take dev->serialize (and do already in several places).
Thus: ps->devsem taken, then dev->serialize.
However, dev->serialize is taken by the USB core before calling driver_disconnect.
Thus: dev->serialize taken, then ps->devsem.
> > Actually, simply replacing ps->devsem with ps->dev->serialize > > cannot lead to any new deadlocks, it makes deadlocks that could > > occasionally happen always happen (such deadlocks exist right now in > > usbfs). Some of the current deadlocks can be eliminated without giving > > up ps->devsem, but not all. So the question is: must ps->dev->serialize > > be used? > > It must be held when you call usb_reset_configuration(). It must _not_ be > held when you call usb_set_configuration(). For usb_reset_device() right > now you must not hold it, although that may change in the future. For > usb_unbind_interface() you must not hold it. There's a note that > usb_driver_claim_interface() grabs the BKL for some reason having to do > with usbfs -- no doubt when usbfs is fixed that won't be needed and the > caller will be required to hold dev->serialize instead.
Right. And why should (for example) dev->serialize not be held when it calls usb_set_configuration? - because usb_set_configuration takes dev->serialize. This is one of the places I mentioned above where deadlock can occur right now.
> If you call usb_ifnum_to_if() you ought to hold the serialize lock; > otherwise the configuration might change out from under you. But it's not > necessary. Likewise for usb_epnum_to_ep_desc if you're looking up an > endpoint that isn't part of an interface you have bound.
Why isn't it necessary? As far as I can see it is vital.
> > > Are they any reasons for not keeping ps->devsem? Since usbfs generally > > > acts as a driver and drivers generally don't have to concern themselves > > > with usbdev->serialize (the core handles it for them), shouldn't usbfs > > > also be able to ignore ps->dev->serialize? > > > > No, because it needs to do operations on interfaces it hasn't claimed > > (such as looking them up and claiming them). This is why it needs to > > protect itself, at least momentarily, against configurations shifting > > under it. This can be done by using the BKL more. However it can be > > done more simply using ps->dev->serialize (in fact it is simpler than > > what is there now). > > That agrees with my assessment. It ought to be possible to remove these > references to the BKL in favor of ps->dev->serialize.
Yes, and that is what my patch does. And due to the above problem with deadlock it replaces ps->devsem with ps->dev->serialize everywhere.
> > By the way, if it is somehow fatal to do usb_put_dev after disconnect, > > what is the point of referencing counting at all? You might as well > > free up the usb_device structure immediately after disconnect, since > > there is sure to be a reference before disconnect, and (apparently) > > there had better not be a reference after disconnect... > > There's some sort of misunderstanding here. It's not fatal to do > usb_put_dev() after disconnect, provided you called usb_get_dev() earlier. > I'm not sure what the cause was of the oops you were getting, but it > wasn't that.
It was AFAICS, though of course it shouldn't be.
Ciao,
Duncan. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |