Date: Tue, 13 Jan 2015 16:37:44 +0000 (UTC) From: Hans Petter Selasky <hselasky@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r277136 - in head/sys/dev/usb: . controller Message-ID: <201501131637.t0DGbipG094270@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: hselasky Date: Tue Jan 13 16:37:43 2015 New Revision: 277136 URL: https://svnweb.freebsd.org/changeset/base/277136 Log: 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. The "destroy_dev()" function waits for all system calls involving the given character device to return. Character device system calls may lock the USB enumeration lock, which is also held when "destroy_dev()" is called. This can sometimes lead to a deadlock not noticed by WITNESS. The current solution is to ensure the calling thread is the only one holding the USB enumeration lock and prevent other threads from getting refs while a USB device detach is ongoing. This turned out not to be sufficient. To solve this deadlock we could use "destroy_dev_sched()" to schedule the device destruction in the background, but then we don't know when it is safe to free() the private data of the character device. Instead a callback function is executed by the USB explore process to kill off any leftover USB character devices synchronously after the USB device explore code is finished and the USB enumeration lock is no longer locked. This makes porting easier and also ensures us that character devices must eventually go away after a USB device detach. While at it ensure that "flag_iserror" is only written when "priv_mtx" is locked, which is protecting it. MFC after: 5 days Modified: head/sys/dev/usb/controller/usb_controller.c head/sys/dev/usb/usb_bus.h head/sys/dev/usb/usb_dev.c head/sys/dev/usb/usb_device.c head/sys/dev/usb/usb_device.h Modified: head/sys/dev/usb/controller/usb_controller.c ============================================================================== --- head/sys/dev/usb/controller/usb_controller.c Tue Jan 13 16:18:31 2015 (r277135) +++ head/sys/dev/usb/controller/usb_controller.c Tue Jan 13 16:37:43 2015 (r277136) @@ -59,6 +59,7 @@ #include <dev/usb/usb_busdma.h> #include <dev/usb/usb_dynamic.h> #include <dev/usb/usb_device.h> +#include <dev/usb/usb_dev.h> #include <dev/usb/usb_hub.h> #include <dev/usb/usb_controller.h> @@ -219,6 +220,11 @@ usb_detach(device_t dev) usb_proc_mwait(USB_BUS_EXPLORE_PROC(bus), &bus->detach_msg[0], &bus->detach_msg[1]); +#if USB_HAVE_UGEN + /* Wait for cleanup to complete */ + usb_proc_mwait(USB_BUS_EXPLORE_PROC(bus), + &bus->cleanup_msg[0], &bus->cleanup_msg[1]); +#endif USB_BUS_UNLOCK(bus); #if USB_HAVE_PER_BUS_PROCESS @@ -631,6 +637,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) { @@ -813,6 +845,14 @@ usb_attach_sub(device_t dev, struct usb_ 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 + #if USB_HAVE_PER_BUS_PROCESS /* Create USB explore and callback processes */ Modified: head/sys/dev/usb/usb_bus.h ============================================================================== --- head/sys/dev/usb/usb_bus.h Tue Jan 13 16:18:31 2015 (r277135) +++ head/sys/dev/usb/usb_bus.h Tue Jan 13 16:37:43 2015 (r277136) @@ -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. @@ -83,6 +85,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: */ Modified: head/sys/dev/usb/usb_dev.c ============================================================================== --- head/sys/dev/usb/usb_dev.c Tue Jan 13 16:18:31 2015 (r277135) +++ head/sys/dev/usb/usb_dev.c Tue Jan 13 16:37:43 2015 (r277136) @@ -293,8 +293,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"); @@ -365,8 +365,8 @@ usb_unref_device(struct usb_cdev_privdat } 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); } @@ -592,12 +592,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; Modified: head/sys/dev/usb/usb_device.c ============================================================================== --- head/sys/dev/usb/usb_device.c Tue Jan 13 16:18:31 2015 (r277135) +++ head/sys/dev/usb/usb_device.c Tue Jan 13 16:37:43 2015 (r277136) @@ -448,68 +448,29 @@ usb_endpoint_foreach(struct usb_device * return (NULL); } -#if USB_HAVE_UGEN -static uint16_t -usb_get_refcount(struct usb_device *udev) -{ - if (usb_proc_is_called_from(USB_BUS_EXPLORE_PROC(udev->bus)) || - usb_proc_is_called_from(USB_BUS_CONTROL_XFER_PROC(udev->bus))) - 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 } @@ -1186,9 +1147,6 @@ usb_detach_device(struct usb_device *ude 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. @@ -1215,8 +1173,6 @@ usb_detach_device(struct usb_device *ude usb_detach_device_sub(udev, &iface->subdev, &iface->pnpinfo, flag); } - - usb_ref_restore_locked(udev); } /*------------------------------------------------------------------------* @@ -2033,14 +1989,43 @@ usb_make_dev(struct usb_device *udev, co } 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(USB_BUS_EXPLORE_PROC(bus), + &bus->cleanup_msg[0], &bus->cleanup_msg[1]); + USB_BUS_UNLOCK(bus); } static void @@ -2191,6 +2176,9 @@ usb_free_device(struct usb_device *udev, &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); @@ -2676,14 +2664,8 @@ usb_fifo_free_wrap(struct usb_device *ud /* 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 Modified: head/sys/dev/usb/usb_device.h ============================================================================== --- head/sys/dev/usb/usb_device.h Tue Jan 13 16:18:31 2015 (r277135) +++ head/sys/dev/usb/usb_device.h Tue Jan 13 16:37:43 2015 (r277136) @@ -293,6 +293,7 @@ struct usb_device *usb_alloc_device(devi 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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201501131637.t0DGbipG094270>