bring back changes in usb.c revision 1.73

date: 2011/02/04 08:21:39;  author: jakemsr;  state: Exp;  lines: +85 -34
* merge 'onqueue' and 'running' members of struct usb_task into
  a new member, 'state'.
* add new function 'usb_wait_task()', which waits for queued or running
  usb_tasks to complete.
* in the USB_DEVICEINFO ioctl, fill struct usb_device_info in a
  usb_task, thereby avoiding races against driver attach/detach.

but this time make the state a bitmask, since it is valid to be added
to the task queue while the task is running.  also be more careful
about waking up the task when state changes.

ok miod@

2 files changed, 121 insertions, 52 deletions

diff --git a/sys/dev/usb/usb.c b/sys/dev/usb/usb.c
index af452b9c7e7..99b3eecd7db 100644
--- a/sys/dev/usb/usb.c
+++ b/sys/dev/usb/usb.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: usb.c,v 1.74 2011/02/09 04:25:32 jakemsr Exp $	*/
+/*	$OpenBSD: usb.c,v 1.75 2011/02/09 20:24:39 jakemsr Exp $	*/
 /*	$NetBSD: usb.c,v 1.77 2003/01/01 00:10:26 thorpej Exp $	*/
 
 /*
@@ -275,15 +275,15 @@ usb_add_task(usbd_device_handle dev, struct usb_task *task)
 {
 	int s;
 
-	DPRINTFN(2,("%s: task=%p onqueue=%d type=%d\n", __func__, task,
-	    task->onqueue, task->type));
+	DPRINTFN(2,("%s: task=%p state=%d type=%d\n", __func__, task,
+	    task->state, task->type));
 
 	/* Don't add task if the device's root hub is dying. */
 	if (usbd_is_dying(dev))
 		return;
 
 	s = splusb();
-	if (!task->onqueue) {
+	if (!(task->state & USB_TASK_STATE_ONQ)) {
 		switch (task->type) {
 		case USB_TASK_TYPE_ABORT:
 			TAILQ_INSERT_TAIL(&usb_abort_tasks, task, next);
@@ -295,7 +295,7 @@ usb_add_task(usbd_device_handle dev, struct usb_task *task)
 			TAILQ_INSERT_TAIL(&usb_generic_tasks, task, next);
 			break;
 		}
-		task->onqueue = 1;
+		task->state |= USB_TASK_STATE_ONQ;
 		task->dev = dev;
 	}
 	if (task->type == USB_TASK_TYPE_ABORT)
@@ -310,38 +310,45 @@ usb_rem_task(usbd_device_handle dev, struct usb_task *task)
 {
 	int s;
 
-	DPRINTFN(2,("%s: task=%p onqueue=%d type=%d\n", __func__, task,
-	    task->onqueue, task->type));
+	DPRINTFN(2,("%s: task=%p state=%d type=%d\n", __func__, task,
+	    task->state, task->type));
+
+	if (!(task->state & USB_TASK_STATE_ONQ))
+		return;
 
 	s = splusb();
-	if (task->onqueue) {
-		switch (task->type) {
-		case USB_TASK_TYPE_ABORT:
-			TAILQ_REMOVE(&usb_abort_tasks, task, next);
-			break;
-		case USB_TASK_TYPE_EXPLORE:
-			TAILQ_REMOVE(&usb_explore_tasks, task, next);
-			break;
-		case USB_TASK_TYPE_GENERIC:
-			TAILQ_REMOVE(&usb_generic_tasks, task, next);
-			break;
-		}
-		task->onqueue = 0;
+
+	switch (task->type) {
+	case USB_TASK_TYPE_ABORT:
+		TAILQ_REMOVE(&usb_abort_tasks, task, next);
+		break;
+	case USB_TASK_TYPE_EXPLORE:
+		TAILQ_REMOVE(&usb_explore_tasks, task, next);
+		break;
+	case USB_TASK_TYPE_GENERIC:
+		TAILQ_REMOVE(&usb_generic_tasks, task, next);
+		break;
 	}
+	task->state &= ~USB_TASK_STATE_ONQ;
+	if (task->state == USB_TASK_STATE_NONE)
+		wakeup(task);
+
 	splx(s);
 }
 
 void
-usb_rem_wait_task(usbd_device_handle dev, struct usb_task *task)
+usb_wait_task(usbd_device_handle dev, struct usb_task *task)
 {
 	int s;
 
-	DPRINTFN(2,("%s: task=%p onqueue=%d type=%d\n", __func__, task,
-	    task->onqueue, task->type));
+	DPRINTFN(2,("%s: task=%p state=%d type=%d\n", __func__, task,
+	    task->state, task->type));
+
+	if (task->state == USB_TASK_STATE_NONE)
+		return;
 
 	s = splusb();
-	usb_rem_task(dev, task);
-	while (task->running) {
+	while (task->state != USB_TASK_STATE_NONE) {
 		DPRINTF(("%s: waiting for task to complete\n", __func__));
 		tsleep(task, PWAIT, "endtask", 0);
 	}
@@ -349,6 +356,13 @@ usb_rem_wait_task(usbd_device_handle dev, struct usb_task *task)
 }
 
 void
+usb_rem_wait_task(usbd_device_handle dev, struct usb_task *task)
+{
+	usb_rem_task(dev, task);
+	usb_wait_task(dev, task);
+}
+
+void
 usb_first_explore(void *arg)
 {
 	struct usb_softc *sc = arg;
@@ -406,16 +420,23 @@ usb_task_thread(void *arg)
 			tsleep(&usb_run_tasks, PWAIT, "usbtsk", 0);
 			continue;
 		}
-		task->onqueue = 0;
-		/* Don't execute the task if the root hub is gone. */
-		if (usbd_is_dying(task->dev))
-			continue;
-		task->running = 1;
-		splx(s);
-		task->fun(task->arg);
-		s = splusb();
-		task->running = 0;
-		wakeup(task);
+		/*
+		 * Set the state run bit before clearing the onq bit.
+		 * This avoids state == none between dequeue and
+		 * execution, which could cause usb_wait_task() to do
+		 * the wrong thing.
+		 */
+		task->state |= USB_TASK_STATE_RUN;
+		task->state &= ~USB_TASK_STATE_ONQ;
+		/* Don't actually execute the task if dying. */
+		if (!usbd_is_dying(task->dev)) {
+			splx(s);
+			task->fun(task->arg);
+			s = splusb();
+		}
+		task->state &= ~USB_TASK_STATE_RUN;
+		if (task->state == USB_TASK_STATE_NONE)
+			wakeup(task);
 	}
 	splx(s);
 
@@ -443,16 +464,23 @@ usb_abort_task_thread(void *arg)
 			tsleep(&usb_run_abort_tasks, PWAIT, "usbatsk", 0);
 			continue;
 		}
-		task->onqueue = 0;
-		/* Don't execute the task if the root hub is gone. */
-		if (usbd_is_dying(task->dev))
-			continue;
-		task->running = 1;
-		splx(s);
-		task->fun(task->arg);
-		s = splusb();
-		task->running = 0;
-		wakeup(task);
+		/*
+		 * Set the state run bit before clearing the onq bit.
+		 * This avoids state == none between dequeue and
+		 * execution, which could cause usb_wait_task() to do
+		 * the wrong thing.
+		 */
+		task->state |= USB_TASK_STATE_RUN;
+		task->state &= ~USB_TASK_STATE_ONQ;
+		/* Don't actually execute the task if dying. */
+		if (!usbd_is_dying(task->dev)) {
+			splx(s);
+			task->fun(task->arg);
+			s = splusb();
+		}
+		task->state &= ~USB_TASK_STATE_RUN;
+		if (task->state == USB_TASK_STATE_NONE)
+			wakeup(task);
 	}
 	splx(s);
 
@@ -493,6 +521,26 @@ usbclose(dev_t dev, int flag, int mode, struct proc *p)
 	return (0);
 }
 
+void
+usbd_fill_di_task(void *arg)
+{
+	struct usb_device_info *di = (struct usb_device_info *)arg;
+	struct usb_softc *sc;
+	usbd_device_handle dev;
+
+	/* check that the bus and device are still present */
+	if (di->udi_bus >= usb_cd.cd_ndevs)
+		return;
+	sc = usb_cd.cd_devs[di->udi_bus];
+	if (sc == NULL)
+		return;
+	dev = sc->sc_bus->devices[di->udi_addr];
+	if (dev == NULL)
+		return;
+
+	usbd_fill_deviceinfo(dev, di, 1);
+}
+
 int
 usbioctl(dev_t devt, u_long cmd, caddr_t data, int flag, struct proc *p)
 {
@@ -589,14 +637,31 @@ usbioctl(dev_t devt, u_long cmd, caddr_t data, int flag, struct proc *p)
 	{
 		struct usb_device_info *di = (void *)data;
 		int addr = di->udi_addr;
+		struct usb_task di_task;
 		usbd_device_handle dev;
 
 		if (addr < 1 || addr >= USB_MAX_DEVICES)
 			return (EINVAL);
+
 		dev = sc->sc_bus->devices[addr];
 		if (dev == NULL)
 			return (ENXIO);
-		usbd_fill_deviceinfo(dev, di, 1);
+
+		di->udi_bus = unit;
+
+		/* All devices get a driver, thanks to ugen(4).  If the
+		 * task ends without adding a driver name, there was an error.
+		 */
+		di->udi_devnames[0][0] = '\0';
+
+		usb_init_task(&di_task, usbd_fill_di_task, di,
+		    USB_TASK_TYPE_GENERIC);
+		usb_add_task(sc->sc_bus->root_hub, &di_task);
+		usb_wait_task(sc->sc_bus->root_hub, &di_task);
+
+		if (di->udi_devnames[0][0] == '\0')
+			return (ENXIO);
+
 		break;
 	}
 
diff --git a/sys/dev/usb/usbdi.h b/sys/dev/usb/usbdi.h
index 9aaa0bba5f9..fddeb194c1a 100644
--- a/sys/dev/usb/usbdi.h
+++ b/sys/dev/usb/usbdi.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: usbdi.h,v 1.41 2011/02/09 04:25:32 jakemsr Exp $ */
+/*	$OpenBSD: usbdi.h,v 1.42 2011/02/09 20:24:39 jakemsr Exp $ */
 /*	$NetBSD: usbdi.h,v 1.62 2002/07/11 21:14:35 augustss Exp $	*/
 /*	$FreeBSD: src/sys/dev/usb/usbdi.h,v 1.18 1999/11/17 22:33:49 n_hibma Exp $	*/
 
@@ -139,6 +139,7 @@ usbd_status usbd_set_interface(usbd_interface_handle, int);
 int usbd_get_no_alts(usb_config_descriptor_t *, int);
 usbd_status  usbd_get_interface(usbd_interface_handle iface, u_int8_t *aiface);
 void usbd_fill_deviceinfo(usbd_device_handle, struct usb_device_info *, int);
+void usbd_fill_di_task(void *);
 int usbd_get_interface_altindex(usbd_interface_handle iface);
 
 usb_interface_descriptor_t *usbd_find_idesc(usb_config_descriptor_t *cd,
@@ -193,22 +194,25 @@ struct usb_task {
 	void (*fun)(void *);
 	void *arg;
 	char type;
-#define USB_TASK_TYPE_GENERIC	0
+#define	USB_TASK_TYPE_GENERIC	0
 #define USB_TASK_TYPE_EXPLORE	1
 #define USB_TASK_TYPE_ABORT	2
-	char onqueue;
-	char running;
+	u_int state;
+#define	USB_TASK_STATE_NONE	0x0
+#define	USB_TASK_STATE_ONQ	0x1
+#define	USB_TASK_STATE_RUN	0x2
+
 };
 
 void usb_add_task(usbd_device_handle, struct usb_task *);
 void usb_rem_task(usbd_device_handle, struct usb_task *);
+void usb_wait_task(usbd_device_handle, struct usb_task *);
 void usb_rem_wait_task(usbd_device_handle, struct usb_task *);
 #define usb_init_task(t, f, a, y) \
 	((t)->fun = (f),	\
 	(t)->arg = (a),		\
 	(t)->type = (y),	\
-	(t)->onqueue = 0,	\
-	(t)->running = 0)
+	(t)->state = USB_TASK_STATE_NONE)
 
 struct usb_devno {
 	u_int16_t ud_vendor;