MFC r305421:

Resolve deadlock between device_detach() and usbd_do_request_flags()
by reviving the SX control request lock and refining which lock
protects the common scratch area in "struct usb_device".

The SX control request lock was removed by r246759 because it caused a
lock order reversal with the USB enumeration lock inside
usbd_transfer_setup() as a function of r246616. It was thought that
reducing the number of locks would resolve the LOR, but because some
USB device drivers use usbd_do_request_flags() inside callback
functions, like in taskqueues, a deadlock may occur when these are
drained from device_detach(). By restoring the SX control request
lock usbd_do_request_flags() is allowed to complete its execution
when a USB device driver is detaching. By using the SX control request
lock to protect the scratch area, the LOR introduced by r246616 is
also resolved.

Bump the FreeBSD version while at it to force recompilation of all USB
kernel modules.

Found by:	avos@

8 files changed, 61 insertions, 28 deletions

diff --git a/sys/dev/usb/template/usb_template.c b/sys/dev/usb/template/usb_template.c
index 9adb30a..9bebbf6 100644
--- a/sys/dev/usb/template/usb_template.c
+++ b/sys/dev/usb/template/usb_template.c
@@ -1245,7 +1245,7 @@ usb_temp_setup(struct usb_device *udev,
 		return (0);
 
 	/* Protect scratch area */
-	do_unlock = usbd_enum_lock(udev);
+	do_unlock = usbd_ctrl_lock(udev);
 
 	uts = udev->scratch.temp_setup;
 
@@ -1324,7 +1324,7 @@ done:
 	if (error)
 		usb_temp_unsetup(udev);
 	if (do_unlock)
-		usbd_enum_unlock(udev);
+		usbd_ctrl_unlock(udev);
 	return (error);
 }
 
diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c
index a5c2444..b9fa7d4 100644
--- a/sys/dev/usb/usb_device.c
+++ b/sys/dev/usb/usb_device.c
@@ -1585,6 +1585,7 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus,
 	/* initialise our SX-lock */
 	sx_init_flags(&udev->enum_sx, "USB config SX lock", SX_DUPOK);
 	sx_init_flags(&udev->sr_sx, "USB suspend and resume SX lock", SX_NOWITNESS);
+	sx_init_flags(&udev->ctrl_sx, "USB control transfer SX lock", SX_DUPOK);
 
 	cv_init(&udev->ctrlreq_cv, "WCTRL");
 	cv_init(&udev->ref_cv, "UGONE");
@@ -1770,7 +1771,7 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus,
 	 */
 
 	/* Protect scratch area */
-	do_unlock = usbd_enum_lock(udev);
+	do_unlock = usbd_ctrl_lock(udev);
 
 	scratch_ptr = udev->scratch.data;
 
@@ -1821,7 +1822,7 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus,
 	}
 
 	if (do_unlock)
-		usbd_enum_unlock(udev);
+		usbd_ctrl_unlock(udev);
 
 	/* assume 100mA bus powered for now. Changed when configured. */
 	udev->power = USB_MIN_POWER;
@@ -2195,6 +2196,7 @@ usb_free_device(struct usb_device *udev, uint8_t flag)
 	
 	sx_destroy(&udev->enum_sx);
 	sx_destroy(&udev->sr_sx);
+	sx_destroy(&udev->ctrl_sx);
 
 	cv_destroy(&udev->ctrlreq_cv);
 	cv_destroy(&udev->ref_cv);
@@ -2358,7 +2360,7 @@ usbd_set_device_strings(struct usb_device *udev)
 	uint8_t do_unlock;
 
 	/* Protect scratch area */
-	do_unlock = usbd_enum_lock(udev);
+	do_unlock = usbd_ctrl_lock(udev);
 
 	temp_ptr = (char *)udev->scratch.data;
 	temp_size = sizeof(udev->scratch.data);
@@ -2418,7 +2420,7 @@ usbd_set_device_strings(struct usb_device *udev)
 	}
 
 	if (do_unlock)
-		usbd_enum_unlock(udev);
+		usbd_ctrl_unlock(udev);
 }
 
 /*
@@ -2825,6 +2827,40 @@ usbd_enum_is_locked(struct usb_device *udev)
 }
 
 /*
+ * The following function is used to serialize access to USB control
+ * transfers and the USB scratch area. If the lock is already grabbed
+ * this function returns zero. Else a value of one is returned.
+ */
+uint8_t
+usbd_ctrl_lock(struct usb_device *udev)
+{
+	if (sx_xlocked(&udev->ctrl_sx))
+		return (0);
+	sx_xlock(&udev->ctrl_sx);
+
+	/*
+	 * We need to allow suspend and resume at this point, else the
+	 * control transfer will timeout if the device is suspended!
+	 */
+	if (usbd_enum_is_locked(udev))
+		usbd_sr_unlock(udev);
+	return (1);
+}
+
+void
+usbd_ctrl_unlock(struct usb_device *udev)
+{
+	sx_xunlock(&udev->ctrl_sx);
+
+	/*
+	 * Restore the suspend and resume lock after we have unlocked
+	 * the USB control transfer lock to avoid LOR:
+	 */
+	if (usbd_enum_is_locked(udev))
+		usbd_sr_lock(udev);
+}
+
+/*
  * The following function is used to set the per-interface specific
  * plug and play information. The string referred to by the pnpinfo
  * argument can safely be freed after calling this function. The
diff --git a/sys/dev/usb/usb_device.h b/sys/dev/usb/usb_device.h
index 24a5a4a..22e1560 100644
--- a/sys/dev/usb/usb_device.h
+++ b/sys/dev/usb/usb_device.h
@@ -162,7 +162,7 @@ struct usb_temp_setup {
 
 /* 
  * The scratch area for USB devices. Access to this structure is
- * protected by the enumeration SX lock.
+ * protected by the control SX lock.
  */
 union usb_device_scratch {
 	struct usb_hw_ep_scratch hw_ep_scratch[1];
@@ -183,6 +183,7 @@ struct usb_device {
 	struct usb_udev_msg cs_msg[2];
 	struct sx enum_sx;
 	struct sx sr_sx;
+  	struct sx ctrl_sx;
 	struct mtx device_mtx;
 	struct cv ctrlreq_cv;
 	struct cv ref_cv;
@@ -320,6 +321,8 @@ uint8_t	usbd_enum_lock_sig(struct usb_device *);
 void	usbd_enum_unlock(struct usb_device *);
 void	usbd_sr_lock(struct usb_device *);
 void	usbd_sr_unlock(struct usb_device *);
+uint8_t	usbd_ctrl_lock(struct usb_device *);
+void	usbd_ctrl_unlock(struct usb_device *);
 uint8_t usbd_enum_is_locked(struct usb_device *);
 
 #if USB_HAVE_TT_SUPPORT
diff --git a/sys/dev/usb/usb_generic.c b/sys/dev/usb/usb_generic.c
index 17956cb..73b62a7 100644
--- a/sys/dev/usb/usb_generic.c
+++ b/sys/dev/usb/usb_generic.c
@@ -714,16 +714,16 @@ ugen_get_cdesc(struct usb_fifo *f, struct usb_gen_descriptor *ugd)
 	return (error);
 }
 
-/*
- * This function is called having the enumeration SX locked which
- * protects the scratch area used.
- */
 static int
 ugen_get_sdesc(struct usb_fifo *f, struct usb_gen_descriptor *ugd)
 {
 	void *ptr;
 	uint16_t size;
 	int error;
+	uint8_t do_unlock;
+
+	/* Protect scratch area */
+	do_unlock = usbd_ctrl_lock(f->udev);
 
 	ptr = f->udev->scratch.data;
 	size = sizeof(f->udev->scratch.data);
@@ -744,6 +744,9 @@ ugen_get_sdesc(struct usb_fifo *f, struct usb_gen_descriptor *ugd)
 
 		error = copyout(ptr, ugd->ugd_data, size);
 	}
+	if (do_unlock)
+		usbd_ctrl_unlock(f->udev);
+
 	return (error);
 }
 
diff --git a/sys/dev/usb/usb_request.c b/sys/dev/usb/usb_request.c
index 634d423..7c02282 100644
--- a/sys/dev/usb/usb_request.c
+++ b/sys/dev/usb/usb_request.c
@@ -460,16 +460,9 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx,
 	}
 
 	/*
-	 * Grab the USB device enumeration SX-lock serialization is
-	 * achieved when multiple threads are involved:
+	 * Serialize access to this function:
 	 */
-	do_unlock = usbd_enum_lock(udev);
-
-	/*
-	 * We need to allow suspend and resume at this point, else the
-	 * control transfer will timeout if the device is suspended!
-	 */
-	usbd_sr_unlock(udev);
+	do_unlock = usbd_ctrl_lock(udev);
 
 	hr_func = usbd_get_hr_func(udev);
 
@@ -713,10 +706,8 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx,
 	USB_XFER_UNLOCK(xfer);
 
 done:
-	usbd_sr_lock(udev);
-
 	if (do_unlock)
-		usbd_enum_unlock(udev);
+		usbd_ctrl_unlock(udev);
 
 	if ((mtx != NULL) && (mtx != &Giant))
 		mtx_lock(mtx);
diff --git a/sys/dev/usb/usb_transfer.c b/sys/dev/usb/usb_transfer.c
index 3c920f9..a2ca28b 100644
--- a/sys/dev/usb/usb_transfer.c
+++ b/sys/dev/usb/usb_transfer.c
@@ -953,7 +953,7 @@ usbd_transfer_setup(struct usb_device *udev,
 		return (error);
 
 	/* Protect scratch area */
-	do_unlock = usbd_enum_lock(udev);
+	do_unlock = usbd_ctrl_lock(udev);
 
 	refcount = 0;
 	info = NULL;
@@ -1274,7 +1274,7 @@ done:
 	error = parm->err;
 
 	if (do_unlock)
-		usbd_enum_unlock(udev);
+		usbd_ctrl_unlock(udev);
 
 	return (error);
 }
diff --git a/sys/dev/usb/usb_util.c b/sys/dev/usb/usb_util.c
index ffd005b..cc9ede1 100644
--- a/sys/dev/usb/usb_util.c
+++ b/sys/dev/usb/usb_util.c
@@ -98,7 +98,7 @@ device_set_usb_desc(device_t dev)
 	}
 
 	/* Protect scratch area */
-	do_unlock = usbd_enum_lock(udev);
+	do_unlock = usbd_ctrl_lock(udev);
 
 	temp_p = (char *)udev->scratch.data;
 
@@ -115,7 +115,7 @@ device_set_usb_desc(device_t dev)
 	}
 
 	if (do_unlock)
-		usbd_enum_unlock(udev);
+		usbd_ctrl_unlock(udev);
 
 	device_set_desc_copy(dev, temp_p);
 	device_printf(dev, "<%s> on %s\n", temp_p,
diff --git a/sys/sys/param.h b/sys/sys/param.h
index b539a62..15b3002 100644
--- a/sys/sys/param.h
+++ b/sys/sys/param.h
@@ -58,7 +58,7 @@
  *		in the range 5 to 9.
  */
 #undef __FreeBSD_version
-#define __FreeBSD_version 1100502	/* Master, propagated to newvers */
+#define __FreeBSD_version 1100503	/* Master, propagated to newvers */
 
 /*
  * __FreeBSD_kernel__ indicates that this system uses the kernel of FreeBSD,