Re: CVS commit: src/sys

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

> Module Name:    src
> Committed By:   mrg
> Date:           Thu Aug  9 06:26:47 UTC 2018
> 
> Modified Files:                                                         
>         src/sys/dev/usb: ehci.c ehcivar.h motg.c ohci.c ohcivar.h uhci.c
>             uhcivar.h usbdi.c usbdivar.h xhci.c xhcivar.h
>         src/sys/external/bsd/dwc2: dwc2.c
>             
> Log Message:
> pull across abort fixes from nick-nhusb.  add more abort fixes, using
> ideas from Taylor and Nick, and myself.  special thanks to both who
> inspired much of the code here, if not wrote it directly.

Unfortunately, there is a hole in this protocol which I didn't think
of until after you committed.

Consider the following sequence of events:

CPU 0                           CPU 1
-----                           -----
usbd_alloc_xfer
usbd_setup_xfer
usbd_transfer
callout_schedule
ehci_idone
                                ehci_timeout
mutex_enter in ehci_idone
                                (ehci_timeout loses race to acqure bus lock)
callout_stop (doesn't stop fired callout)
(xfer callback)
usbd_transfer
callout_schedule
                                mutex_enter in ehci_timeout
                                ehci_timeout_task
                                aborts task instantly
                                (xfer callback)
usbd_free_xfer
                                ehci_timeout
callout_stop (doesn't stop fired callout)
kmem_free
                                (stuff in ehci_timeout)

There are two separate bugs here.

1. The _second_ xfer (sharing the same struct usbd_xfer data
   structure but representing a subsequent transfer) is timed out
   instantly when it shouldn't be timed out until it requested.

2. The callout can run after usbd_free_xfer -- use-after-free.

The fix for the second bug is simple: use callout_halt (and
usb_rem_task_wait) in usbd_free_xfer, rather than the #if DIAGNOSTIC
callout_stop bogosity that it does right now.

However, the fix for the first bug requires more care.  As the timeout
mechanism works now, we need to ensure the callout and task have
_finished_ before we resubmit another xfer using the same usbd_xfer
object.  But there are two reasons why we can't do this:

(a) The hardware completion interrupt handler can't sleep, so it can't
    use callout_halt.  Now, maybe you could argue that callout_halt on
    a callout whose action only acquires locks is OK for a softintr.
    But it _certainly_ can't use usb_rem_task_wait which does a
    cv_wait on some hardware condition.

(b) There is nothing in the USBD API that happens _between_ xfer
    callback and when you can next usbd_setup_xfer and usbd_transfer.
    In fact, many drivers assume they can do exactly that straight
    from the xfer callback in softintr context, so we can't even add
    an API that does a wait.

Fortunately, there is no need to repeatedly wait for the callout and
task to complete.  We just need a way to _request_ that they
reschedule, if they are still running.

The attached patchset has five parts.  It is compile-tested only, so
it might not work, but it sketches the idea I have in mind.

1. Teach usb_rem_task to return whether it interceded successfully.
2. Add usb_task_pending for diagnostic assertions.
3. Call callout_halt and usb_rem_task_wait in usbd_free_xfer.
4. Add common state to struct usbd_xfer:
   - A flag saying whether the timeout is active or not.
   - A flag saying, if the timeout is active, whether it should just
     abort, or reschedule itself instead for a later time.
5. An example implementation of this in ehci.

Unfortunately, this patchset has a net + rather than net - like your
commit.  Fortunately, most of the logic I added to ehci should carry
over to other host controllers, and can probably be factored out into
usbdi.c to reduce the code expansion.  But we'll take these steps
slowly to make pullups feasible.

From bbfab8c436da16c10c09300e3c3a4c73529393f8 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 10 Aug 2018 05:51:54 +0000
Subject: [PATCH 1/5] Teach usb_rem_task to return whether removed from queue
 or not.

---
 sys/dev/usb/usb.c   | 10 +++++++---
 sys/dev/usb/usbdi.h |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/sys/dev/usb/usb.c b/sys/dev/usb/usb.c
index 0b848f0e34bb..38c3a1d123b6 100644
--- a/sys/dev/usb/usb.c
+++ b/sys/dev/usb/usb.c
@@ -430,14 +430,16 @@ usb_add_task(struct usbd_device *dev, struct usb_task *task, int queue)
 /*
  * usb_rem_task(dev, task)
  *
- *	If task is queued to run, remove it from the queue.
+ *	If task is queued to run, remove it from the queue.  Return
+ *	true if it successfully removed the task from the queue, false
+ *	if not.
  *
  *	Caller is _not_ guaranteed that the task is not running when
  *	this is done.
  *
  *	Never sleeps.
  */
-void
+bool
 usb_rem_task(struct usbd_device *dev, struct usb_task *task)
 {
 	unsigned queue;
@@ -451,10 +453,12 @@ usb_rem_task(struct usbd_device *dev, struct usb_task *task)
 			TAILQ_REMOVE(&taskq->tasks, task, next);
 			task->queue = USB_NUM_TASKQS;
 			mutex_exit(&taskq->lock);
-			break;
+			return true; /* removed from the queue */
 		}
 		mutex_exit(&taskq->lock);
 	}
+
+	return false;		/* was not removed from the queue */
 }
 
 /*
diff --git a/sys/dev/usb/usbdi.h b/sys/dev/usb/usbdi.h
index bef17f70f116..f1c84ac39802 100644
--- a/sys/dev/usb/usbdi.h
+++ b/sys/dev/usb/usbdi.h
@@ -220,7 +220,7 @@ struct usb_task {
 #define	USB_TASKQ_MPSAFE	0x80
 
 void usb_add_task(struct usbd_device *, struct usb_task *, int);
-void usb_rem_task(struct usbd_device *, struct usb_task *);
+bool usb_rem_task(struct usbd_device *, struct usb_task *);
 bool usb_rem_task_wait(struct usbd_device *, struct usb_task *, int,
     kmutex_t *);
 #define usb_init_task(t, f, a, fl) ((t)->fun = (f), (t)->arg = (a), (t)->queue = USB_NUM_TASKQS, (t)->flags = (fl))
-- 
2.11.0


From e53f9a4aace74e0420207321c7ef2d8c8b2d5736 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 10 Aug 2018 05:56:42 +0000
Subject: [PATCH 2/5] New function usb_task_pending for diagnostic assertions.

---
 sys/dev/usb/usb.c   | 15 +++++++++++++++
 sys/dev/usb/usbdi.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/sys/dev/usb/usb.c b/sys/dev/usb/usb.c
index 38c3a1d123b6..d08129552655 100644
--- a/sys/dev/usb/usb.c
+++ b/sys/dev/usb/usb.c
@@ -529,6 +529,21 @@ usb_rem_task_wait(struct usbd_device *dev, struct usb_task *task, int queue,
 	return removed;
 }
 
+/*
+ * usb_task_pending(dev, task)
+ *
+ *	True if task is queued, false if not.  Note that if task is
+ *	already running, it is not considered queued.
+ *
+ *	For diagnostic assertions only.
+ */
+bool
+usb_task_pending(struct usbd_device *dev, struct usb_task *task)
+{
+
+	return task->queue != USB_NUM_TASKQS;
+}
+
 void
 usb_event_thread(void *arg)
 {
diff --git a/sys/dev/usb/usbdi.h b/sys/dev/usb/usbdi.h
index f1c84ac39802..a84891566743 100644
--- a/sys/dev/usb/usbdi.h
+++ b/sys/dev/usb/usbdi.h
@@ -223,6 +223,7 @@ void usb_add_task(struct usbd_device *, struct usb_task *, int);
 bool usb_rem_task(struct usbd_device *, struct usb_task *);
 bool usb_rem_task_wait(struct usbd_device *, struct usb_task *, int,
     kmutex_t *);
+bool usb_task_pending(struct usbd_device *, struct usb_task *);
 #define usb_init_task(t, f, a, fl) ((t)->fun = (f), (t)->arg = (a), (t)->queue = USB_NUM_TASKQS, (t)->flags = (fl))
 
 struct usb_devno {
-- 
2.11.0


From 834e22b4956dc1f28ad6d40c3966ad7ba6e08d1f Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 10 Aug 2018 05:57:48 +0000
Subject: [PATCH 3/5] Nothing guarantees xfer's timeout has completed.

Wait for it when we free the xfer.
---
 sys/dev/usb/usbdi.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
index 2f24137b81f2..ec47aabef6c6 100644
--- a/sys/dev/usb/usbdi.c
+++ b/sys/dev/usb/usbdi.c
@@ -493,12 +493,14 @@ usbd_free_xfer(struct usbd_xfer *xfer)
 	if (xfer->ux_buf) {
 		usbd_free_buffer(xfer);
 	}
-#if defined(DIAGNOSTIC)
-	if (callout_pending(&xfer->ux_callout)) {
-		callout_stop(&xfer->ux_callout);
-		printf("usbd_free_xfer: timeout_handle pending\n");
-	}
-#endif
+
+	/* Wait for any straggling timeout to complete. */
+	mutex_enter(xfer->ux_bus->ub_lock);
+	callout_halt(&xfer->ux_callout, xfer->ux_bus->ub_lock);
+	usb_rem_task_wait(xfer->ux_pipe->up_dev, &xfer->ux_aborttask,
+	    USB_TASKQ_HC, xfer->ux_bus->ub_lock);
+	mutex_exit(xfer->ux_bus->ub_lock);
+
 	cv_destroy(&xfer->ux_cv);
 	xfer->ux_bus->ub_methods->ubm_freex(xfer->ux_bus, xfer);
 	return USBD_NORMAL_COMPLETION;
-- 
2.11.0


From 71c7b85d809046f6e59d6c62fdf9b0245a0d14b9 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 10 Aug 2018 05:59:03 +0000
Subject: [PATCH 4/5] New xfer state variables ux_timeout_set and
 ux_timeout_reset.

These are needed because:

- The host controller interrupt cannot wait for the callout or task
  to finish running.

- Nothing in the USBD API as is waits for the callout or task to
  finish running.

- Callers expect to be able to resubmit USB xfers from xfer callbacks
  without waiting for anything to finish running.

The variable ux_timeout_set can be used by a host controller to
decide on submission whether to schedule the callout or to ask an
already-scheduled callout or already-queued task to reschedule the
callout, by setting the variable ux_timeout_reset to true.

When the callout or task runs and sees that ux_timeout_reset is true,
rather than queue the task or abort the xfer, it can instead just
schedule the callout anew.
---
 sys/dev/usb/usbdi.c    |  1 +
 sys/dev/usb/usbdivar.h | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/sys/dev/usb/usbdi.c b/sys/dev/usb/usbdi.c
index ec47aabef6c6..f291178701fa 100644
--- a/sys/dev/usb/usbdi.c
+++ b/sys/dev/usb/usbdi.c
@@ -496,6 +496,7 @@ usbd_free_xfer(struct usbd_xfer *xfer)
 
 	/* Wait for any straggling timeout to complete. */
 	mutex_enter(xfer->ux_bus->ub_lock);
+	xfer->ux_timeout_reset = false; /* do not resuscitate */
 	callout_halt(&xfer->ux_callout, xfer->ux_bus->ub_lock);
 	usb_rem_task_wait(xfer->ux_pipe->up_dev, &xfer->ux_aborttask,
 	    USB_TASKQ_HC, xfer->ux_bus->ub_lock);
diff --git a/sys/dev/usb/usbdivar.h b/sys/dev/usb/usbdivar.h
index 43fd14d2f17a..a8be6a475b8b 100644
--- a/sys/dev/usb/usbdivar.h
+++ b/sys/dev/usb/usbdivar.h
@@ -288,6 +288,19 @@ struct usbd_xfer {
 
 	struct usb_task		ux_aborttask;
 	struct callout		ux_callout;
+
+	/*
+	 * Protected by bus lock.
+	 *
+	 * - ux_timeout_set: The timeout is scheduled as a callout or
+	 *   usb task, and has not yet acquired the bus lock.
+	 *
+	 * - ux_timeout_reset: The xfer completed, and was resubmitted
+	 *   before the callout or task was able to acquire the bus
+	 *   lock, so one or the other needs to schedule a new callout.
+	 */
+	bool			ux_timeout_set;
+	bool			ux_timeout_reset;
 };
 
 void usbd_init(void);
-- 
2.11.0


From 507b4ece29c0b43e5137c411368e21f6f324e80a Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 10 Aug 2018 06:04:07 +0000
Subject: [PATCH 5/5] Fix steady state of timeouts in ehci.

---
 sys/dev/usb/ehci.c | 254 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 211 insertions(+), 43 deletions(-)

diff --git a/sys/dev/usb/ehci.c b/sys/dev/usb/ehci.c
index 66f7447edd84..ba71458ddb3d 100644
--- a/sys/dev/usb/ehci.c
+++ b/sys/dev/usb/ehci.c
@@ -168,6 +168,9 @@ Static void		ehci_check_sitd_intr(ehci_softc_t *, struct ehci_xfer *,
 Static void		ehci_idone(struct ehci_xfer *, ex_completeq_t *);
 Static void		ehci_timeout(void *);
 Static void		ehci_timeout_task(void *);
+Static bool		ehci_probe_timeout(struct usbd_xfer *);
+Static void		ehci_schedule_timeout(struct usbd_xfer *);
+Static void		ehci_cancel_timeout(struct usbd_xfer *);
 Static void		ehci_intrlist_timeout(void *);
 Static void		ehci_doorbell(void *);
 Static void		ehci_pcd(void *);
@@ -1054,13 +1057,10 @@ ehci_idone(struct ehci_xfer *ex, ex_completeq_t *cq)
 	}
 
 	/*
-	 * Cancel the timeout and the task, which have not yet
-	 * run.  If they have already fired, at worst they are
-	 * waiting for the lock.  They will see that the xfer
-	 * is no longer in progress and give up.
+	 * Cancel the timeout.  The callout or task may be running, and
+	 * waiting for the bus lock, but they will be harmless.
 	 */
-	callout_stop(&xfer->ux_callout);
-	usb_rem_task(xfer->ux_pipe->up_dev, &xfer->ux_aborttask);
+	ehci_cancel_timeout(xfer);
 
 #ifdef DIAGNOSTIC
 #ifdef EHCI_DEBUG
@@ -3147,35 +3147,28 @@ ehci_abort_xfer(struct usbd_xfer *xfer, usbd_status status)
 	KASSERT(mutex_owned(&sc->sc_lock));
 	ASSERT_SLEEPABLE();
 
-	if (status == USBD_CANCELLED) {
-		/*
-		 * We are synchronously aborting.  Try to stop the
-		 * callout and task, but if we can't, wait for them to
-		 * complete.
-		 */
-		callout_halt(&xfer->ux_callout, &sc->sc_lock);
-		usb_rem_task_wait(xfer->ux_pipe->up_dev, &xfer->ux_aborttask,
-		    USB_TASKQ_HC, &sc->sc_lock);
-	} else {
-		/* Otherwise, we are timing out.  */
-		KASSERT(status == USBD_TIMEOUT);
-	}
-
 	/*
-	 * The xfer cannot have been cancelled already.  It is the
-	 * responsibility of the caller of usbd_abort_pipe not to try
-	 * to abort a pipe multiple times, whether concurrently or
-	 * sequentially.
+	 * Nobody else can set this status: only one caller can time
+	 * out, and only one caller can synchronously abort.  So the
+	 * status can't be the status we're trying to set this to.
 	 */
-	KASSERT(xfer->ux_status != USBD_CANCELLED);
-
-	/* Only the timeout, which runs only once, can time it out.  */
-	KASSERT(xfer->ux_status != USBD_TIMEOUT);
+	KASSERT(xfer->ux_status != status);
 
-	/* If anyone else beat us, we're done.  */
+	/*
+	 * If host controller or timer interrupt has completed it, too
+	 * late to abort.  Forget about it.
+	 */
 	if (xfer->ux_status != USBD_IN_PROGRESS)
 		return;
 
+	if (status == USBD_CANCELLED) {
+		/* We are synchronously aborting.  Cancel the timeout.  */
+		ehci_cancel_timeout(xfer);
+	} else {
+		/* Otherwise, we are timing out.  No action needed.  */
+		KASSERT(status == USBD_TIMEOUT);
+	}
+
 	/* We beat everyone else.  Claim the status.  */
 	xfer->ux_status = status;
 
@@ -3402,7 +3395,8 @@ ehci_timeout(void *addr)
 #endif
 
 	mutex_enter(&sc->sc_lock);
-	if (!sc->sc_dying && xfer->ux_status == USBD_IN_PROGRESS)
+	callout_ack(&xfer->ux_callout);
+	if (ehci_probe_timeout(xfer))
 		usb_add_task(dev, &xfer->ux_aborttask, USB_TASKQ_HC);
 	mutex_exit(&sc->sc_lock);
 }
@@ -3418,10 +3412,193 @@ ehci_timeout_task(void *addr)
 	DPRINTF("xfer=%#jx", (uintptr_t)xfer, 0, 0, 0);
 
 	mutex_enter(&sc->sc_lock);
-	ehci_abort_xfer(xfer, USBD_TIMEOUT);
+	if (ehci_probe_timeout(xfer))
+		ehci_abort_xfer(xfer, USBD_TIMEOUT);
 	mutex_exit(&sc->sc_lock);
 }
 
+/*
+ * ehci_probe_timeout(xfer)
+ *
+ *	Probe the status of xfer's timeout.  Acknowledge and process a
+ *	request to reschedule.  Return true if the timeout is still
+ *	valid and the caller should take further action (queueing a
+ *	task or aborting the xfer), false if it must stop here.
+ */
+Static bool
+ehci_probe_timeout(struct usbd_xfer *xfer)
+{
+	EHCIHIST_FUNC(); EHCIHIST_CALLED();
+	struct ehci_softc *sc = EHCI_XFER2SC(xfer);
+	bool valid;
+
+	KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+
+	/* The timeout must be set.  */
+	KASSERT(xfer->ux_timeout_set);
+
+	/*
+	 * Neither callout nor task may be pending; they execute
+	 * alternately in lock step.
+	 */
+	KASSERT(!callout_pending(&xfer->ux_callout));
+	KASSERT(!usb_task_pending(xfer->ux_pipe->up_dev, &xfer->ux_aborttask));
+
+	/* There are a few cases... */
+	if (sc->sc_dying) {
+		/* Host controller dying.  Drop it all on the floor.  */
+		xfer->ux_timeout_set = false;
+		xfer->ux_timeout_reset = false;
+		valid = false;
+	} else if (xfer->ux_timeout_reset) {
+		/*
+		 * The xfer completed _and_ got resubmitted while we
+		 * waited for the lock.  Acknowledge the request to
+		 * reschedule, and reschedule it if there is a timeout
+		 * and the bus is not polling.
+		 */
+		xfer->ux_timeout_reset = false;
+		if (xfer->ux_timeout && !sc->sc_bus.ub_usepolling) {
+			KASSERT(xfer->ux_timeout_set);
+			callout_schedule(&xfer->ux_callout,
+			    mstohz(xfer->ux_timeout));
+		} else {
+			/* No more callout or task scheduled.  */
+			xfer->ux_timeout_set = false;
+		}
+		valid = false;
+	} else if (xfer->ux_status != USBD_IN_PROGRESS) {
+		/*
+		 * The xfer has completed by hardware completion or by
+		 * software abort.  The timeout is no longer valid.
+		 */
+		valid = false;
+	} else {
+		/*
+		 * The xfer has completed, but has not been
+		 * resubmitted, so the timeout is valid.
+		 */
+		valid = true;
+	}
+
+	/* Any reset must have been processed.  */
+	KASSERT(!xfer->ux_timeout_reset);
+
+	/*
+	 * Neither callout nor task may be pending; they execute
+	 * alternately in lock step.
+	 */
+	KASSERT(!callout_pending(&xfer->ux_callout));
+	KASSERT(!usb_task_pending(xfer->ux_pipe->up_dev, &xfer->ux_aborttask));
+
+	KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+
+	return valid;
+}
+
+/*
+ * ehci_schedule_timeout(xfer)
+ *
+ *	Ensure that xfer has a timeout.  If the callout is already
+ *	queued or the task is already running, request that they
+ *	reschedule the callout.  If not, and if we're not polling,
+ *	schedule the callout anew.
+ */
+Static void
+ehci_schedule_timeout(struct usbd_xfer *xfer)
+{
+	EHCIHIST_FUNC(); EHCIHIST_CALLED();
+	ehci_softc_t *sc = EHCI_XFER2SC(xfer);
+
+	KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+
+	if (xfer->ux_timeout_set) {
+		/*
+		 * Callout or task has fired from a prior completed
+		 * xfer but has not yet noticed that the xfer is done.
+		 * Ask it to reschedule itself to ux_timeout.
+		 */
+		xfer->ux_timeout_reset = true;
+	} else if (xfer->ux_timeout && !sc->sc_bus.ub_usepolling) {
+		/* Callout is not scheduled.  Schedule it.  */
+		KASSERT(!callout_pending(&xfer->ux_callout));
+		callout_reset(&xfer->ux_callout, mstohz(xfer->ux_timeout),
+		    ehci_timeout, xfer);
+		xfer->ux_timeout_set = true;
+	}
+
+	KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+}
+
+/*
+ * ehci_cancel_timeout(xfer)
+ *
+ *	Cancel the callout and the task of xfer, which have not yet run
+ *	to completion.  If they have already fired, at worst they are
+ *	waiting for the bus lock.  They will see that the xfer is no
+ *	longer in progress and give up, or they will see that the xfer
+ *	has been resubmitted with a new timeout and reschedule the
+ *	callout.
+ *
+ *	If a resubmitted request completed so fast that the callout
+ *	didn't have time to process a timer reset, just cancel the
+ *	timer reset.
+ */
+Static void
+ehci_cancel_timeout(struct usbd_xfer *xfer)
+{
+	EHCIHIST_FUNC(); EHCIHIST_CALLED();
+	struct ehci_softc *sc = EHCI_XFER2SC(xfer);
+
+	KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+
+	KASSERT(xfer->ux_timeout_set);
+	xfer->ux_timeout_reset = false;
+	if (!callout_stop(&xfer->ux_callout)) {
+		/*
+		 * We stopped the callout before it ran.  The timeout
+		 * is no longer set.
+		 */
+		xfer->ux_timeout_set = false;
+	} else if (callout_invoking(&xfer->ux_callout)) {
+		/*
+		 * The callout has begun to run but it has not yet
+		 * acquired the lock.  The task cannot be queued yet,
+		 * and the callout cannot have been rescheduled yet.
+		 *
+		 * By the time the callout acquires the lock, we will
+		 * have transitioned from USBD_IN_PROGRESS to a
+		 * completed status, and possibly also resubmitted the
+		 * xfer and set xfer->ux_timeout_reset = true.  In both
+		 * cases, the callout will DTRT, so no further action
+		 * is needed here.
+		 */
+	} else if (usb_rem_task(xfer->ux_pipe->up_dev, &xfer->ux_aborttask)) {
+		/*
+		 * The callout had fired and scheduled the task, but we
+		 * stopped the task before it could run.  The timeout
+		 * is therefore no longer set -- the next resubmission
+		 * of the xfer must schedule a new timeout.
+		 *
+		 * The callout should not be be pending at this point:
+		 * it is scheduled only under the lock, and only when
+		 * xfer->ux_timeout_set is false, or by the callout or
+		 * task itself when xfer->ux_timeout_reset is true.
+		 */
+		xfer->ux_timeout_set = false;
+	}
+
+	/*
+	 * The callout cannot be scheduled and the task cannot be
+	 * queued at this point.  Either we cancelled them, or they are
+	 * already running and waiting for the bus lock.
+	 */
+	KASSERT(!callout_pending(&xfer->ux_callout));
+	KASSERT(!usb_task_pending(xfer->ux_pipe->up_dev, &xfer->ux_aborttask));
+
+	KASSERT(sc->sc_bus.ub_usepolling || mutex_owned(&sc->sc_lock));
+}
+
 /************************/
 
 Static int
@@ -3661,10 +3838,7 @@ ehci_device_ctrl_start(struct usbd_xfer *xfer)
 
 	/* Insert qTD in QH list - also does usb_syncmem(sqh) */
 	ehci_set_qh_qtd(sqh, setup);
-	if (xfer->ux_timeout && !sc->sc_bus.ub_usepolling) {
-		callout_reset(&xfer->ux_callout, mstohz(xfer->ux_timeout),
-		    ehci_timeout, xfer);
-	}
+	ehci_schedule_timeout(xfer);
 	ehci_add_intr_list(sc, exfer);
 	xfer->ux_status = USBD_IN_PROGRESS;
 	mutex_exit(&sc->sc_lock);
@@ -3858,10 +4032,7 @@ ehci_device_bulk_start(struct usbd_xfer *xfer)
 
 	/* also does usb_syncmem(sqh) */
 	ehci_set_qh_qtd(sqh, exfer->ex_sqtdstart);
-	if (xfer->ux_timeout && !sc->sc_bus.ub_usepolling) {
-		callout_reset(&xfer->ux_callout, mstohz(xfer->ux_timeout),
-		    ehci_timeout, xfer);
-	}
+	ehci_schedule_timeout(xfer);
 	ehci_add_intr_list(sc, exfer);
 	xfer->ux_status = USBD_IN_PROGRESS;
 	mutex_exit(&sc->sc_lock);
@@ -4073,10 +4244,7 @@ ehci_device_intr_start(struct usbd_xfer *xfer)
 
 	/* also does usb_syncmem(sqh) */
 	ehci_set_qh_qtd(sqh, exfer->ex_sqtdstart);
-	if (xfer->ux_timeout && !sc->sc_bus.ub_usepolling) {
-		callout_reset(&xfer->ux_callout, mstohz(xfer->ux_timeout),
-		    ehci_timeout, xfer);
-	}
+	ehci_schedule_timeout(xfer);
 	ehci_add_intr_list(sc, exfer);
 	xfer->ux_status = USBD_IN_PROGRESS;
 	mutex_exit(&sc->sc_lock);
-- 
2.11.0