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
