currently, the USB_DEVICEINFO ioctl (used by usbdevs(8)) races against usb device attach/detach. this is particularly problematic for device detachment. if the ioctl is running exactly when a hub with devices attached is removed, there's a good chance of crashing.
the following diff makes the racing parts of the ioctl run as a usb_task in the task thread. since attach and detach are also run as usb_tasks in the task thread, the ioctl and attach/detach no longer race. this also requires the addition of a new function, usb_wait_task(), which waits for a task to be completed. I've also condensed the two members of struct usb_task that represent state into one member. *please test on as many systems as possible.* this won't make 4.9 without wide testing. thanks. -- jake...@sdf.lonestar.org SDF Public Access UNIX System - http://sdf.lonestar.org Index: usb.c =================================================================== RCS file: /cvs/src/sys/dev/usb/usb.c,v retrieving revision 1.72 diff -u -p usb.c --- usb.c 25 Jan 2011 20:03:36 -0000 1.72 +++ usb.c 28 Jan 2011 20:33:50 -0000 @@ -275,15 +275,15 @@ usb_add_task(usbd_device_handle dev, struct usb_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 * 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,43 @@ usb_rem_task(usbd_device_handle dev, struct usb_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_NONE; + 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 +354,13 @@ usb_rem_wait_task(usbd_device_handle dev, struct usb_t } 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,15 +418,16 @@ 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)) + if (usbd_is_dying(task->dev)) { + task->state = USB_TASK_STATE_NONE; continue; - task->running = 1; + } + task->state = USB_TASK_STATE_RUN; splx(s); task->fun(task->arg); s = splusb(); - task->running = 0; + task->state = USB_TASK_STATE_NONE; wakeup(task); } splx(s); @@ -443,15 +456,16 @@ 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)) + if (usbd_is_dying(task->dev)) { + task->state = USB_TASK_STATE_NONE; continue; - task->running = 1; + } + task->state = USB_TASK_STATE_RUN; splx(s); task->fun(task->arg); s = splusb(); - task->running = 0; + task->state = USB_TASK_STATE_NONE; wakeup(task); } splx(s); @@ -493,6 +507,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 +623,31 @@ usbioctl(dev_t devt, u_long cmd, caddr_t data, int fla { 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; } Index: usbdi.h =================================================================== RCS file: /cvs/src/sys/dev/usb/usbdi.h,v retrieving revision 1.39 diff -u -p usbdi.h --- usbdi.h 25 Jan 2011 20:03:36 -0000 1.39 +++ usbdi.h 28 Jan 2011 20:33:51 -0000 @@ -139,6 +139,7 @@ usbd_status usbd_set_interface(usbd_interface_handle, 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, @@ -187,28 +188,33 @@ const usb_descriptor_t *usb_desc_iter_next(usbd_desc_i * has been detected. But it may also be used by drivers that need to * perform (short) tasks that must have a process context. */ +enum usb_task_state { + USB_TASK_STATE_NONE, + USB_TASK_STATE_ONQ, + USB_TASK_STATE_RUN +}; + struct usb_task { TAILQ_ENTRY(usb_task) next; usbd_device_handle dev; 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; + enum usb_task_state state; }; 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;
