On 10/03/17(Fri) 15:27, Martin Pieuchot wrote:
> 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.
Updated diff on top of -current, I'd appreciate test reports and reviews
:)
Index: ehci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.200
diff -u -p -r1.200 ehci.c
--- ehci.c 15 May 2017 10:52:08 -0000 1.200
+++ ehci.c 15 May 2017 11:06:19 -0000
@@ -3350,7 +3350,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.95
diff -u -p -r1.95 usbdi.c
--- usbdi.c 15 May 2017 10:52:08 -0000 1.95
+++ usbdi.c 18 May 2017 11:06:27 -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;
+
+ while (pipe->uxid == uxid) {
+ flags = PRIBIO | (flags & USBD_CATCH ? PCATCH : 0);
- err = tsleep(xfer, flags, "usbsyn", 0);
- if (err && !xfer->done) {
- usbd_abort_pipe(pipe);
- if (err == EINTR)
- xfer->status = USBD_INTERRUPTED;
+ 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,13 +741,6 @@ usb_transfer_complete(struct usbd_xfer *
printf("%s: xfer=%p not on queue\n", __func__, 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);
@@ -741,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,
@@ -778,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",
@@ -804,7 +814,7 @@ usb_transfer_complete(struct usbd_xfer *
}
if ((flags & USBD_SYNCHRONOUS) && !polling)
- wakeup(xfer);
+ wakeup(pipe);
if (!pipe->repeat) {
/* XXX should we stop the queue on all errors? */
@@ -838,6 +848,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;
}
@@ -845,7 +856,6 @@ usb_insert_transfer(struct usbd_xfer *xf
return (err);
}
-/* Called at splusb() */
void
usbd_start_next(struct usbd_pipe *pipe)
{
@@ -854,23 +864,13 @@ usbd_start_next(struct usbd_pipe *pipe)
splsoftassert(IPL_SOFTUSB);
-#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.72
diff -u -p -r1.72 usbdivar.h
--- usbdivar.h 8 Apr 2017 02:57:25 -0000 1.72
+++ usbdivar.h 15 May 2017 11:06:19 -0000
@@ -177,6 +177,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;
@@ -195,7 +196,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
