From d0fa21538c397e352c328cc82df3fe335882fb09 Mon Sep 17 00:00:00 2001 From: hselasky Date: Thu, 5 Feb 2015 22:03:12 +0000 Subject: [PATCH] MFC r277136: Resolve a special case deadlock: When two or more threads are simultaneously detaching kernel drivers on the same USB device we can get stuck in the "usb_wait_pending_ref_locked()" function because the conditions needed for allowing detach are not met. While at it ensure that "flag_iserror" is only written when "priv_mtx" is locked, which is protecting it. git-svn-id: svn://svn.freebsd.org/base/stable/8@278295 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f --- sys/dev/usb/controller/usb_controller.c | 39 +++++++++ sys/dev/usb/usb_bus.h | 6 ++ sys/dev/usb/usb_dev.c | 12 +-- sys/dev/usb/usb_device.c | 102 ++++++++++-------------- sys/dev/usb/usb_device.h | 1 + 5 files changed, 94 insertions(+), 66 deletions(-) diff --git a/sys/dev/usb/controller/usb_controller.c b/sys/dev/usb/controller/usb_controller.c index 12bf583d0..5c447ae24 100644 --- a/sys/dev/usb/controller/usb_controller.c +++ b/sys/dev/usb/controller/usb_controller.c @@ -56,6 +56,7 @@ #include #include #include +#include #include #include @@ -205,6 +206,11 @@ usb_detach(device_t dev) usb_proc_mwait(&bus->explore_proc, &bus->detach_msg[0], &bus->detach_msg[1]); +#if USB_HAVE_UGEN + /* Wait for cleanup to complete */ + usb_proc_mwait(&bus->explore_proc, + &bus->cleanup_msg[0], &bus->cleanup_msg[1]); +#endif USB_BUS_UNLOCK(bus); /* Get rid of USB callback processes */ @@ -612,6 +618,32 @@ usb_bus_shutdown(struct usb_proc_msg *pm) USB_BUS_LOCK(bus); } +/*------------------------------------------------------------------------* + * usb_bus_cleanup + * + * This function is used to cleanup leftover USB character devices. + *------------------------------------------------------------------------*/ +#if USB_HAVE_UGEN +static void +usb_bus_cleanup(struct usb_proc_msg *pm) +{ + struct usb_bus *bus; + struct usb_fs_privdata *pd; + + bus = ((struct usb_bus_msg *)pm)->bus; + + while ((pd = LIST_FIRST(&bus->pd_cleanup_list)) != NULL) { + + LIST_REMOVE(pd, pd_next); + USB_BUS_UNLOCK(bus); + + usb_destroy_dev_sync(pd); + + USB_BUS_LOCK(bus); + } +} +#endif + static void usb_power_wdog(void *arg) { @@ -792,6 +824,13 @@ usb_attach_sub(device_t dev, struct usb_bus *bus) bus->shutdown_msg[1].hdr.pm_callback = &usb_bus_shutdown; bus->shutdown_msg[1].bus = bus; +#if USB_HAVE_UGEN + LIST_INIT(&bus->pd_cleanup_list); + bus->cleanup_msg[0].hdr.pm_callback = &usb_bus_cleanup; + bus->cleanup_msg[0].bus = bus; + bus->cleanup_msg[1].hdr.pm_callback = &usb_bus_cleanup; + bus->cleanup_msg[1].bus = bus; +#endif /* Create USB explore and callback processes */ if (usb_proc_create(&bus->giant_callback_proc, diff --git a/sys/dev/usb/usb_bus.h b/sys/dev/usb/usb_bus.h index dd6b880a9..825d2ef7a 100644 --- a/sys/dev/usb/usb_bus.h +++ b/sys/dev/usb/usb_bus.h @@ -27,6 +27,8 @@ #ifndef _USB_BUS_H_ #define _USB_BUS_H_ +struct usb_fs_privdata; + /* * The following structure defines the USB explore message sent to the USB * explore process. @@ -73,6 +75,10 @@ struct usb_bus { struct usb_bus_msg resume_msg[2]; struct usb_bus_msg reset_msg[2]; struct usb_bus_msg shutdown_msg[2]; +#if USB_HAVE_UGEN + struct usb_bus_msg cleanup_msg[2]; + LIST_HEAD(,usb_fs_privdata) pd_cleanup_list; +#endif /* * This mutex protects the USB hardware: */ diff --git a/sys/dev/usb/usb_dev.c b/sys/dev/usb/usb_dev.c index 391f62f87..9b0bdac0d 100644 --- a/sys/dev/usb/usb_dev.c +++ b/sys/dev/usb/usb_dev.c @@ -288,8 +288,8 @@ error: usbd_enum_unlock(cpd->udev); if (crd->is_uref) { - cpd->udev->refcount--; - cv_broadcast(&cpd->udev->ref_cv); + if (--(cpd->udev->refcount) == 0) + cv_broadcast(&cpd->udev->ref_cv); } mtx_unlock(&usb_ref_lock); DPRINTFN(2, "fail\n"); @@ -360,8 +360,8 @@ usb_unref_device(struct usb_cdev_privdata *cpd, } if (crd->is_uref) { crd->is_uref = 0; - cpd->udev->refcount--; - cv_broadcast(&cpd->udev->ref_cv); + if (--(cpd->udev->refcount) == 0) + cv_broadcast(&cpd->udev->ref_cv); } mtx_unlock(&usb_ref_lock); } @@ -587,12 +587,12 @@ usb_fifo_free(struct usb_fifo *f) /* decrease refcount */ f->refcount--; - /* prevent any write flush */ - f->flag_iserror = 1; /* need to wait until all callers have exited */ while (f->refcount != 0) { mtx_unlock(&usb_ref_lock); /* avoid LOR */ mtx_lock(f->priv_mtx); + /* prevent write flush, if any */ + f->flag_iserror = 1; /* get I/O thread out of any sleep state */ if (f->flag_sleeping) { f->flag_sleeping = 0; diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c index 6b8766af4..374336649 100644 --- a/sys/dev/usb/usb_device.c +++ b/sys/dev/usb/usb_device.c @@ -430,68 +430,29 @@ usb_endpoint_foreach(struct usb_device *udev, struct usb_endpoint *ep) return (NULL); } -#if USB_HAVE_UGEN -static uint16_t -usb_get_refcount(struct usb_device *udev) -{ - if (usb_proc_is_called_from(&udev->bus->explore_proc) || - usb_proc_is_called_from(&udev->bus->control_xfer_proc)) - return (1); - return (2); -} -#endif - /*------------------------------------------------------------------------* - * usb_wait_pending_ref_locked + * usb_wait_pending_refs * * This function will wait for any USB references to go away before - * returning and disable further USB device refcounting on the - * specified USB device. This function is used when detaching a USB - * device. + * returning. This function is used before freeing a USB device. *------------------------------------------------------------------------*/ static void -usb_wait_pending_ref_locked(struct usb_device *udev) +usb_wait_pending_refs(struct usb_device *udev) { #if USB_HAVE_UGEN - const uint16_t refcount = usb_get_refcount(udev); - - DPRINTF("Refcount = %d\n", (int)refcount); + DPRINTF("Refcount = %d\n", (int)udev->refcount); + mtx_lock(&usb_ref_lock); + udev->refcount--; while (1) { /* wait for any pending references to go away */ - mtx_lock(&usb_ref_lock); - if (udev->refcount == refcount) { - /* prevent further refs being taken */ + if (udev->refcount == 0) { + /* prevent further refs being taken, if any */ udev->refcount = USB_DEV_REF_MAX; - mtx_unlock(&usb_ref_lock); break; } - usbd_enum_unlock(udev); cv_wait(&udev->ref_cv, &usb_ref_lock); - mtx_unlock(&usb_ref_lock); - (void) usbd_enum_lock(udev); } -#endif -} - -/*------------------------------------------------------------------------* - * usb_ref_restore_locked - * - * This function will restore the reference count value after a call - * to "usb_wait_pending_ref_locked()". - *------------------------------------------------------------------------*/ -static void -usb_ref_restore_locked(struct usb_device *udev) -{ -#if USB_HAVE_UGEN - const uint16_t refcount = usb_get_refcount(udev); - - DPRINTF("Refcount = %d\n", (int)refcount); - - /* restore reference count and wakeup waiters, if any */ - mtx_lock(&usb_ref_lock); - udev->refcount = refcount; - cv_broadcast(&udev->ref_cv); mtx_unlock(&usb_ref_lock); #endif } @@ -1157,9 +1118,6 @@ usb_detach_device(struct usb_device *udev, uint8_t iface_index, sx_assert(&udev->enum_sx, SA_LOCKED); - /* wait for pending refs to go away */ - usb_wait_pending_ref_locked(udev); - /* * First detach the child to give the child's detach routine a * chance to detach the sub-devices in the correct order. @@ -1186,8 +1144,6 @@ usb_detach_device(struct usb_device *udev, uint8_t iface_index, usb_detach_device_sub(udev, &iface->subdev, &iface->pnpinfo, flag); } - - usb_ref_restore_locked(udev); } /*------------------------------------------------------------------------* @@ -1999,15 +1955,44 @@ usb_make_dev(struct usb_device *udev, const char *devname, int ep, return (pd); } +void +usb_destroy_dev_sync(struct usb_fs_privdata *pd) +{ + DPRINTFN(1, "Destroying device at ugen%d.%d\n", + pd->bus_index, pd->dev_index); + + /* + * Destroy character device synchronously. After this + * all system calls are returned. Can block. + */ + destroy_dev(pd->cdev); + + free(pd, M_USBDEV); +} + void usb_destroy_dev(struct usb_fs_privdata *pd) { + struct usb_bus *bus; + if (pd == NULL) return; - destroy_dev(pd->cdev); + mtx_lock(&usb_ref_lock); + bus = devclass_get_softc(usb_devclass_ptr, pd->bus_index); + mtx_unlock(&usb_ref_lock); - free(pd, M_USBDEV); + if (bus == NULL) { + usb_destroy_dev_sync(pd); + return; + } + + USB_BUS_LOCK(bus); + LIST_INSERT_HEAD(&bus->pd_cleanup_list, pd, pd_next); + /* get cleanup going */ + usb_proc_msignal(&bus->explore_proc, + &bus->cleanup_msg[0], &bus->cleanup_msg[1]); + USB_BUS_UNLOCK(bus); } static void @@ -2156,6 +2141,9 @@ usb_free_device(struct usb_device *udev, uint8_t flag) &udev->cs_msg[0], &udev->cs_msg[1]); USB_BUS_UNLOCK(udev->bus); + /* wait for all references to go away */ + usb_wait_pending_refs(udev); + sx_destroy(&udev->enum_sx); sx_destroy(&udev->sr_sx); @@ -2716,14 +2704,8 @@ usb_fifo_free_wrap(struct usb_device *udev, /* no need to free this FIFO */ continue; } - /* wait for pending refs to go away */ - usb_wait_pending_ref_locked(udev); - /* free this FIFO */ usb_fifo_free(f); - - /* restore refcount */ - usb_ref_restore_locked(udev); } } #endif diff --git a/sys/dev/usb/usb_device.h b/sys/dev/usb/usb_device.h index 86d6bbc73..172023254 100644 --- a/sys/dev/usb/usb_device.h +++ b/sys/dev/usb/usb_device.h @@ -282,6 +282,7 @@ struct usb_device *usb_alloc_device(device_t parent_dev, struct usb_bus *bus, struct usb_fs_privdata *usb_make_dev(struct usb_device *, const char *, int, int, int, uid_t, gid_t, int); void usb_destroy_dev(struct usb_fs_privdata *); +void usb_destroy_dev_sync(struct usb_fs_privdata *); #endif usb_error_t usb_probe_and_attach(struct usb_device *udev, uint8_t iface_index); -- 2.45.2