On Tuesday 25 July 2006 10:44 am, Alan Stern wrote:
> On Tue, 25 Jul 2006, David Brownell wrote:
>
> > Plus, this trace clearly shows the issue is with UHCI, not OHCI!
>
> No, not really. The trace says that the OHCI operations caused
> hcd_data_lock to be acquired in hcd_endpoint_disabled() with interrupts
> enabled,
Except that hcd_endpoint_disable() does a local_irq_disable() before it
kicks the HCD for urb unlink; so that lock would never acquired with IRQs
enabled.
... 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:
> In hcd_endpoint_disable(), while interrupts are disabled we call unlink1()
> which in turn may call usb_rh_urb_dequeue() -- in fact, _will_ call it for
> root-hub status URBs. Note, however, this comment at the top of
> usb_rh_urb_dequeue():
>
> /* 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.
> 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.
At some point in the last couple years that code morphed into its current
form, using local_irq_disable()/local_irq_enable() instead of saving
and restoring the correct IRQ context.
> 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.
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.
> At any rate, it's obvious that the problem was caused when
> usb_rh_urb_dequeue() unconditionally re-enabled interrupts.
OK, that seems to be correct ... rh_urb_dequeue() is the problem,
because it should instead be restoring the previous state.
You have convinced me that this is indeed a case where the lock
tool reported a real bug, albeit in an excessively cryptic form!
That's the second time this year I've been glad to have tools that
do runtime lock validation. :)
> 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??
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 ...
The only common ground there seems to be "unlink can always be done with
irqs blocked".
- Dave
-------------------------------------------------------------------------
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
