In polling mode, finished transfers are processed by the waiting thread.
This happens inside usbd_dopoll().  That means it's unsafe to dereference 
``xfer'' after calling it:

352:            usbd_dopoll(pipe->device);
353:            if (xfer->done)
354:                    break;


When interrupts are enabled and a transfer times out it is unsafe to
dereference ``xfer'' after being awaken.  That's because the aborting
task calls wakeup(9) after executing the callback:


807:                    pipe->methods->done(xfer);
808:                    if (xfer->callback)
809:                            xfer->callback(xfer, xfer->priv, xfer->status);

                ...

821:            if ((flags & USBD_SYNCHRONOUS) && !polling)
822:                     wakeup(xfer);


These problems are inherent to the fact that the ``done'' flag is on
the ``xfer'' descriptor.

The diff below fixes the problem by using a unique ID to check if a
submitted transfer is finished or not.  Since we know that the ``pipe''
associated to a transfer wont disappear until it is closed, we can use
it to store the current ID.
When can also tsleep(9) on the address of the pipe instead of the address
of the transfer.

Note that with this change usbd_transfer() will no longer report all
possible USB error messages.  But I'd say it's an improvement since
not a single driver can handle them.

ok?

Index: ehci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.197
diff -u -p -r1.197 ehci.c
--- ehci.c      10 Mar 2017 11:18:48 -0000      1.197
+++ ehci.c      10 Mar 2017 12:00:57 -0000
@@ -3348,7 +3348,6 @@ ehci_device_isoc_start(struct usbd_xfer 
 
        TAILQ_INSERT_TAIL(&sc->sc_intrhead, ex, inext);
        xfer->status = USBD_IN_PROGRESS;
-       xfer->done = 0;
        splx(s);
 
        return (USBD_IN_PROGRESS);
Index: usbdi.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.89
diff -u -p -r1.89 usbdi.c
--- usbdi.c     10 Mar 2017 11:18:48 -0000      1.89
+++ usbdi.c     10 Mar 2017 12:00:58 -0000
@@ -54,6 +54,8 @@ extern int usbdebug;
 #define DPRINTFN(n,x)
 #endif
 
+uint64_t usbd_xfer_id;         /* unique ID per xfer */
+
 void usbd_request_async_cb(struct usbd_xfer *, void *, usbd_status);
 void usbd_start_next(struct usbd_pipe *pipe);
 usbd_status usbd_open_pipe_ival(struct usbd_interface *, u_int8_t, u_int8_t,
@@ -282,6 +284,8 @@ usbd_transfer(struct usbd_xfer *xfer)
        struct usbd_bus *bus = pipe->device->bus;
        int polling = bus->use_polling;
        usbd_status err;
+       uint32_t timeout;
+       uint64_t uxid;
        int flags, s;
 
        if (usbd_is_dying(pipe->device))
@@ -293,8 +297,6 @@ usbd_transfer(struct usbd_xfer *xfer)
        if (usbdebug > 5)
                usbd_dump_queue(pipe);
 #endif
-       xfer->done = 0;
-
        if (pipe->aborting)
                return (USBD_CANCELLED);
 
@@ -320,8 +322,12 @@ usbd_transfer(struct usbd_xfer *xfer)
                usb_syncmem(&xfer->dmabuf, 0, xfer->length,
                    BUS_DMASYNC_PREREAD);
 
-       err = pipe->methods->transfer(xfer);
+       /* We cannot dereference ``xfer'' after giving it to the HC. */
+       uxid = xfer->id;
+       flags = xfer->flags;
+       timeout = xfer->timeout;
 
+       err = pipe->methods->transfer(xfer);
        if (err != USBD_IN_PROGRESS && err != USBD_NORMAL_COMPLETION) {
                /* The transfer has not been queued, so free buffer. */
                if (xfer->rqflags & URQ_AUTO_DMABUF) {
@@ -330,50 +336,52 @@ usbd_transfer(struct usbd_xfer *xfer)
                }
        }
 
-       if (!(xfer->flags & USBD_SYNCHRONOUS))
+       if (!(flags & USBD_SYNCHRONOUS) || (err != USBD_IN_PROGRESS))
                return (err);
 
        /* Sync transfer, wait for completion. */
-       if (err != USBD_IN_PROGRESS)
-               return (err);
-
        s = splusb();
        if (polling) {
                int timo;
 
-               for (timo = xfer->timeout; timo >= 0; timo--) {
+               for (timo = timeout; timo >= 0; timo--) {
                        usb_delay_ms(bus, 1);
                        if (bus->dying) {
-                               xfer->status = USBD_IOERROR;
-                               usb_transfer_complete(xfer);
+                               err = USBD_IOERROR;
                                break;
                        }
 
                        usbd_dopoll(pipe->device);
-                       if (xfer->done)
+                       if (pipe->uxid != uxid) {
+                               err = USBD_NORMAL_COMPLETION;
                                break;
+                       }
                }
 
-               if (timo < 0) {
-                       xfer->status = USBD_TIMEOUT;
-                       usb_transfer_complete(xfer);
-               }
+               if (timo < 0)
+                       err = USBD_TIMEOUT;
        } else {
-               while (!xfer->done) {
-                       flags = PRIBIO|(xfer->flags & USBD_CATCH ? PCATCH : 0);
+               int error;
 
-                       err = tsleep(xfer, flags, "usbsyn", 0);
-                       if (err && !xfer->done) {
-                               usbd_abort_pipe(pipe);
-                               if (err == EINTR)
-                                       xfer->status = USBD_INTERRUPTED;
+               while (pipe->uxid == uxid) {
+                       flags = PRIBIO | (flags & USBD_CATCH ? PCATCH : 0);
+
+                       error = tsleep(pipe, flags, "usbsyn", 0);
+                       if (error && (pipe->uxid == uxid)) {
+                               if (error == EINTR)
+                                       err = USBD_INTERRUPTED;
                                else
-                                       xfer->status = USBD_TIMEOUT;
+                                       err = USBD_TIMEOUT;
+                       } else {
+                               err = USBD_NORMAL_COMPLETION;
                        }
                }
        }
+       if (err)
+               usbd_abort_pipe(pipe);
        splx(s);
-       return (xfer->status);
+
+       return (err);
 }
 
 void *
@@ -414,6 +422,12 @@ usbd_alloc_xfer(struct usbd_device *dev)
        xfer = dev->bus->methods->allocx(dev->bus);
        if (xfer == NULL)
                return (NULL);
+
+       usbd_xfer_id++;
+       if (usbd_xfer_id == 0)
+               usbd_xfer_id++;
+
+       xfer->id = usbd_xfer_id;
 #ifdef DIAGNOSTIC
        xfer->busy_free = XFER_FREE;
 #endif
@@ -727,19 +741,6 @@ usb_transfer_complete(struct usbd_xfer *
                printf("%s: xfer=%p not on queue\n", __func__, xfer);
                return;
        }
-#endif
-
-#ifdef DIAGNOSTIC
-       if (pipe == NULL) {
-               printf("usb_transfer_complete: pipe==0, xfer=%p\n", xfer);
-               return;
-       }
-#endif
-       /* XXXX */
-       if (polling)
-               pipe->running = 0;
-
-#ifdef DIAGNOSTIC
        if (xfer->actlen > xfer->length) {
                printf("%s: actlen > len %u > %u\n", __func__, xfer->actlen,
                    xfer->length);
@@ -747,6 +748,9 @@ usb_transfer_complete(struct usbd_xfer *
        }
 #endif
 
+       if (polling)
+               pipe->running = 0;
+
        if (xfer->actlen != 0) {
                if (usbd_xfer_isread(xfer)) {
                        usb_syncmem(&xfer->dmabuf, 0, xfer->actlen,
@@ -784,7 +788,7 @@ usb_transfer_complete(struct usbd_xfer *
        ++pipe->device->bus->stats.uds_requests
                [pipe->endpoint->edesc->bmAttributes & UE_XFERTYPE];
 
-       xfer->done = 1;
+       pipe->uxid = 0;
        if (!xfer->status && xfer->actlen < xfer->length &&
            !(xfer->flags & USBD_SHORT_XFER_OK)) {
                DPRINTFN(-1,("usb_transfer_complete: short transfer %d<%d\n",
@@ -819,7 +823,7 @@ usb_transfer_complete(struct usbd_xfer *
                pipe->repeat = 0;
 
        if ((flags & USBD_SYNCHRONOUS) && !polling)
-               wakeup(xfer);
+               wakeup(pipe);
 
        if (!pipe->repeat) {
                /* XXX should we stop the queue on all errors? */
@@ -853,6 +857,7 @@ usb_insert_transfer(struct usbd_xfer *xf
        if (pipe->running)
                err = USBD_IN_PROGRESS;
        else {
+               pipe->uxid = xfer->id;
                pipe->running = 1;
                err = USBD_NORMAL_COMPLETION;
        }
@@ -860,7 +865,6 @@ usb_insert_transfer(struct usbd_xfer *xf
        return (err);
 }
 
-/* Called at splusb() */
 void
 usbd_start_next(struct usbd_pipe *pipe)
 {
@@ -869,23 +873,13 @@ usbd_start_next(struct usbd_pipe *pipe)
 
        SPLUSBCHECK;
 
-#ifdef DIAGNOSTIC
-       if (pipe == NULL) {
-               printf("usbd_start_next: pipe == NULL\n");
-               return;
-       }
-       if (pipe->methods == NULL || pipe->methods->start == NULL) {
-               printf("usbd_start_next: pipe=%p no start method\n", pipe);
-               return;
-       }
-#endif
-
        /* Get next request in queue. */
        xfer = SIMPLEQ_FIRST(&pipe->queue);
        DPRINTFN(5, ("usbd_start_next: pipe=%p, xfer=%p\n", pipe, xfer));
        if (xfer == NULL) {
                pipe->running = 0;
        } else {
+               pipe->uxid = xfer->id;
                err = pipe->methods->start(xfer);
                if (err != USBD_IN_PROGRESS) {
                        printf("usbd_start_next: error=%d\n", err);
Index: usbdivar.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/usbdivar.h,v
retrieving revision 1.71
diff -u -p -r1.71 usbdivar.h
--- usbdivar.h  23 May 2016 11:31:12 -0000      1.71
+++ usbdivar.h  10 Mar 2017 12:00:59 -0000
@@ -176,6 +176,7 @@ struct usbd_pipe {
        SIMPLEQ_HEAD(, usbd_xfer) queue;
        LIST_ENTRY(usbd_pipe)   next;
 
+       uint64_t                uxid; /* xfer being currently transfered */
        struct usbd_xfer        *intrxfer; /* used for repeating requests */
        char                    repeat;
        int                     interval;
@@ -194,7 +195,7 @@ struct usbd_xfer {
        u_int32_t               timeout;
        usbd_status             status;
        usbd_callback           callback;
-       volatile char           done;
+       u_int64_t               id;
 #ifdef DIAGNOSTIC
        u_int32_t               busy_free;
 #define XFER_FREE 0x42555359

Reply via email to