From owner-svn-src-stable-9@FreeBSD.ORG Thu Feb 5 21:38:01 2015 Return-Path: Delivered-To: svn-src-stable-9@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 44A5D7FA; Thu, 5 Feb 2015 21:38:01 +0000 (UTC) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:1900:2254:2068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 2530CF8F; Thu, 5 Feb 2015 21:38:01 +0000 (UTC) Received: from svn.freebsd.org ([127.0.1.70]) by svn.freebsd.org (8.14.9/8.14.9) with ESMTP id t15Lc1sQ063328; Thu, 5 Feb 2015 21:38:01 GMT (envelope-from hselasky@FreeBSD.org) Received: (from hselasky@localhost) by svn.freebsd.org (8.14.9/8.14.9/Submit) id t15Lbx4v063313; Thu, 5 Feb 2015 21:37:59 GMT (envelope-from hselasky@FreeBSD.org) Message-Id: <201502052137.t15Lbx4v063313@svn.freebsd.org> X-Authentication-Warning: svn.freebsd.org: hselasky set sender to hselasky@FreeBSD.org using -f From: Hans Petter Selasky Date: Thu, 5 Feb 2015 21:37:59 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-9@freebsd.org Subject: svn commit: r278291 - in stable/9/sys/dev/usb: . controller X-SVN-Group: stable-9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable-9@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for only the 9-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Feb 2015 21:38:01 -0000 Author: hselasky Date: Thu Feb 5 21:37:59 2015 New Revision: 278291 URL: https://svnweb.freebsd.org/changeset/base/278291 Log: 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. Modified: stable/9/sys/dev/usb/controller/usb_controller.c stable/9/sys/dev/usb/usb_bus.h stable/9/sys/dev/usb/usb_dev.c stable/9/sys/dev/usb/usb_device.c stable/9/sys/dev/usb/usb_device.h Directory Properties: stable/9/sys/ (props changed) stable/9/sys/dev/ (props changed) Modified: stable/9/sys/dev/usb/controller/usb_controller.c ============================================================================== --- stable/9/sys/dev/usb/controller/usb_controller.c Thu Feb 5 21:25:11 2015 (r278290) +++ stable/9/sys/dev/usb/controller/usb_controller.c Thu Feb 5 21:37:59 2015 (r278291) @@ -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 */ @@ -613,6 +619,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) { @@ -793,6 +825,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 + /* Create USB explore and callback processes */ if (usb_proc_create(&bus->giant_callback_proc, Modified: stable/9/sys/dev/usb/usb_bus.h ============================================================================== --- stable/9/sys/dev/usb/usb_bus.h Thu Feb 5 21:25:11 2015 (r278290) +++ stable/9/sys/dev/usb/usb_bus.h Thu Feb 5 21:37:59 2015 (r278291) @@ -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: */ Modified: stable/9/sys/dev/usb/usb_dev.c ============================================================================== --- stable/9/sys/dev/usb/usb_dev.c Thu Feb 5 21:25:11 2015 (r278290) +++ stable/9/sys/dev/usb/usb_dev.c Thu Feb 5 21:37:59 2015 (r278291) @@ -290,8 +290,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"); @@ -362,8 +362,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); } @@ -589,12 +589,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: stable/9/sys/dev/usb/usb_device.c ============================================================================== --- stable/9/sys/dev/usb/usb_device.c Thu Feb 5 21:25:11 2015 (r278290) +++ stable/9/sys/dev/usb/usb_device.c Thu Feb 5 21:37:59 2015 (r278291) @@ -430,68 +430,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(&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 } @@ -1156,9 +1117,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. @@ -1185,8 +1143,6 @@ usb_detach_device(struct usb_device *ude usb_detach_device_sub(udev, &iface->subdev, &iface->pnpinfo, flag); } - - usb_ref_restore_locked(udev); } /*------------------------------------------------------------------------* @@ -1999,14 +1955,46 @@ 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; + } + + /* make sure we can re-use the device name */ + delist_dev(pd->cdev); + + 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 @@ -2157,6 +2145,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); @@ -2642,14 +2633,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: stable/9/sys/dev/usb/usb_device.h ============================================================================== --- stable/9/sys/dev/usb/usb_device.h Thu Feb 5 21:25:11 2015 (r278290) +++ stable/9/sys/dev/usb/usb_device.h Thu Feb 5 21:37:59 2015 (r278291) @@ -281,6 +281,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);