ChangeSet 1.1002.3.4, 2003/02/20 10:18:03-08:00, david-b@pacbell.net
[PATCH] USB: usbtest checks for in-order completions
This makes "test 10" verify that completions are returned in-order,
resolving a FIXME. OHCI and EHCI do, but UHCI doesn't. (*) That
simplified a cleanup of the queue fault tests to make it easier
to handle optional faults (with one or more failure modes).
It also returns a "lost subcase" that accidentally was not getting
run. And in a case of pure paranoia, the unlink tests handle the
EBUSY return from an async urb unlink ... if that ever shows up I'd
expect it to be on an SMP box.
(*) I'd suspected as much given the first round of tests with UHCI;
the diagnostics from "usbtest" made no sense otherwise. This
is just repeatable confirmation of the earlier bug report. This
could cause trouble in the usb_sg_*() I/O calls.
In this case, "testusb -at10 -g4" reported that subcase 1 completed
out of order (before subcase 0) ... without looking at details, I'd
guess a list_add() vs list_add_tail() issue.
Then after trying the queue cleanup code, I got diagnostics from
uhci_destroy_urb_priv; then a kmalloc poisoning oops in uhci_irq,
or "uhci_remove_pending_qhs+0x7c/0x1b0" in more detail. Those
looks to be the same "can't unlink from completions" errors that
was also reported before in that code.
Note that "testusb -at10" (like "testusb -at9") can be made to
work with any USB device, using "usbtest" module options.
drivers/usb/misc/usbtest.c | 134 +++++++++++++++++++++++++++------------------
1 files changed, 82 insertions(+), 52 deletions(-)
diff -Nru a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
--- a/drivers/usb/misc/usbtest.c Fri Feb 28 14:51:49 2003
+++ b/drivers/usb/misc/usbtest.c Fri Feb 28 14:51:49 2003
@@ -552,6 +552,7 @@
* (b) protocol stalls (control-only) will autorecover.
* it's quite not like bulk/intr; no halt clearing.
* (c) short control reads are reported and handled.
+ * (d) queues are always processed in-order
*/
struct ctrl_ctx {
@@ -563,66 +564,68 @@
int status;
struct urb **urb;
struct usbtest_param *param;
+ int last;
+};
+
+#define NUM_SUBCASES 13 /* how many test subcases here? */
+
+struct subcase {
+ struct usb_ctrlrequest setup;
+ int number;
+ int expected;
};
static void ctrl_complete (struct urb *urb, struct pt_regs *regs)
{
struct ctrl_ctx *ctx = urb->context;
struct usb_ctrlrequest *reqp;
+ struct subcase *subcase;
int status = urb->status;
reqp = (struct usb_ctrlrequest *)urb->setup_packet;
+ subcase = container_of (reqp, struct subcase, setup);
spin_lock (&ctx->lock);
ctx->count--;
ctx->pending--;
- /* FIXME verify that the completions are in the right sequence.
- * we could store the test number with the setup packet, that
- * buffer has extra space.
+ /* queue must transfer and complete in fifo order, unless
+ * usb_unlink_urb() is used to unlink something not at the
+ * physical queue head (not tested).
*/
-
- switch (status) {
- case 0: /* success */
- case -EREMOTEIO: /* short read */
- if (reqp->bRequestType == (USB_DIR_IN|USB_RECIP_DEVICE)
- && reqp->bRequest == USB_REQ_GET_DESCRIPTOR
- && ((le16_to_cpu (reqp->wValue) >> 8)
- == USB_DT_DEVICE)) {
- if (reqp->wLength > USB_DT_DEVICE_SIZE
- && status == -EREMOTEIO)
- status = 0;
- else if (reqp->wLength == USB_DT_DEVICE_SIZE
- && status != 0)
- status = -EIO;
- if (status)
- goto error;
- }
- break;
- case -ECONNRESET: /* async unlink */
- break;
- case -EPIPE: /* (protocol) stall */
- if (reqp->bRequestType == (USB_DIR_IN|USB_RECIP_INTERFACE)
- && reqp->bRequest == USB_REQ_GET_INTERFACE)
+ if (subcase->number > 0) {
+ if ((subcase->number - ctx->last) != 1) {
+ dbg ("subcase %d completed out of order, last %d",
+ subcase->number, ctx->last);
+ status = -EDOM;
+ goto error;
+ }
+ }
+ ctx->last = subcase->number;
+
+ /* succeed or fault in only one way? */
+ if (status == subcase->expected)
+ status = 0;
+
+ /* async unlink for cleanup? */
+ else if (status != -ECONNRESET) {
+
+ /* some faults are allowed, not required */
+ if (subcase->expected > 0 && (
+ ((urb->status == -subcase->expected /* happened */
+ || urb->status == 0)))) /* didn't */
status = 0;
- else if (reqp->bRequestType == (USB_DIR_IN|USB_RECIP_DEVICE)
- && reqp->bRequest == USB_REQ_GET_DESCRIPTOR) {
- switch (le16_to_cpu (reqp->wValue) >> 8) {
- case USB_DT_DEVICE_QUALIFIER:
- case USB_DT_OTHER_SPEED_CONFIG:
- case USB_DT_INTERFACE:
- case USB_DT_ENDPOINT:
- status = 0;
- }
- } else if (reqp->bRequestType == USB_RECIP_ENDPOINT
- && reqp->bRequest == USB_REQ_CLEAR_FEATURE)
+ /* sometimes more than one fault is allowed */
+ else if (subcase->number == 12 && status == -EPIPE)
status = 0;
- /* some stalls we plan on; others would be errors */
- if (status == 0)
- break;
- /* else FALLTHROUGH */
+ else
+ dbg ("subtest %d error, status %d",
+ subcase->number, status);
+ }
+
+ /* unexpected status codes mean errors; ideally, in hardware */
+ if (status) {
error:
- default: /* this fault's an error */
if (ctx->status == 0) {
int i;
@@ -631,10 +634,8 @@
reqp->bRequestType, reqp->bRequest,
status, ctx->count);
- /* FIXME use this "unlink everything" exit route
- * in all cases, not just for fault cleanup.
- * it'll be another test mode, but one that makes
- * testing be more consistent.
+ /* FIXME this "unlink everything" exit route should
+ * be a separate test case.
*/
/* unlink whatever's still pending */
@@ -688,6 +689,7 @@
context.pending = 0;
context.status = -ENOMEM;
context.param = param;
+ context.last = -1;
/* allocate and init the urbs we'll queue.
* as with bulk/intr sglists, sglen is the queue depth; it also
@@ -701,7 +703,9 @@
int pipe = usb_rcvctrlpipe (udev, 0);
unsigned len;
struct urb *u;
- struct usb_ctrlrequest req, *reqp;
+ struct usb_ctrlrequest req;
+ struct subcase *reqp;
+ int expected = 0;
/* requests here are mostly expected to succeed on any
* device, but some are chosen to trigger protocol stalls
@@ -711,7 +715,7 @@
req.bRequest = USB_REQ_GET_DESCRIPTOR;
req.bRequestType = USB_DIR_IN|USB_RECIP_DEVICE;
- switch (i % 12 /* number of subtest cases here */) {
+ switch (i % NUM_SUBCASES) {
case 0: // get device descriptor
req.wValue = cpu_to_le16 (USB_DT_DEVICE << 8);
len = sizeof (struct usb_device_descriptor);
@@ -725,6 +729,7 @@
req.bRequestType = USB_DIR_IN|USB_RECIP_INTERFACE;
// index = 0 means first interface
len = 1;
+ expected = EPIPE;
break;
case 3: // get interface status
req.bRequest = USB_REQ_GET_STATUS;
@@ -740,6 +745,8 @@
case 5: // get device qualifier (MAY STALL)
req.wValue = cpu_to_le16 (USB_DT_DEVICE_QUALIFIER << 8);
len = sizeof (struct usb_qualifier_descriptor);
+ if (udev->speed != USB_SPEED_HIGH)
+ expected = EPIPE;
break;
case 6: // get first config descriptor, plus interface
req.wValue = cpu_to_le16 ((USB_DT_CONFIG << 8) | 0);
@@ -750,6 +757,7 @@
req.wValue = cpu_to_le16 (USB_DT_INTERFACE << 8);
// interface == 0
len = sizeof (struct usb_interface_descriptor);
+ expected = -EPIPE;
break;
// NOTE: two consecutive stalls in the queue here.
// that tests fault recovery a bit more aggressively.
@@ -760,6 +768,7 @@
// wIndex 0 == ep0 (shouldn't halt!)
len = 0;
pipe = usb_sndctrlpipe (udev, 0);
+ expected = EPIPE;
break;
case 9: // get endpoint status
req.bRequest = USB_REQ_GET_STATUS;
@@ -770,18 +779,21 @@
case 10: // trigger short read (EREMOTEIO)
req.wValue = cpu_to_le16 ((USB_DT_CONFIG << 8) | 0);
len = 1024;
+ expected = -EREMOTEIO;
break;
// NOTE: two consecutive _different_ faults in the queue.
case 11: // get endpoint descriptor (ALWAYS STALLS)
req.wValue = cpu_to_le16 (USB_DT_ENDPOINT << 8);
// endpoint == 0
len = sizeof (struct usb_interface_descriptor);
+ expected = -EPIPE;
break;
// NOTE: sometimes even a third fault in the queue!
case 12: // get string 0 descriptor (MAY STALL)
req.wValue = cpu_to_le16 (USB_DT_STRING << 8);
// string == 0, for language IDs
len = sizeof (struct usb_interface_descriptor);
+ expected = EREMOTEIO; // or EPIPE, if no strings
break;
default:
err ("bogus number of ctrl queue testcases!");
@@ -793,12 +805,14 @@
if (!u)
goto cleanup;
- reqp = usb_buffer_alloc (udev, sizeof req, SLAB_KERNEL,
+ reqp = usb_buffer_alloc (udev, sizeof *reqp, SLAB_KERNEL,
&u->setup_dma);
if (!reqp)
goto cleanup;
- *reqp = req;
- u->setup_packet = (char *) reqp;
+ reqp->setup = req;
+ reqp->number = i % NUM_SUBCASES;
+ reqp->expected = expected;
+ u->setup_packet = (char *) &reqp->setup;
u->context = &context;
u->complete = ctrl_complete;
@@ -839,6 +853,7 @@
kfree (urb);
return context.status;
}
+#undef NUM_SUBCASES
/*-------------------------------------------------------------------------*/
@@ -886,7 +901,16 @@
* hcd states and code paths, even with little other system load.
*/
wait_ms (jiffies % (2 * INTERRUPT_RATE));
+retry:
retval = usb_unlink_urb (urb);
+ if (retval == -EBUSY) {
+ /* we can't unlink urbs while they're completing.
+ * "normal" drivers would prevent resubmission, but
+ * since we're testing unlink paths, we can't.
+ */
+ dbg ("unlink retry");
+ goto retry;
+ }
if (!(retval == 0 || retval == -EINPROGRESS)) {
dbg ("submit/unlink fail %d", retval);
return retval;
@@ -1309,7 +1333,13 @@
.alt = 1,
};
-/* ezusb family device with dedicated usb test firmware*/
+/* ezusb family device with dedicated usb test firmware,
+ * or a peripheral running Linux and 'zero.c' test firmware.
+ *
+ * FIXME usbtest should read the descriptors, since compatible
+ * test firmware might run on hardware (pxa250 for one) that
+ * can't configure an ep2in-bulk.
+ */
static struct usbtest_info fw_info = {
.name = "usb test device",
.ep_in = 2,