>>>Summary:
>>>Seems like a bug in usbdevfs. The attached program is enough to
>>>trigger it.
>>
>>Sure does :(
>>
>>Don't remember if I've asked, but does this also happen on 2.5?
>
>
> It does with 2.5.22 (with an oops!). Here is the oops:
> ...
> Trace; cc8a0427 <[usb-uhci-hcd]uhci_urb_dequeue+53/5c>
> Trace; cc8913cf <[usbcore]hcd_unlink_urb+143/1cc>
> Trace; cc891890 <[usbcore]usb_api_blocking_completion+0/20>
> Trace; cc891882 <[usbcore]usb_unlink_urb+26/34>
> ...
>
> Code; cc89f4c0 <[usb-uhci-hcd]uhci_unlink_urb_sync+90/118>
> 00000000 <_EIP>:
> Code; cc89f4c0 <[usb-uhci-hcd]uhci_unlink_urb_sync+90/118> <=====
> 0: 8b 45 00 mov 0x0(%ebp),%eax <=====
Ah, so both of the "hcd-ized" UHCI drivers have a common bug:
they've got logic to look at the USB_ASYNC_UNLINK flag and
block unless it's clear ... but the hcd framework is already
handling the synchronous behavior, so that's wrong.
Try to repeat that with the patch I've attached, which rips
out that duplicated code ... and so should at least get rid of
that oops, even if it doesn't entirely fix the timeout issue.
(Or: try with either the OHCI driver, or with the EHCI driver
through a USB 2.0 hub, if you have appropriate hardware.)
- Dave
p.s. Disclaimer about this patch: all it does is rip out
code and make it compile without warnings, but I've
not tested it otherwise. There's a possiblity it'll
uncover latent issues on the other code path, but then
that's exactly why we only want one unlink code path
inside the HCDs! So Greg, please merge anyway ...
--- ./drivers/usb-dist/host/uhci-hcd.c Tue Jun 18 18:06:27 2002
+++ ./drivers/usb/host/uhci-hcd.c Fri Jun 21 05:26:02 2002
@@ -1665,37 +1665,15 @@
uhci_unlink_generic(uhci, urb);
- if (urb->transfer_flags & USB_ASYNC_UNLINK) {
- urbp->status = urb->status = -ECONNABORTED;
+ spin_lock(&uhci->urb_remove_list_lock);
- spin_lock(&uhci->urb_remove_list_lock);
-
- /* If we're the first, set the next interrupt bit */
- if (list_empty(&uhci->urb_remove_list))
- uhci_set_next_interrupt(uhci);
-
- list_add(&urbp->urb_list, &uhci->urb_remove_list);
-
- spin_unlock(&uhci->urb_remove_list_lock);
-
- spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
- } else {
- urb->status = -ENOENT;
-
- spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
-
- if (in_interrupt()) { /* wait at least 1 frame */
- static int errorcount = 10;
-
- if (errorcount--)
- dbg("uhci_urb_dequeue called from interrupt for urb
%p", urb);
- udelay(1000);
- } else
- schedule_timeout(1+1*HZ/1000);
-
- uhci_finish_urb(hcd, urb);
- }
+ /* If we're the first, set the next interrupt bit */
+ if (list_empty(&uhci->urb_remove_list))
+ uhci_set_next_interrupt(uhci);
+ list_add(&urbp->urb_list, &uhci->urb_remove_list);
+ spin_unlock(&uhci->urb_remove_list_lock);
+ spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
return 0;
}
@@ -1788,7 +1766,7 @@
tmp = tmp->next;
- u->transfer_flags |= USB_ASYNC_UNLINK | USB_TIMEOUT_KILLED;
+ u->transfer_flags |= USB_TIMEOUT_KILLED;
uhci_urb_dequeue(hcd, u);
}
--- ./drivers/usb-dist/host/usb-uhci-hcd.c Thu May 30 15:16:45 2002
+++ ./drivers/usb/host/usb-uhci-hcd.c Fri Jun 21 05:04:33 2002
@@ -271,21 +271,15 @@
static int uhci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
{
unsigned long flags=0;
- struct uhci_hcd *uhci;
+ struct uhci_hcd *uhci = hcd_to_uhci (hcd);
+ int ret;
dbg("uhci_urb_dequeue called for %p",urb);
- uhci = hcd_to_uhci (hcd);
-
- if (urb->transfer_flags & USB_ASYNC_UNLINK) {
- int ret;
- spin_lock_irqsave (&uhci->urb_list_lock, flags);
- ret = uhci_unlink_urb_async(uhci, urb, UNLINK_ASYNC_STORE_URB);
- spin_unlock_irqrestore (&uhci->urb_list_lock, flags);
- return ret;
- }
- else
- return uhci_unlink_urb_sync(uhci, urb);
+ spin_lock_irqsave (&uhci->urb_list_lock, flags);
+ ret = uhci_unlink_urb_async(uhci, urb, UNLINK_ASYNC_STORE_URB);
+ spin_unlock_irqrestore (&uhci->urb_list_lock, flags);
+ return ret;
}
/*--------------------------------------------------------------------------*/
static int uhci_get_frame (struct usb_hcd *hcd)
--- ./drivers/usb-dist/host/usb-uhci-q.c Thu May 30 15:16:45 2002
+++ ./drivers/usb/host/usb-uhci-q.c Fri Jun 21 05:20:31 2002
@@ -541,22 +541,6 @@
}
}
/*-------------------------------------------------------------------*/
-static void uhci_clean_iso_step2(struct uhci_hcd *uhci, urb_priv_t *urb_priv)
-{
- struct list_head *p;
- uhci_desc_t *td;
- int now=UHCI_GET_CURRENT_FRAME(uhci);
-
- dbg("uhci_clean_iso_step2");
- while ((p = urb_priv->desc_list.next) != &urb_priv->desc_list) {
- td = list_entry (p, uhci_desc_t, desc_list);
- list_del (p);
- INIT_LIST_HEAD(&td->horizontal);
- list_add_tail (&td->horizontal, &uhci->free_desc_td);
- td->last_used=now;
- }
-}
-/*-------------------------------------------------------------------*/
/* mode: CLEAN_TRANSFER_NO_DELETION: unlink but no deletion mark (step 1 of
async_unlink)
CLEAN_TRANSFER_REGULAR: regular (unlink/delete-mark)
CLEAN_TRANSFER_DELETION_MARK: deletion mark for QH (step 2 of async_unlink)
@@ -759,44 +743,6 @@
return 0; // completion will follow
}
/*-------------------------------------------------------------------*/
-// kills an urb by unlinking descriptors and waiting for at least one frame
-static int uhci_unlink_urb_sync (struct uhci_hcd *uhci, struct urb *urb)
-{
- uhci_desc_t *qh;
- urb_priv_t *urb_priv;
- unsigned long flags=0;
-
- spin_lock_irqsave (&uhci->urb_list_lock, flags);
-// err("uhci_unlink_urb_sync %p, %i",urb,urb->status);
-
- // move descriptors out the the running chains, dequeue urb
- uhci_unlink_urb_async(uhci, urb, UNLINK_ASYNC_DONT_STORE);
-
- urb_priv = urb->hcpriv;
-
- spin_unlock_irqrestore (&uhci->urb_list_lock, flags);
-
- // cleanup the rest
- switch (usb_pipetype (urb->pipe)) {
- case PIPE_INTERRUPT:
- case PIPE_ISOCHRONOUS:
- uhci_wait_ms(1);
- uhci_clean_iso_step2(uhci, urb_priv);
- break;
-
- case PIPE_BULK:
- case PIPE_CONTROL:
- qh = list_entry (urb_priv->desc_list.next, uhci_desc_t, desc_list);
- uhci_clean_transfer(uhci, urb, qh, CLEAN_TRANSFER_DELETION_MARK);
- uhci_wait_ms(1);
- }
- urb->status = -ENOENT; // mark urb as killed
-
- finish_urb(uhci,urb);
-
- return 0;
-}
-/*-------------------------------------------------------------------*/
// unlink urbs for specific device or all devices
static void uhci_unlink_urbs(struct uhci_hcd *uhci, struct usb_device *usb_dev, int
remove_all)
{
@@ -816,8 +762,6 @@
// err("unlink urb: %p, dev %p, ud %p", urb, usb_dev,urb->dev);
- //urb->transfer_flags |=USB_ASYNC_UNLINK;
-
if (remove_all || (usb_dev == urb->dev)) {
spin_unlock_irqrestore (&uhci->urb_list_lock, flags);
err("forced removing of queued URB %p due to disconnect",urb);
@@ -850,7 +794,7 @@
type = usb_pipetype (urb->pipe);
if ( urb->timeout && time_after(jiffies, hcpriv->started +
urb->timeout)) {
- urb->transfer_flags |= USB_TIMEOUT_KILLED | USB_ASYNC_UNLINK;
+ urb->transfer_flags |= USB_TIMEOUT_KILLED;
async_dbg("uhci_check_timeout: timeout for %p",urb);
uhci_unlink_urb_async(uhci, urb, UNLINK_ASYNC_STORE_URB);
}
