I thought I'd send this around now that Dan and Johannes have updated
UHCI to handle queueing for control and interrupt urbs. (Yay!) The patch:
- Rips the automagic resubmit code out of the main three HCDs.
The UHCI bits should resemble some code from Dan, and the patches
to EHCI and OHCI should be what you've seen before.
- Updates a few key drivers -- like HID and HUB! -- to stop relying
on automagic resubmit, and a couple others. Over half the drivers
using interrupt transfers are missing resubmit-in-completion logic.
- Updates "usbtest" so it should be usable to test interrupt transfers
now, using a module parameter. (Use it with lower test counts, they
will be almost 20 slower than the same tests using bulk transfers.)
- Corresponding kerneldoc updates.
It got more testing the first time I posted it, when it never gave any
problems, so I'm trusting the guts of this. Even though part way
through testing it, I tripped over a networking BUG() ... I hope you
don't have to watch your khubd die young that way! :)
Not ready for integration yet, IMO, but worth testing particularly if
you have a driver that uses interrupt transfers. I'll be interested
in feedback and patches for the other drivers.
- Dave
--- ./include/linux/usb.h-dist Fri Oct 18 21:42:45 2002
+++ ./include/linux/usb.h Sun Oct 20 20:06:27 2002
@@ -792,6 +792,5 @@
* request-specific driver context.
* @complete: Completion handler. This URB is passed as the parameter to the
- * completion function. Except for interrupt or isochronous transfers
- * that aren't being unlinked, the completion function may then do what
+ * completion function. The completion function may then do what
* it likes with the URB, including resubmitting or freeing it.
* @iso_frame_desc: Used to provide arrays of ISO transfer buffers and to
@@ -886,9 +885,4 @@
* actual_length field tells how many bytes were transferred.
*
- * For interrupt URBs, the URB provided to the callback
- * function is still "owned" by the USB core subsystem unless the status
- * indicates that the URB has been unlinked. Completion handlers should
- * not modify such URBs until they have been unlinked.
- *
* ISO transfer status is reported in the status and actual_length fields
* of the iso_frame_desc array, and the number of errors is reported in
--- ./drivers/usb-dist/core/hcd.c Fri Oct 18 21:45:01 2002
+++ ./drivers/usb/core/hcd.c Sun Oct 20 20:06:27 2002
@@ -496,23 +496,15 @@
urb->actual_length = length;
urb->status = 0;
+ urb->hcpriv = 0;
urb->complete (urb);
+ return;
}
- spin_lock_irqsave (&hcd_data_lock, flags);
- urb->status = -EINPROGRESS;
- if (HCD_IS_RUNNING (hcd->state)
- && rh_status_urb (hcd, urb) != 0) {
- /* another driver snuck in? */
- dbg ("%s, can't resubmit roothub status urb?",
- hcd->self.bus_name);
- spin_unlock_irqrestore (&hcd_data_lock, flags);
- BUG ();
- }
- spin_unlock_irqrestore (&hcd_data_lock, flags);
- } else {
+ } else
spin_unlock_irqrestore (&urb->lock, flags);
- spin_lock_irqsave (&hcd_data_lock, flags);
- rh_status_urb (hcd, urb);
- spin_unlock_irqrestore (&hcd_data_lock, flags);
- }
+
+ /* retrigger timer until completion: success or unlink */
+ spin_lock_irqsave (&hcd_data_lock, flags);
+ rh_status_urb (hcd, urb);
+ spin_unlock_irqrestore (&hcd_data_lock, flags);
} else {
/* this urb's been unlinked */
@@ -1156,5 +1148,8 @@
}
- /* maybe set up to block on completion notification */
+ /* maybe set up to block until the urb's completion fires. the
+ * lower level hcd code is always async, locking on urb->status
+ * updates; an intercepted completion unblocks us.
+ */
if ((urb->transfer_flags & USB_TIMEOUT_KILLED))
urb->status = -ETIMEDOUT;
@@ -1291,9 +1286,4 @@
* the device driver won't cause problems if it frees, modifies,
* or resubmits this URB.
- * Bandwidth and other resources will be deallocated.
- *
- * HCDs must not use this for periodic URBs that are still scheduled
- * and will be reissued. They should just call their completion handlers
- * until the urb is returned to the device driver by unlinking.
*/
void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb)
--- ./drivers/usb-dist/core/hub.c Fri Oct 18 21:45:01 2002
+++ ./drivers/usb/core/hub.c Sun Oct 20 20:06:27 2002
@@ -119,5 +119,7 @@
{
struct usb_hub *hub = (struct usb_hub *)urb->context;
+ struct usb_device *dev = interface_to_usbdev(hub->intf);
unsigned long flags;
+ int status;
switch (urb->status) {
@@ -130,7 +132,7 @@
/* Cause a hub reset after 10 consecutive errors */
dbg("hub '%s' status %d for interrupt transfer",
- urb->dev->devpath, urb->status);
+ dev->devpath, urb->status);
if ((++hub->nerrors < 10) || hub->error)
- return;
+ goto resubmit;
hub->error = urb->status;
/* FALL THROUGH */
@@ -150,4 +152,11 @@
}
spin_unlock_irqrestore(&hub_event_lock, flags);
+
+resubmit:
+ urb->dev = dev;
+ if ((status = usb_submit_urb (hub->urb, GFP_ATOMIC)) != 0)
+ err ("hub '%s-%s' status %d for interrupt resubmit",
+ dev->bus->bus_name, dev->devpath,
+ status);
}
--- ./drivers/usb-dist/core/urb.c Fri Oct 18 21:45:01 2002
+++ ./drivers/usb/core/urb.c Sun Oct 20 20:31:54 2002
@@ -109,43 +109,45 @@
* Successful submissions return 0; otherwise this routine returns a
* negative error number. If the submission is successful, the complete()
- * fuction of the urb will be called when the USB host driver is
- * finished with the urb (either a successful transmission, or some
- * error case.)
- *
- * Unreserved Bandwidth Transfers:
- *
- * Bulk or control requests complete only once. When the completion
+ * callback from the urb will be called exactly once, when the USB core and
+ * host controller driver are finished with the urb. When the completion
* function is called, control of the URB is returned to the device
* driver which issued the request. The completion handler may then
* immediately free or reuse that URB.
*
- * Bulk URBs may be queued by submitting an URB to an endpoint before
- * previous ones complete. This can maximize bandwidth utilization by
- * letting the USB controller start work on the next URB without any
- * delay to report completion (scheduling and processing an interrupt)
- * and then submit that next request.
- *
* For control endpoints, the synchronous usb_control_msg() call is
* often used (in non-interrupt context) instead of this call.
+ * That is often used through convenience wrappers, for the requests
+ * that are standardized in the USB 2.0 specification. For bulk
+ * endpoints, a synchronous usb_bulk_msg() call is available.
+ *
+ * Request Queuing:
+ *
+ * URBs may be submitted to endpoints before previous ones complete, to
+ * minimize the impact of interrupt latencies and system overhead on data
+ * throughput. This is required for continuous isochronous data streams,
+ * and may also be required for some kinds of interrupt transfers. Such
+ * queueing also maximizes bandwidth utilization by letting USB controllers
+ * start work on later requests before driver software has finished the
+ * completion processing for earlier requests.
+ *
+ * Bulk and Isochronous URBs may always be queued. At this writing, all
+ * mainstream host controller drivers support queueing for control and
+ * interrupt transfer requests.
*
* Reserved Bandwidth Transfers:
*
- * Periodic URBs (interrupt or isochronous) are performed repeatedly.
- *
- * For interrupt requests this is (currently) automagically done
- * until the original request is aborted. When the completion callback
- * indicates the URB has been unlinked (with a special status code),
- * control of that URB returns to the device driver. Otherwise, the
- * completion handler does not control the URB, and should not change
- * any of its fields.
- *
- * For isochronous requests, the completion handler is expected to
- * submit an urb, typically resubmitting its parameter, until drivers
- * stop wanting data transfers. (For example, audio playback might have
- * finished, or a webcam turned off.)
- *
- * If the USB subsystem can't reserve sufficient bandwidth to perform
- * the periodic request, and bandwidth reservation is being done for
- * this controller, submitting such a periodic request will fail.
+ * Periodic transfers (interrupt or isochronous) are performed repeatedly,
+ * using the interval specified in the urb. Submitting the first urb to
+ * the endpoint reserves the bandwidth necessary to make those transfers.
+ * If the USB subsystem can't allocate sufficient bandwidth to perform
+ * the periodic request, submitting such a periodic request should fail.
+ *
+ * Device drivers must explicitly request that repetition, by ensuring that
+ * some URB is always on the endpoint's queue (except possibly for short
+ * periods during completion callacks). When there is no longer an urb
+ * queued, the endpoint's bandwidth reservation is canceled. This means
+ * drivers can use their completion handlers to ensure they keep bandwidth
+ * they need, by reinitializing and resubmitting the just-completed urb
+ * until the driver longer needs that periodic bandwidth.
*
* Memory Flags:
--- ./drivers/usb-dist/core/message.c Fri Oct 18 21:45:01 2002
+++ ./drivers/usb/core/message.c Sun Oct 20 20:06:27 2002
@@ -288,10 +288,4 @@
* The request may be canceled with usb_sg_cancel(), either before or after
* usb_sg_wait() is called.
- *
- * NOTE:
- *
- * At this writing, don't use the interrupt transfer mode, since the old old
- * "automagic resubmit" mode hasn't yet been removed. It should be removed
- * by the time 2.5 finalizes.
*/
int usb_sg_init (
--- ./drivers/usb-dist/host/ehci-hcd.c Fri Oct 18 21:45:02 2002
+++ ./drivers/usb/host/ehci-hcd.c Sun Oct 20 20:06:43 2002
@@ -107,6 +107,4 @@
#endif
-#define INTR_AUTOMAGIC /* to be removed later in 2.5 */
-
/* magic numbers that can affect system performance */
#define EHCI_TUNE_CERR 3 /* 0-3 qtd retries; 0 == don't stop */
--- ./drivers/usb-dist/host/ehci-q.c Fri Oct 18 21:45:01 2002
+++ ./drivers/usb/host/ehci-q.c Sun Oct 20 20:06:43 2002
@@ -161,11 +161,4 @@
static void ehci_urb_done (struct ehci_hcd *ehci, struct urb *urb)
{
-#ifdef INTR_AUTOMAGIC
- struct urb *resubmit = 0;
- struct usb_device *dev = 0;
-
- static int ehci_urb_enqueue (struct usb_hcd *, struct urb *, int);
-#endif
-
if (likely (urb->hcpriv != 0)) {
struct ehci_qh *qh = (struct ehci_qh *) urb->hcpriv;
@@ -176,12 +169,4 @@
/* ... update hc-wide periodic stats (for usbfs) */
hcd_to_bus (&ehci->hcd)->bandwidth_int_reqs--;
-
-#ifdef INTR_AUTOMAGIC
- if (!((urb->status == -ENOENT)
- || (urb->status == -ECONNRESET))) {
- resubmit = usb_get_urb (urb);
- dev = urb->dev;
- }
-#endif
}
qh_put (ehci, qh);
@@ -208,23 +193,4 @@
usb_hcd_giveback_urb (&ehci->hcd, urb);
-#ifdef INTR_AUTOMAGIC
- if (resubmit && ((urb->status == -ENOENT)
- || (urb->status == -ECONNRESET))) {
- usb_put_urb (resubmit);
- resubmit = 0;
- }
- // device drivers will soon be doing something like this
- if (resubmit) {
- int status;
-
- resubmit->dev = dev;
- status = SUBMIT_URB (resubmit, SLAB_KERNEL);
- if (status != 0)
- err ("can't resubmit interrupt urb %p: status %d",
- resubmit, status);
- usb_put_urb (resubmit);
- }
-#endif
-
spin_lock (&ehci->lock);
}
--- ./drivers/usb-dist/host/ohci-q.c Fri Oct 18 21:45:02 2002
+++ ./drivers/usb/host/ohci-q.c Sun Oct 20 20:06:43 2002
@@ -63,52 +63,4 @@
}
-static void td_submit_urb (struct ohci_hcd *ohci, struct urb *urb);
-
-/* Report interrupt transfer completion, maybe reissue */
-static inline void intr_resub (struct ohci_hcd *hc, struct urb *urb)
-{
- struct urb_priv *urb_priv = urb->hcpriv;
- unsigned long flags;
-
-// FIXME going away along with the rest of interrrupt automagic...
-
- /* FIXME: MP race. If another CPU partially unlinks
- * this URB (urb->status was updated, hasn't yet told
- * us to dequeue) before we call complete() here, an
- * extra "unlinked" completion will be reported...
- */
-
- spin_lock_irqsave (&urb->lock, flags);
- if (likely (urb->status == -EINPROGRESS))
- urb->status = 0;
- spin_unlock_irqrestore (&urb->lock, flags);
-
- if (!(urb->transfer_flags & URB_NO_DMA_MAP)
- && usb_pipein (urb->pipe))
- pci_dma_sync_single (hc->hcd.pdev, urb->transfer_dma,
- urb->transfer_buffer_length,
- PCI_DMA_FROMDEVICE);
-
-#ifdef OHCI_VERBOSE_DEBUG
- urb_print (urb, "INTR", usb_pipeout (urb->pipe));
-#endif
- urb->complete (urb);
-
- /* always requeued, but ED_SKIP if complete() unlinks.
- * EDs are removed from periodic table only at SOF intr.
- */
- urb->actual_length = 0;
- spin_lock_irqsave (&urb->lock, flags);
- if (urb_priv->state != URB_DEL)
- urb->status = -EINPROGRESS;
- spin_unlock (&urb->lock);
-
- /* syncing with PCI_DMA_TODEVICE is evidently trouble... */
-
- spin_lock (&hc->lock);
- td_submit_urb (hc, urb);
- spin_unlock_irqrestore (&hc->lock, flags);
-}
-
/*-------------------------------------------------------------------------*
@@ -1023,20 +975,8 @@
urb_priv->td_cnt++;
- /* If all this urb's TDs are done, call complete().
- * Interrupt transfers are the only special case:
- * they're reissued, until "deleted" by usb_unlink_urb
- * (real work done in a SOF intr, by finish_unlinks).
- */
+ /* If all this urb's TDs are done, call complete() */
if (urb_priv->td_cnt == urb_priv->length) {
- int resubmit;
-
- resubmit = usb_pipeint (urb->pipe)
- && (urb_priv->state != URB_DEL);
-
spin_unlock_irqrestore (&ohci->lock, flags);
- if (resubmit)
- intr_resub (ohci, urb);
- else
- finish_urb (ohci, urb);
+ finish_urb (ohci, urb);
spin_lock_irqsave (&ohci->lock, flags);
}
--- ./drivers/usb-dist/host/uhci-hcd.c Fri Oct 18 21:45:02 2002
+++ ./drivers/usb/host/uhci-hcd.c Sun Oct 20 21:10:53 2002
@@ -1248,8 +1248,9 @@
static inline int uhci_submit_interrupt(struct uhci_hcd *uhci, struct urb *urb,
struct urb *eurb)
{
- /* Interrupt-IN can't be more than 1 packet */
- if (usb_pipein(urb->pipe) && urb->transfer_buffer_length >
usb_maxpacket(urb->dev, urb->pipe, usb_pipeout(urb->pipe)))
- return -EINVAL;
-
+ /* USB 1.1 interrupt transfers only involve one packet per interval;
+ * that's the uhci_submit_common() "breadth first" policy. Drivers
+ * can submit urbs of any length, but longer ones might need many
+ * intervals to complete.
+ */
return uhci_submit_common(uhci, urb, eurb,
uhci->skelqh[__interval_to_skel(urb->interval)]);
}
@@ -1805,42 +1806,17 @@
{
struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
- struct usb_device *dev = urb->dev;
struct uhci_hcd *uhci = hcd_to_uhci(hcd);
- int killed, resubmit_interrupt, status, ret;
+ int status;
unsigned long flags;
spin_lock_irqsave(&urb->lock, flags);
-
- killed = (urb->status == -ENOENT || urb->status == -ECONNRESET);
- resubmit_interrupt = (usb_pipetype(urb->pipe) == PIPE_INTERRUPT &&
- urb->interval && !killed);
-
status = urbp->status;
uhci_destroy_urb_priv(uhci, urb);
- if (!killed)
+ if (urb->status != -ENOENT && urb->status != -ECONNRESET)
urb->status = status;
spin_unlock_irqrestore(&urb->lock, flags);
- if (resubmit_interrupt) {
- urb->complete(urb);
-
- /* Recheck the status. The completion handler may have */
- /* unlinked the resubmitting interrupt URB */
- /* Note that this doesn't do what usb_hcd_giveback_urb() */
- /* normally does, so that doesn't ever get done. */
- if (urb->status == -ECONNRESET) {
- usb_put_urb(urb);
- return;
- }
-
- urb->dev = dev;
- urb->status = -EINPROGRESS;
- urb->actual_length = 0;
- urb->bandwidth = 0;
- if ((ret = uhci_urb_enqueue(&uhci->hcd, urb, 0)))
- printk(KERN_ERR __FILE__ ": could not resubmit interrupt URB :
%d\n", ret);
- } else
- usb_hcd_giveback_urb(hcd, urb);
+ usb_hcd_giveback_urb(hcd, urb);
}
--- ./drivers/usb-dist/input/hid-core.c Fri Oct 18 21:44:58 2002
+++ ./drivers/usb/input/hid-core.c Sun Oct 20 20:06:43 2002
@@ -901,10 +901,24 @@
static void hid_irq_in(struct urb *urb)
{
- if (urb->status) {
- dbg("nonzero status in input irq %d", urb->status);
+ struct hid_device *hid = urb->context;
+ int status;
+
+ switch (urb->status) {
+ case 0: /* success */
+ hid_input_report(HID_INPUT_REPORT, urb);
+ break;
+ case -ECONNRESET: /* unlink completed */
+ case -ENOENT:
return;
+ default: /* error */
+ dbg("nonzero status in input irq %d", urb->status);
}
-
- hid_input_report(HID_INPUT_REPORT, urb);
+
+ urb->dev = hid->dev;
+ status = usb_submit_urb (urb, SLAB_ATOMIC);
+ if (status)
+ err ("can't resubmit intr, %s-%s/input%d, status %d",
+ hid->dev->bus->bus_name, hid->dev->devpath,
+ hid->ifnum, status);
}
--- ./drivers/usb-dist/input/usbkbd.c Fri Oct 18 21:44:58 2002
+++ ./drivers/usb/input/usbkbd.c Sun Oct 20 20:06:43 2002
@@ -88,5 +88,13 @@
int i;
- if (urb->status) return;
+ switch (urb->status) {
+ case 0: /* success */
+ break;
+ case -ENOENT: /* unlink */
+ case -ECONNRESET:
+ return;
+ default: /* error */
+ goto resubmit;
+ }
for (i = 0; i < 8; i++)
@@ -113,4 +121,11 @@
memcpy(kbd->old, kbd->new, 8);
+
+resubmit:
+ urb->dev = kbd->usbdev;
+ i = usb_submit_urb (urb, SLAB_ATOMIC);
+ if (i)
+ err ("can't resubmit intr, %s-%s/input0, status %d",
+ kbd->usbdev->bus->bus_name, kbd->usbdev->devpath, i);
}
--- ./drivers/usb-dist/misc/usbtest.c Fri Oct 18 21:45:00 2002
+++ ./drivers/usb/misc/usbtest.c Sun Oct 20 20:06:43 2002
@@ -544,4 +544,8 @@
/*-------------------------------------------------------------------------*/
+// control queueing !!
+
+/*-------------------------------------------------------------------------*/
+
/* We only have this one interface to user space, through usbfs.
* User mode code can scan usbfs to find N different devices (maybe on
@@ -840,12 +844,4 @@
wtest = " intr-out";
}
-
-#if 1
- // FIXME disabling this until we finally get rid of
- // interrupt "automagic" resubmission
- dbg ("%s: no interrupt transfers for now", dev->id);
- kfree (dev);
- return -ENODEV;
-#endif
} else {
if (info->ep_in) {
--- ./drivers/usb-dist/net/pegasus.c Fri Oct 18 21:44:58 2002
+++ ./drivers/usb/net/pegasus.c Sun Oct 20 20:06:43 2002
@@ -671,4 +671,5 @@
struct net_device *net;
__u8 *d;
+ int status;
if (!pegasus)
@@ -701,4 +702,10 @@
}
}
+
+ urb->dev = pegasus->usb;
+ status = usb_submit_urb (urb, SLAB_ATOMIC);
+ if (status)
+ err ("%s: can't resubmit interrupt urb, %d",
+ net->name, status);
}
