On Tue, 25 Jul 2006, David Brownell wrote: > ... except that as you noted later, that invariant was blown away by a > routine lower down in usbcore, which violates one of the cardinal locking > rules of not enabling IRQs on exit if they weren't enabled on entry:
At least the violation is documented! That's better than nothing. :-) > > /* Asynchronous unlinks of root-hub control URBs are legal, but they > > * don't do anything. Status URB unlinks must be made in process context > > * with interrupts enabled. > > */ > > That'd be a problem ... ** ANY ** urb can be unlinked asynchronously, > that's how the fundamental USB unlink primitive is defined. Are you referring to the first sentence in the comment? I don't think it's a problem. Asynchronous unlinks of root-hub control URBs _do_ succeed... but the URBs don't complete any faster than they would have otherwise. Hopefully the various HCDs process their control URBs with reasonable dispatch. Or are you referring to the second sentence in the comment? It doesn't mention asynchronous unlinks in particular; it just says that interrupts have to be enabled. > > I no longer remember the exact reason, but apparently the second sentence > > refers to the fact that del_timer_sync() is used when unlinking status > > URBs. No doubt that's a holdover from old code; > > The related 2.5.0-ish code (according to BitKeeper!) was: > > spin_lock_irqsave(&hcd_data_lock, flags); > del_timer_sync(&hcd->rh_timer); > hcd->rh_timer.data = 0; > spin_unlock_irqrestore(&hcd_data_lock, flags); > > Now, del_timer_sync() "must not be called from interrupt contexts" but > that's not going to be called from an interrupt. On SMP, that routine > will spin until the timer finishes running on any other CPU, which is > reasonable to do with IRQs disabled (at least in rare cases like this), > so I don't think that's incorrect. Okay. For some reason I thought del_timer_sync() always required the ability to sleep. Looks like I was wrong. > > in any case it clearly > > could be removed if we changed all the HCDs over to the new root-hub > > polling scheme since it is conditional on (!hcd->uses_new_polling). A > > few other changes would be needed as well, nothing serious. > > Modifying all the HCDs isn't especially straightforward though, so I'm not > sure how well that approach could work. I wonder how much modification would really be needed. Probably not much at all. We'd have to set the uses_new_polling and poll_rh flags, but that's trivial. More worrisome is potential changes in behavior. However the only real difference between the old polling scheme and the new one is that the new scheme calls hcd->hub_status_data() regardless of whether a status URB is queued or not. Since the HCD shouldn't care whether an URB is queued, it would only need to make sure that polling is turned off while it is suspended and turned back on when it resumes. That should be doable. > It'd be better to make that code use local_irq_save()/local_irq_restore(). > I'll give that a try later today; this is easy enough to reproduce with > rmmod. Yes, that's the simplest solution for now. > > My patch > > makes sure that it is called with interrupts already enabled, so the > > problem goes away. > > But then what would prevent the races that were previously prevented by > keeping IRQs disabled there?? What races? No spinlocks are held at that point, so any races exposed by my patch were already present on SMP systems. > Remember that the unlink code paths are especially tricky because they > need to defend against concurrent unlink from hcd_driver.remove (which is > what rmmod does, also pccardd in Matthias' case), usb_driver unlink calls, > and khubd cleanup during usb device unplug. Yes, worst case there can > be three CPUs racing each other ... I don't see how concurrent unlinks would cause any problems. The code is careful to acquire spinlocks where it matters. > The only common ground there seems to be "unlink can always be done with > irqs blocked". I agree it's bad in principle to require interrupts to be enabled at that spot. Alan Stern ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
