Module Name: src
Committed By: riastradh
Date: Wed Feb 12 16:00:34 UTC 2020
Modified Files:
src/sys/dev/usb: ehci.c
Log Message:
Fix steady state of timeouts in ehci.
This is complicated because:
1. There are three ways that an xfer can be completed:
(a) hardware interrupt completes xfer
(b) software decision aborts xfer with USBD_CANCELLED
(c) timeout aborts xfer with USBD_TIMEOUT
2. The timeout abort can't be done in callout because ehci_sync_hc,
called unconditionally by ehci_abort_xfer to wait until the device
has finished using any references to the xfer, may sleep. So we
have to schedule a callout that, when run, will schedule a usb_task.
3. The hardware completion interrupt can't sleep waiting for a callout
or task to finish -- can't use callout_halt or usb_rem_task_wait.
So the callout and usb_task must be able to run _after_ the hardware
completion interrupt, and recognize that they're late to the party.
(Note, though, that usbd_free_xfer does wait for the callout and
task to complete, so there's no danger they may use themselves after
free.)
4. The xfer may resubmitted -- and the timeout may be rescheduled --
immediately after the hardware completion interrupt, _while_ the
callout and/or usb_task may still be scheduled. Specifically, we
may have the following sequence of events:
(a) hardware completion interrupt
(b) callout or usb_task fires
(c) driver resubmits xfer
(d) callout or usb_task acquires lock and looks around dazed and
bewildered at the firehose of events like reading the news in 2019
The mechanism for sorting this out is that we have two bits of state:
- xfer->ux_timeout_set informs the driver, when submitting an xfer and
setting up its timeout, whether either the callout or usb_task is
already scheduled or not.
- xfer->ux_timeout_reset informs the callout or usb_task whether it
should reschedule the callout, because the xfer got resubmitted, or
not.
To generate a diff of this commit:
cvs rdiff -u -r1.269 -r1.270 src/sys/dev/usb/ehci.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/usb/ehci.c
diff -u src/sys/dev/usb/ehci.c:1.269 src/sys/dev/usb/ehci.c:1.270
--- src/sys/dev/usb/ehci.c:1.269 Tue Feb 4 06:30:46 2020
+++ src/sys/dev/usb/ehci.c Wed Feb 12 16:00:34 2020
@@ -1,4 +1,4 @@
-/* $NetBSD: ehci.c,v 1.269 2020/02/04 06:30:46 mrg Exp $ */
+/* $NetBSD: ehci.c,v 1.270 2020/02/12 16:00:34 riastradh Exp $ */
/*
* Copyright (c) 2004-2012 The NetBSD Foundation, Inc.
@@ -53,7 +53,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ehci.c,v 1.269 2020/02/04 06:30:46 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ehci.c,v 1.270 2020/02/12 16:00:34 riastradh Exp $");
#include "ohci.h"
#include "uhci.h"
@@ -168,6 +168,9 @@ Static void ehci_check_sitd_intr(ehci_s
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_async(struct usbd_xfer *);
Static void ehci_intrlist_timeout(void *);
Static void ehci_doorbell(void *);
Static void ehci_pcd(void *);
@@ -1057,13 +1060,11 @@ ehci_idone(struct ehci_xfer *ex, ex_comp
}
/*
- * 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.
+ * We are completing the xfer. Cancel the timeout if we can,
+ * but only asynchronously. See ehci_cancel_timeout_async for
+ * why we need not wait for the callout or task here.
*/
- callout_stop(&xfer->ux_callout);
- usb_rem_task(xfer->ux_pipe->up_dev, &xfer->ux_aborttask);
+ ehci_cancel_timeout_async(xfer);
#ifdef DIAGNOSTIC
#ifdef EHCI_DEBUG
@@ -3235,35 +3236,38 @@ ehci_abort_xfer(struct usbd_xfer *xfer,
KASSERT(mutex_owned(&sc->sc_lock));
ASSERT_SLEEPABLE();
+ /*
+ * 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 != status);
+
+ /*
+ * 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. Try to stop the
- * callout and task, but if we can't, wait for them to
- * complete.
+ * We are synchronously aborting. Cancel the timeout
+ * if we can, but only asynchronously. See
+ * ehci_cancel_timeout_async for why we need not wait
+ * for the callout or task here.
*/
- 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);
+ ehci_cancel_timeout_async(xfer);
} else {
- /* Otherwise, we are timing out. */
+ /*
+ * Otherwise, we are timing out. No action needed to
+ * cancel the timeout because we _are_ the timeout.
+ * This case should happen only via the timeout task,
+ * invoked via the callout.
+ */
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.
- */
- KASSERT(xfer->ux_status != USBD_CANCELLED);
-
- /* Only the timeout, which runs only once, can time it out. */
- KASSERT(xfer->ux_status != USBD_TIMEOUT);
-
- /* If anyone else beat us, we're done. */
- if (xfer->ux_status != USBD_IN_PROGRESS)
- return;
-
/* We beat everyone else. Claim the status. */
xfer->ux_status = status;
@@ -3473,6 +3477,16 @@ dying:
KASSERT(mutex_owned(&sc->sc_lock));
}
+/*
+ * ehci_timeout(xfer)
+ *
+ * Called at IPL_SOFTCLOCK when too much time has elapsed waiting
+ * for xfer to complete. Since we can't abort the xfer at
+ * IPL_SOFTCLOCK, defer to a usb_task to run it in thread context,
+ * unless the xfer has completed or aborted concurrently -- and if
+ * the xfer has also been resubmitted, take care of rescheduling
+ * the callout.
+ */
Static void
ehci_timeout(void *addr)
{
@@ -3489,12 +3503,38 @@ ehci_timeout(void *addr)
}
#endif
+ /* Acquire the lock so we can transition the timeout state. */
mutex_enter(&sc->sc_lock);
- if (!sc->sc_dying && xfer->ux_status == USBD_IN_PROGRESS)
+
+ /*
+ * Use ehci_probe_timeout to check whether the timeout is still
+ * valid, or to reschedule the callout if necessary. If it is
+ * still valid, schedule the task.
+ */
+ if (ehci_probe_timeout(xfer))
usb_add_task(dev, &xfer->ux_aborttask, USB_TASKQ_HC);
+
+ /*
+ * Notify ehci_cancel_timeout_async that we may have scheduled
+ * the task. This causes callout_invoking to return false in
+ * ehci_cancel_timeout_async so that it can tell which stage in
+ * the callout->task->abort process we're at.
+ */
+ callout_ack(&xfer->ux_callout);
+
+ /* All done -- release the lock. */
mutex_exit(&sc->sc_lock);
}
+/*
+ * ehci_timeout_task(xfer)
+ *
+ * Called in thread context when too much time has elapsed waiting
+ * for xfer to complete. Issue ehci_abort_xfer(USBD_TIMEOUT),
+ * unless the xfer has completed or aborted concurrently -- and if
+ * the xfer has also been resubmitted, take care of rescheduling
+ * the callout.
+ */
Static void
ehci_timeout_task(void *addr)
{
@@ -3505,11 +3545,223 @@ ehci_timeout_task(void *addr)
DPRINTF("xfer=%#jx", (uintptr_t)xfer, 0, 0, 0);
+ /* Acquire the lock so we can transition the timeout state. */
mutex_enter(&sc->sc_lock);
- ehci_abort_xfer(xfer, USBD_TIMEOUT);
+
+ /*
+ * Use ehci_probe_timeout to check whether the timeout is still
+ * valid, or to reschedule the callout if necessary. If it is
+ * still valid, schedule the task.
+ */
+ if (ehci_probe_timeout(xfer))
+ ehci_abort_xfer(xfer, USBD_TIMEOUT);
+
+ /* All done -- release the lock. */
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, and has not been resubmitted, so the
+ * timeout must be unset, and is no longer valid for
+ * the caller.
+ */
+ xfer->ux_timeout_set = false;
+ valid = false;
+ } else {
+ /*
+ * The xfer has not yet completed, so the timeout is
+ * valid.
+ */
+ valid = true;
+ }
+
+ /* Any reset must have been processed. */
+ KASSERT(!xfer->ux_timeout_reset);
+
+ /*
+ * Either we claim the timeout is set, or the callout is idle.
+ * If the timeout is still set, we may be handing off to the
+ * task instead, so this is an if but not an iff.
+ */
+ KASSERT(xfer->ux_timeout_set || !callout_pending(&xfer->ux_callout));
+
+ /*
+ * The task must be idle now.
+ *
+ * - If the caller is the callout, _and_ the timeout is still
+ * valid, the caller will schedule it, but it hasn't been
+ * scheduled yet. (If the timeout is not valid, the task
+ * should not be scheduled.)
+ *
+ * - If the caller is the task, it cannot be scheduled again
+ * until the callout runs again, which won't happen until we
+ * next release the lock.
+ */
+ 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_async(xfer)
+ *
+ * Cancel the callout and the task of xfer, which have not yet run
+ * to completion, but don't wait for the callout or task to finish
+ * running.
+ *
+ * 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_async(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 and called callout_ack. 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
@@ -3751,10 +4003,7 @@ ehci_device_ctrl_start(struct usbd_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;
if (!polling)
@@ -3952,10 +4201,7 @@ ehci_device_bulk_start(struct usbd_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;
if (!polling)
@@ -4171,10 +4417,7 @@ ehci_device_intr_start(struct usbd_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;
if (!polling)
