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.

5 files changed, 98 insertions, 66 deletions

diff --git a/sys/dev/usb/controller/usb_controller.c b/sys/dev/usb/controller/usb_controller.c
index 5addc9d..f67c94d 100644
--- a/sys/dev/usb/controller/usb_controller.c
+++ b/sys/dev/usb/controller/usb_controller.c
@@ -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>
@@ -221,6 +222,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
@@ -633,6 +639,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)
 {
@@ -815,6 +847,14 @@ 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
+
 #if USB_HAVE_PER_BUS_PROCESS
 	/* Create USB explore and callback processes */
 
diff --git a/sys/dev/usb/usb_bus.h b/sys/dev/usb/usb_bus.h
index d00d10a1..afc20f4 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.
@@ -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:
 	 */
diff --git a/sys/dev/usb/usb_dev.c b/sys/dev/usb/usb_dev.c
index ddc6472..92f8b6b 100644
--- a/sys/dev/usb/usb_dev.c
+++ b/sys/dev/usb/usb_dev.c
@@ -294,8 +294,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");
@@ -366,8 +366,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);
 }
@@ -593,12 +593,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 2ce4e23..44314aa 100644
--- a/sys/dev/usb/usb_device.c
+++ b/sys/dev/usb/usb_device.c
@@ -451,68 +451,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(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
 }
@@ -1189,9 +1150,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.
@@ -1218,8 +1176,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);
 }
 
 /*------------------------------------------------------------------------*
@@ -2036,14 +1992,46 @@ usb_make_dev(struct usb_device *udev, const char *devname, int ep,
 }
 
 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(USB_BUS_EXPLORE_PROC(bus),
+	    &bus->cleanup_msg[0], &bus->cleanup_msg[1]);
+	USB_BUS_UNLOCK(bus);
 }
 
 static void
@@ -2194,6 +2182,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);
 
@@ -2679,14 +2670,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 6bc3980..5a94998 100644
--- a/sys/dev/usb/usb_device.h
+++ b/sys/dev/usb/usb_device.h
@@ -293,6 +293,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);