On Fri, Jan 28, 2011 at 09:17:22PM +0000, Jacob Meuser wrote: > 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.
I could use some more test reports. this is in the latest snapshots for all archs. 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; -- jake...@sdf.lonestar.org SDF Public Access UNIX System - http://sdf.lonestar.org
