[src/trunk]: src/sys/dev/usb New function usb_rem_task_wait(dev, task, queue).

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/d45dc8f8f4d2
branches:  trunk
changeset: 365035:d45dc8f8f4d2
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Sun Jul 29 01:59:46 2018 +0000

description:
New function usb_rem_task_wait(dev, task, queue).

If task is scheduled to run, removes it from the queue.  If it may
have already begun to run, waits for it to complete.  Caller must
guarantee it will not switch to another queue.  If caller guarantees
it will not be scheduled again, then usb_rem_task_wait guarantees it
is not running on return.

This will enable us to fix a litany of bugs in detach where we
currently fail to wait for a pending task.

diffstat:

 sys/dev/usb/usb.c   |  100 +++++++++++++++++++++++++++++++++++++++++++++++++--
 sys/dev/usb/usbdi.h |    3 +-
 2 files changed, 98 insertions(+), 5 deletions(-)

diffs (173 lines):

diff -r 9f70dbf9cb1d -r d45dc8f8f4d2 sys/dev/usb/usb.c
--- a/sys/dev/usb/usb.c Sun Jul 29 01:45:25 2018 +0000
+++ b/sys/dev/usb/usb.c Sun Jul 29 01:59:46 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: usb.c,v 1.169 2018/06/29 17:48:24 msaitoh Exp $        */
+/*     $NetBSD: usb.c,v 1.170 2018/07/29 01:59:46 riastradh Exp $      */
 
 /*
  * Copyright (c) 1998, 2002, 2008, 2012 The NetBSD Foundation, Inc.
@@ -37,7 +37,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: usb.c,v 1.169 2018/06/29 17:48:24 msaitoh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usb.c,v 1.170 2018/07/29 01:59:46 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_usb.h"
@@ -146,6 +146,7 @@
        kcondvar_t cv;
        struct lwp *task_thread_lwp;
        const char *name;
+       struct usb_task *current_task;
 };
 
 static struct usb_taskq usb_taskq[USB_NUM_TASKQS];
@@ -294,6 +295,7 @@
                mutex_init(&taskq->lock, MUTEX_DEFAULT, IPL_USB);
                cv_init(&taskq->cv, "usbtsk");
                taskq->name = taskq_names[i];
+               taskq->current_task = NULL;
                if (kthread_create(PRI_NONE, KTHREAD_MPSAFE, NULL,
                    usb_task_thread, taskq, &taskq->task_thread_lwp,
                    "%s", taskq->name)) {
@@ -426,8 +428,14 @@
 }
 
 /*
- * XXX This does not wait for completion!  Most uses need such an
- * operation.  Urgh...
+ * usb_rem_task(dev, task)
+ *
+ *     If task is queued to run, remove it from the queue.
+ *
+ *     Caller is _not_ guaranteed that the task is not running when
+ *     this is done.
+ *
+ *     Never sleeps.
  */
 void
 usb_rem_task(struct usbd_device *dev, struct usb_task *task)
@@ -449,6 +457,85 @@
        }
 }
 
+/*
+ * usb_taskq_wait(taskq, task)
+ *
+ *     Wait for taskq to finish executing task, if it is executing
+ *     task.  Caller must hold the taskq lock.
+ */
+static void
+usb_taskq_wait(struct usb_taskq *taskq, struct usb_task *task)
+{
+
+       KASSERT(mutex_owned(&taskq->lock));
+
+       while (taskq->current_task == task)
+               cv_wait(&taskq->cv, &taskq->lock);
+
+       KASSERT(taskq->current_task != task);
+}
+
+/*
+ * usb_rem_task_wait(dev, task, queue)
+ *
+ *     If task is scheduled to run, remove it from the queue.  If it
+ *     may have already begun to run, wait for it to complete.
+ *
+ *     Caller MUST guarantee that task will not be scheduled on a
+ *     _different_ queue, at least until after this returns.
+ *
+ *     If caller guarantees that task will not be scheduled on the
+ *     same queue before this returns, then caller is guaranteed that
+ *     the task is not running at all when this returns.
+ *
+ *     May sleep.
+ */
+void
+usb_rem_task_wait(struct usbd_device *dev, struct usb_task *task, int queue)
+{
+       struct usb_taskq *taskq;
+       int queue1;
+
+       USBHIST_FUNC(); USBHIST_CALLED(usbdebug);
+       ASSERT_SLEEPABLE();
+       KASSERT(0 <= queue);
+       KASSERT(queue < USB_NUM_TASKQS);
+
+       taskq = &usb_taskq[queue];
+
+       if ((queue1 = task->queue) == USB_NUM_TASKQS) {
+               /*
+                * It is not on the queue, but it may have already run.
+                * Wait for it.
+                */
+               mutex_enter(&taskq->lock);
+               usb_taskq_wait(taskq, task);
+               mutex_exit(&taskq->lock);
+       } else {
+               /*
+                * It may be on the queue (and not another one), but
+                * the state may have changed by now because we don't
+                * have the queue locked.  Lock and reload.
+                */
+               KASSERTMSG(queue1 == queue,
+                   "task %p on q%d expected on q%d", task, queue1, queue);
+               mutex_enter(&taskq->lock);
+               queue1 = task->queue;
+               if (queue1 == queue) {
+                       /* Still queued, not run.  Just remove it.  */
+                       TAILQ_REMOVE(&taskq->tasks, task, next);
+                       task->queue = USB_NUM_TASKQS;
+               } else {
+                       /* Already ran.  Wait for it.  */
+                       KASSERTMSG(queue1 == USB_NUM_TASKQS,
+                           "task %p on q%d expected on q%d",
+                           task, queue1, queue);
+                       usb_taskq_wait(taskq, task);
+               }
+               mutex_exit(&taskq->lock);
+       }
+}
+
 void
 usb_event_thread(void *arg)
 {
@@ -517,6 +604,7 @@
                        mpsafe = ISSET(task->flags, USB_TASKQ_MPSAFE);
                        TAILQ_REMOVE(&taskq->tasks, task, next);
                        task->queue = USB_NUM_TASKQS;
+                       taskq->current_task = task;
                        mutex_exit(&taskq->lock);
 
                        if (!mpsafe)
@@ -527,6 +615,10 @@
                                KERNEL_UNLOCK_ONE(curlwp);
 
                        mutex_enter(&taskq->lock);
+                       KASSERTMSG(taskq->current_task == task,
+                           "somebody scribbled on usb taskq %p", taskq);
+                       taskq->current_task = NULL;
+                       cv_broadcast(&taskq->cv);
                }
        }
        mutex_exit(&taskq->lock);
diff -r 9f70dbf9cb1d -r d45dc8f8f4d2 sys/dev/usb/usbdi.h
--- a/sys/dev/usb/usbdi.h       Sun Jul 29 01:45:25 2018 +0000
+++ b/sys/dev/usb/usbdi.h       Sun Jul 29 01:59:46 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: usbdi.h,v 1.92 2016/08/14 14:42:22 skrll Exp $ */
+/*     $NetBSD: usbdi.h,v 1.93 2018/07/29 01:59:46 riastradh Exp $     */
 /*     $FreeBSD: src/sys/dev/usb/usbdi.h,v 1.18 1999/11/17 22:33:49 n_hibma Exp $      */
 
 /*
@@ -219,6 +219,7 @@
 
 void usb_add_task(struct usbd_device *, struct usb_task *, int);
 void usb_rem_task(struct usbd_device *, struct usb_task *);
+void usb_rem_task_wait(struct usbd_device *, struct usb_task *, int);
 #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 {