[PATCH] Re: [linux-usb-devel] unending timeouts (patch for 2.5.22 oops)

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/host/uhci-hcd.c

@@ -1665,37 +1665,15 @@ static int uhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb)
 
 	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 @@ static void stall_callback(unsigned long ptr)
 
 		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/host/usb-uhci-hcd.c

@@ -271,21 +271,15 @@ static int uhci_urb_enqueue (struct usb_hcd *hcd, struct urb *urb, int mem_flags
 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/host/usb-uhci-q.c

@@ -541,22 +541,6 @@ static void uhci_clean_iso_step1(struct uhci_hcd *uhci, urb_priv_t *urb_priv)
 	}
 }
 /*-------------------------------------------------------------------*/
-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 @@ static int uhci_unlink_urb_async (struct uhci_hcd *uhci, struct urb *urb, int mo
 	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 @@ static void uhci_unlink_urbs(struct uhci_hcd *uhci, struct usb_device *usb_dev,
 
 //		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 @@ static void uhci_check_timeouts(struct uhci_hcd *uhci)
 		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);
 		}