USB: make HCDs responsible for managing endpoint queues

This patch (as954) implements a suggestion of David Brownell's.  Now
the host controller drivers are responsible for linking and unlinking
URBs to/from their endpoint queues.  This eliminates the possiblity of
strange situations where usbcore thinks an URB is linked but the HCD
thinks it isn't.  It also means HCDs no longer have to check for URBs
being dequeued before they were fully enqueued.

In addition to the core changes, this requires changing every host
controller driver and the root-hub URB handler.  For the most part the
required changes are fairly small; drivers have to call
usb_hcd_link_urb_to_ep() in their urb_enqueue method,
usb_hcd_check_unlink_urb() in their urb_dequeue method, and
usb_hcd_unlink_urb_from_ep() before giving URBs back.  A few HCDs make
matters more complicated by the way they split up the flow of control.

In addition some method interfaces get changed.  The endpoint argument
for urb_enqueue is now redundant so it is removed.  The unlink status
is required by usb_hcd_check_unlink_urb(), so it has been added to
urb_dequeue.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: David Brownell <david-b@pacbell.net>
CC: Olav Kongas <ok@artecdesign.ee>
CC: Tony Olech <tony.olech@elandigitalsystems.com>
CC: Yoshihiro Shimoda <shimoda.yoshihiro@renesas.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

drivers/usb/core/hcd.h

@@ -189,11 +189,10 @@ struct hc_driver {
 	int	(*get_frame_number) (struct usb_hcd *hcd);
 
 	/* manage i/o requests, device state */
-	int	(*urb_enqueue) (struct usb_hcd *hcd,
-					struct usb_host_endpoint *ep,
-					struct urb *urb,
-					gfp_t mem_flags);
-	int	(*urb_dequeue) (struct usb_hcd *hcd, struct urb *urb);
+	int	(*urb_enqueue)(struct usb_hcd *hcd,
+				struct urb *urb, gfp_t mem_flags);
+	int	(*urb_dequeue)(struct usb_hcd *hcd,
+				struct urb *urb, int status);
 
 	/* hw synch, freeing endpoint resources that urb_dequeue can't */
 	void 	(*endpoint_disable)(struct usb_hcd *hcd,
@@ -211,6 +210,11 @@ struct hc_driver {
 		/* Needed only if port-change IRQs are level-triggered */
 };
 
+extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb);
+extern int usb_hcd_check_unlink_urb(struct usb_hcd *hcd, struct urb *urb,
+		int status);
+extern void usb_hcd_unlink_urb_from_ep(struct usb_hcd *hcd, struct urb *urb);
+
 extern int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags);
 extern int usb_hcd_unlink_urb (struct urb *urb, int status);
 extern void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb);

drivers/usb/gadget/dummy_hcd.c

@@ -962,13 +962,13 @@ static struct platform_driver dummy_udc_driver = {
 
 static int dummy_urb_enqueue (
 	struct usb_hcd			*hcd,
-	struct usb_host_endpoint	*ep,
 	struct urb			*urb,
 	gfp_t				mem_flags
 ) {
 	struct dummy	*dum;
 	struct urbp	*urbp;
 	unsigned long	flags;
+	int		rc;
 
 	if (!urb->transfer_buffer && urb->transfer_buffer_length)
 		return -EINVAL;
@@ -980,6 +980,11 @@ static int dummy_urb_enqueue (
 
 	dum = hcd_to_dummy (hcd);
 	spin_lock_irqsave (&dum->lock, flags);
+	rc = usb_hcd_link_urb_to_ep(hcd, urb);
+	if (rc) {
+		kfree(urbp);
+		goto done;
+	}
 
 	if (!dum->udev) {
 		dum->udev = urb->dev;
@@ -997,22 +1002,28 @@ static int dummy_urb_enqueue (
 		mod_timer (&dum->timer, jiffies + 1);
 
 	spin_unlock_irqrestore (&dum->lock, flags);
-	return 0;
+ done:
+	return rc;
 }
 
-static int dummy_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
+static int dummy_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 {
 	struct dummy	*dum;
 	unsigned long	flags;
+	int		rc;
 
 	/* giveback happens automatically in timer callback,
 	 * so make sure the callback happens */
 	dum = hcd_to_dummy (hcd);
 	spin_lock_irqsave (&dum->lock, flags);
-	if (dum->rh_state != DUMMY_RH_RUNNING && !list_empty(&dum->urbp_list))
+
+	rc = usb_hcd_check_unlink_urb(hcd, urb, status);
+	if (!rc && dum->rh_state != DUMMY_RH_RUNNING &&
+			!list_empty(&dum->urbp_list))
 		mod_timer (&dum->timer, jiffies);
+
 	spin_unlock_irqrestore (&dum->lock, flags);
-	return 0;
+	return rc;
 }
 
 static void maybe_set_status (struct urb *urb, int status)
@@ -1511,6 +1522,7 @@ static void dummy_timer (unsigned long _dum)
 		if (ep)
 			ep->already_seen = ep->setup_stage = 0;
 
+		usb_hcd_unlink_urb_from_ep(dummy_to_hcd(dum), urb);
 		spin_unlock (&dum->lock);
 		usb_hcd_giveback_urb (dummy_to_hcd(dum), urb);
 		spin_lock (&dum->lock);

drivers/usb/host/ehci-hcd.c

@@ -719,7 +719,6 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
  */
 static int ehci_urb_enqueue (
 	struct usb_hcd	*hcd,
-	struct usb_host_endpoint *ep,
 	struct urb	*urb,
 	gfp_t		mem_flags
 ) {
@@ -734,12 +733,12 @@ static int ehci_urb_enqueue (
 	default:
 		if (!qh_urb_transaction (ehci, urb, &qtd_list, mem_flags))
 			return -ENOMEM;
-		return submit_async (ehci, ep, urb, &qtd_list, mem_flags);
+		return submit_async(ehci, urb, &qtd_list, mem_flags);
 
 	case PIPE_INTERRUPT:
 		if (!qh_urb_transaction (ehci, urb, &qtd_list, mem_flags))
 			return -ENOMEM;
-		return intr_submit (ehci, ep, urb, &qtd_list, mem_flags);
+		return intr_submit(ehci, urb, &qtd_list, mem_flags);
 
 	case PIPE_ISOCHRONOUS:
 		if (urb->dev->speed == USB_SPEED_HIGH)
@@ -777,13 +776,18 @@ static void unlink_async (struct ehci_hcd *ehci, struct ehci_qh *qh)
  * completions normally happen asynchronously
  */
 
-static int ehci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
+static int ehci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci (hcd);
 	struct ehci_qh		*qh;
 	unsigned long		flags;
+	int			rc;
 
 	spin_lock_irqsave (&ehci->lock, flags);
+	rc = usb_hcd_check_unlink_urb(hcd, urb, status);
+	if (rc)
+		goto done;
+
 	switch (usb_pipetype (urb->pipe)) {
 	// case PIPE_CONTROL:
 	// case PIPE_BULK:
@@ -838,7 +842,7 @@ static int ehci_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
 	}
 done:
 	spin_unlock_irqrestore (&ehci->lock, flags);
-	return 0;
+	return rc;
 }
 
 /*-------------------------------------------------------------------------*/

drivers/usb/host/ehci-q.c

@@ -262,6 +262,7 @@ __acquires(ehci->lock)
 #endif
 
 	/* complete() can reenter this HCD */
+	usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
 	spin_unlock (&ehci->lock);
 	usb_hcd_giveback_urb (ehci_to_hcd(ehci), urb);
 	spin_lock (&ehci->lock);
@@ -913,7 +914,6 @@ static struct ehci_qh *qh_append_tds (
 static int
 submit_async (
 	struct ehci_hcd		*ehci,
-	struct usb_host_endpoint *ep,
 	struct urb		*urb,
 	struct list_head	*qtd_list,
 	gfp_t			mem_flags
@@ -922,18 +922,18 @@ submit_async (
 	int			epnum;
 	unsigned long		flags;
 	struct ehci_qh		*qh = NULL;
-	int			rc = 0;
+	int			rc;
 
 	qtd = list_entry (qtd_list->next, struct ehci_qtd, qtd_list);
-	epnum = ep->desc.bEndpointAddress;
+	epnum = urb->ep->desc.bEndpointAddress;
 
 #ifdef EHCI_URB_TRACE
 	ehci_dbg (ehci,
 		"%s %s urb %p ep%d%s len %d, qtd %p [qh %p]\n",
 		__FUNCTION__, urb->dev->devpath, urb,
 		epnum & 0x0f, (epnum & USB_DIR_IN) ? "in" : "out",
 		urb->transfer_buffer_length,
-		qtd, ep->hcpriv);
+		qtd, urb->ep->hcpriv);
 #endif
 
 	spin_lock_irqsave (&ehci->lock, flags);
@@ -942,9 +942,13 @@ submit_async (
 		rc = -ESHUTDOWN;
 		goto done;
 	}
+	rc = usb_hcd_link_urb_to_ep(ehci_to_hcd(ehci), urb);
+	if (unlikely(rc))
+		goto done;
 
-	qh = qh_append_tds (ehci, urb, qtd_list, epnum, &ep->hcpriv);
+	qh = qh_append_tds(ehci, urb, qtd_list, epnum, &urb->ep->hcpriv);
 	if (unlikely(qh == NULL)) {
+		usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
 		rc = -ENOMEM;
 		goto done;
 	}

drivers/usb/host/ehci-sched.c

@@ -797,31 +797,33 @@ static int qh_schedule(struct ehci_hcd *ehci, struct ehci_qh *qh)
 
 static int intr_submit (
 	struct ehci_hcd		*ehci,
-	struct usb_host_endpoint *ep,
 	struct urb		*urb,
 	struct list_head	*qtd_list,
 	gfp_t			mem_flags
 ) {
 	unsigned		epnum;
 	unsigned long		flags;
 	struct ehci_qh		*qh;
-	int			status = 0;
+	int			status;
 	struct list_head	empty;
 
 	/* get endpoint and transfer/schedule data */
-	epnum = ep->desc.bEndpointAddress;
+	epnum = urb->ep->desc.bEndpointAddress;
 
 	spin_lock_irqsave (&ehci->lock, flags);
 
 	if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE,
 			&ehci_to_hcd(ehci)->flags))) {
 		status = -ESHUTDOWN;
-		goto done;
+		goto done_not_linked;
 	}
+	status = usb_hcd_link_urb_to_ep(ehci_to_hcd(ehci), urb);
+	if (unlikely(status))
+		goto done_not_linked;
 
 	/* get qh and force any scheduling errors */
 	INIT_LIST_HEAD (&empty);
-	qh = qh_append_tds (ehci, urb, &empty, epnum, &ep->hcpriv);
+	qh = qh_append_tds(ehci, urb, &empty, epnum, &urb->ep->hcpriv);
 	if (qh == NULL) {
 		status = -ENOMEM;
 		goto done;
@@ -832,13 +834,16 @@ static int intr_submit (
 	}
 
 	/* then queue the urb's tds to the qh */
-	qh = qh_append_tds (ehci, urb, qtd_list, epnum, &ep->hcpriv);
+	qh = qh_append_tds(ehci, urb, qtd_list, epnum, &urb->ep->hcpriv);
 	BUG_ON (qh == NULL);
 
 	/* ... update usbfs periodic stats */
 	ehci_to_hcd(ehci)->self.bandwidth_int_reqs++;
 
 done:
+	if (unlikely(status))
+		usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
+done_not_linked:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 	if (status)
 		qtd_list_free (ehci, urb, qtd_list);
@@ -1686,12 +1691,19 @@ static int itd_submit (struct ehci_hcd *ehci, struct urb *urb,
 	/* schedule ... need to lock */
 	spin_lock_irqsave (&ehci->lock, flags);
 	if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE,
-			       &ehci_to_hcd(ehci)->flags)))
+			       &ehci_to_hcd(ehci)->flags))) {
 		status = -ESHUTDOWN;
-	else
-		status = iso_stream_schedule (ehci, urb, stream);
+		goto done_not_linked;
+	}
+	status = usb_hcd_link_urb_to_ep(ehci_to_hcd(ehci), urb);
+	if (unlikely(status))
+		goto done_not_linked;
+	status = iso_stream_schedule(ehci, urb, stream);
 	if (likely (status == 0))
 		itd_link_urb (ehci, urb, ehci->periodic_size << 3, stream);
+	else
+		usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
+done_not_linked:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 done:
@@ -2049,12 +2061,19 @@ static int sitd_submit (struct ehci_hcd *ehci, struct urb *urb,
 	/* schedule ... need to lock */
 	spin_lock_irqsave (&ehci->lock, flags);
 	if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE,
-			       &ehci_to_hcd(ehci)->flags)))
+			       &ehci_to_hcd(ehci)->flags))) {
 		status = -ESHUTDOWN;
-	else
-		status = iso_stream_schedule (ehci, urb, stream);
+		goto done_not_linked;
+	}
+	status = usb_hcd_link_urb_to_ep(ehci_to_hcd(ehci), urb);
+	if (unlikely(status))
+		goto done_not_linked;
+	status = iso_stream_schedule(ehci, urb, stream);
 	if (status == 0)
 		sitd_link_urb (ehci, urb, ehci->periodic_size << 3, stream);
+	else
+		usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
+done_not_linked:
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
 done:

drivers/usb/host/isp116x-hcd.c

@@ -290,6 +290,7 @@ __releases(isp116x->lock) __acquires(isp116x->lock)
 
 	urb_dbg(urb, "Finish");
 
+	usb_hcd_unlink_urb_from_ep(isp116x_to_hcd(isp116x), urb);
 	spin_unlock(&isp116x->lock);
 	usb_hcd_giveback_urb(isp116x_to_hcd(isp116x), urb);
 	spin_lock(&isp116x->lock);
@@ -673,7 +674,7 @@ static int balance(struct isp116x *isp116x, u16 period, u16 load)
 /*-----------------------------------------------------------------*/
 
 static int isp116x_urb_enqueue(struct usb_hcd *hcd,
-			       struct usb_host_endpoint *hep, struct urb *urb,
+			       struct urb *urb,
 			       gfp_t mem_flags)
 {
 	struct isp116x *isp116x = hcd_to_isp116x(hcd);
@@ -682,6 +683,7 @@ static int isp116x_urb_enqueue(struct usb_hcd *hcd,
 	int is_out = !usb_pipein(pipe);
 	int type = usb_pipetype(pipe);
 	int epnum = usb_pipeendpoint(pipe);
+	struct usb_host_endpoint *hep = urb->ep;
 	struct isp116x_ep *ep = NULL;
 	unsigned long flags;
 	int i;
@@ -705,7 +707,12 @@ static int isp116x_urb_enqueue(struct usb_hcd *hcd,
 	if (!HC_IS_RUNNING(hcd->state)) {
 		kfree(ep);
 		ret = -ENODEV;
-		goto fail;
+		goto fail_not_linked;
+	}
+	ret = usb_hcd_link_urb_to_ep(hcd, urb);
+	if (ret) {
+		kfree(ep);
+		goto fail_not_linked;
 	}
 
 	if (hep->hcpriv)
@@ -818,27 +825,31 @@ static int isp116x_urb_enqueue(struct usb_hcd *hcd,
 	start_atl_transfers(isp116x);
 
       fail:
+	if (ret)
+		usb_hcd_unlink_urb_from_ep(hcd, urb);
+      fail_not_linked:
 	spin_unlock_irqrestore(&isp116x->lock, flags);
 	return ret;
 }
 
 /*
    Dequeue URBs.
 */
-static int isp116x_urb_dequeue(struct usb_hcd *hcd, struct urb *urb)
+static int isp116x_urb_dequeue(struct usb_hcd *hcd, struct urb *urb,
+		int status)
 {
 	struct isp116x *isp116x = hcd_to_isp116x(hcd);
 	struct usb_host_endpoint *hep;
 	struct isp116x_ep *ep, *ep_act;
 	unsigned long flags;
+	int rc;
 
 	spin_lock_irqsave(&isp116x->lock, flags);
+	rc = usb_hcd_check_unlink_urb(hcd, urb, status);
+	if (rc)
+		goto done;
+
 	hep = urb->hcpriv;
-	/* URB already unlinked (or never linked)? */
-	if (!hep) {
-		spin_unlock_irqrestore(&isp116x->lock, flags);
-		return 0;
-	}
 	ep = hep->hcpriv;
 	WARN_ON(hep != ep->hep);
 
@@ -856,9 +867,9 @@ static int isp116x_urb_dequeue(struct usb_hcd *hcd, struct urb *urb)
 
 	if (urb)
 		finish_request(isp116x, ep, urb);
-
+ done:
 	spin_unlock_irqrestore(&isp116x->lock, flags);
-	return 0;
+	return rc;
 }
 
 static void isp116x_endpoint_disable(struct usb_hcd *hcd,