On Tuesday 25 July 2006 2:41 pm, Alan Stern wrote:
> On Tue, 25 Jul 2006, David Brownell wrote:
>
> > > /* 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.
>
> ...
> 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.
Yes. One of the basic reasons to be async is so you can run with
irqs disabled. I should probably have written "unlinked with irqs
disabled".
And the attached patch works just fine, removing that inappropriate
restriction. At least in the "rmmod" case I saw.
> > > 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.
Given that I've seen OHCI quirks in that area -- allowed by the spec,
it seems, just implementation differences the driver needs to know
about and handle -- and that there are several HCDs without any
easily-tested PCI versions, it's hard to say anything except that
it'd be impractical to guarantee it for, say
> > 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.
Matthias, please see if this patch works for you too.
- Dave
This fixes a locking problem reported by the new lock debug tools which, while
very cryptic, gave Alan Stern enough info to finger the problem: a routine
wrongly enabled IRQs on exit, by using the wrong locking/irq primitives.
This patch switches back to the primitives which save/restore IRQ flags.
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
Index: g26/drivers/usb/core/hcd.c
===================================================================
--- g26.orig/drivers/usb/core/hcd.c 2006-07-03 10:45:18.000000000 -0700
+++ g26/drivers/usb/core/hcd.c 2006-07-25 14:00:14.000000000 -0700
@@ -632,31 +632,33 @@ static int rh_urb_enqueue (struct usb_hc
/*-------------------------------------------------------------------------*/
-/* 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.
+/* Root hub control URBs are synchronous, so unlinking would be pointless.
+ * Status URBs may not be unlinked from interrupts (del_timer_sync rule).
*/
static int usb_rh_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
{
+ unsigned long flags;
+
if (usb_pipeendpoint(urb->pipe) == 0) { /* Control URB */
if (in_interrupt())
return 0; /* nothing to do */
- spin_lock_irq(&urb->lock); /* from usb_kill_urb */
- ++urb->reject;
- spin_unlock_irq(&urb->lock);
+ spin_lock_irqsave(&urb->lock, flags);
+ ++urb->reject; /* like usb_kill_urb */
+ spin_unlock_irqrestore(&urb->lock, flags);
wait_event(usb_kill_urb_queue,
atomic_read(&urb->use_count) == 0);
- spin_lock_irq(&urb->lock);
+ spin_lock_irqsave(&urb->lock, flags);
--urb->reject;
- spin_unlock_irq(&urb->lock);
+ spin_unlock_irqrestore(&urb->lock, flags);
} else { /* Status URB */
if (!hcd->uses_new_polling)
del_timer_sync (&hcd->rh_timer);
- local_irq_disable ();
+
+ local_irq_save (flags);
spin_lock (&hcd_root_hub_lock);
if (urb == hcd->status_urb) {
hcd->status_urb = NULL;
@@ -666,7 +668,7 @@ static int usb_rh_urb_dequeue (struct us
spin_unlock (&hcd_root_hub_lock);
if (urb)
usb_hcd_giveback_urb (hcd, urb, NULL);
- local_irq_enable ();
+ local_irq_restore (flags);
}
return 0;
-------------------------------------------------------------------------
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
