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 - 1.200
+++ ehci.c 15 May 2017 11:06:19 -
@@ -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 - 1.95
+++ usbdi.c 18 May 2017 11:06:27 -
@@ -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) {
-