Resolve a LOR after r246616. Protect control requests using the USB device

enumeration lock. Make sure all callers of usbd_enum_lock() check the return
value. Remove the control transfer specific lock. Bump the FreeBSD version
number, hence external USB modules may need to be recompiled due to a USB
device structure change.

MFC after:	1 week

8 files changed, 54 insertions, 42 deletions

diff --git a/sys/dev/usb/controller/usb_controller.c b/sys/dev/usb/controller/usb_controller.c
index cf2ae7a..a28d612 100644
--- a/sys/dev/usb/controller/usb_controller.c
+++ b/sys/dev/usb/controller/usb_controller.c
@@ -427,6 +427,7 @@ usb_bus_suspend(struct usb_proc_msg *pm)
 	struct usb_bus *bus;
 	struct usb_device *udev;
 	usb_error_t err;
+	uint8_t do_unlock;
 
 	bus = ((struct usb_bus_msg *)pm)->bus;
 	udev = bus->devices[USB_ROOT_HUB_ADDR];
@@ -447,7 +448,7 @@ usb_bus_suspend(struct usb_proc_msg *pm)
 
 	bus_generic_shutdown(bus->bdev);
 
-	usbd_enum_lock(udev);
+	do_unlock = usbd_enum_lock(udev);
 
 	err = usbd_set_config_index(udev, USB_UNCONFIG_INDEX);
 	if (err)
@@ -464,7 +465,8 @@ usb_bus_suspend(struct usb_proc_msg *pm)
 	if (bus->methods->set_hw_power_sleep != NULL)
 		(bus->methods->set_hw_power_sleep) (bus, USB_HW_POWER_SUSPEND);
 
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 
 	USB_BUS_LOCK(bus);
 }
@@ -480,6 +482,7 @@ usb_bus_resume(struct usb_proc_msg *pm)
 	struct usb_bus *bus;
 	struct usb_device *udev;
 	usb_error_t err;
+	uint8_t do_unlock;
 
 	bus = ((struct usb_bus_msg *)pm)->bus;
 	udev = bus->devices[USB_ROOT_HUB_ADDR];
@@ -489,7 +492,7 @@ usb_bus_resume(struct usb_proc_msg *pm)
 
 	USB_BUS_UNLOCK(bus);
 
-	usbd_enum_lock(udev);
+	do_unlock = usbd_enum_lock(udev);
 #if 0
 	DEVMETHOD(usb_take_controller, NULL);	/* dummy */
 #endif
@@ -523,7 +526,8 @@ usb_bus_resume(struct usb_proc_msg *pm)
 		    "attach root HUB\n");
 	}
 
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 
 	USB_BUS_LOCK(bus);
 }
@@ -539,6 +543,7 @@ usb_bus_shutdown(struct usb_proc_msg *pm)
 	struct usb_bus *bus;
 	struct usb_device *udev;
 	usb_error_t err;
+	uint8_t do_unlock;
 
 	bus = ((struct usb_bus_msg *)pm)->bus;
 	udev = bus->devices[USB_ROOT_HUB_ADDR];
@@ -550,7 +555,7 @@ usb_bus_shutdown(struct usb_proc_msg *pm)
 
 	bus_generic_shutdown(bus->bdev);
 
-	usbd_enum_lock(udev);
+	do_unlock = usbd_enum_lock(udev);
 
 	err = usbd_set_config_index(udev, USB_UNCONFIG_INDEX);
 	if (err)
@@ -567,7 +572,8 @@ usb_bus_shutdown(struct usb_proc_msg *pm)
 	if (bus->methods->set_hw_power_sleep != NULL)
 		(bus->methods->set_hw_power_sleep) (bus, USB_HW_POWER_SHUTDOWN);
 
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 
 	USB_BUS_LOCK(bus);
 }
diff --git a/sys/dev/usb/usb_dev.c b/sys/dev/usb/usb_dev.c
index c4264c5..9bf430b 100644
--- a/sys/dev/usb/usb_dev.c
+++ b/sys/dev/usb/usb_dev.c
@@ -218,10 +218,10 @@ usb_ref_device(struct usb_cdev_privdata *cpd,
 		mtx_unlock(&usb_ref_lock);
 
 		/*
-		 * We need to grab the sx-lock before grabbing the
-		 * FIFO refs to avoid deadlock at detach!
+		 * We need to grab the enumeration SX-lock before
+		 * grabbing the FIFO refs to avoid deadlock at detach!
 		 */
-		usbd_enum_lock(cpd->udev);
+		crd->do_unlock = usbd_enum_lock(cpd->udev);
 
 		mtx_lock(&usb_ref_lock);
 
@@ -282,9 +282,10 @@ usb_ref_device(struct usb_cdev_privdata *cpd,
 	return (0);
 
 error:
-	if (crd->is_uref) {
+	if (crd->do_unlock)
 		usbd_enum_unlock(cpd->udev);
 
+	if (crd->is_uref) {
 		if (--(cpd->udev->refcount) == 0) {
 			cv_signal(&cpd->udev->ref_cv);
 		}
@@ -336,7 +337,7 @@ usb_unref_device(struct usb_cdev_privdata *cpd,
 
 	DPRINTFN(2, "cpd=%p is_uref=%d\n", cpd, crd->is_uref);
 
-	if (crd->is_uref)
+	if (crd->do_unlock)
 		usbd_enum_unlock(cpd->udev);
 
 	mtx_lock(&usb_ref_lock);
diff --git a/sys/dev/usb/usb_dev.h b/sys/dev/usb/usb_dev.h
index 3c75701..f87ba53 100644
--- a/sys/dev/usb/usb_dev.h
+++ b/sys/dev/usb/usb_dev.h
@@ -84,6 +84,7 @@ struct usb_cdev_refdata {
 	uint8_t			is_write;	/* location has write access */
 	uint8_t			is_uref;	/* USB refcount decr. needed */
 	uint8_t			is_usbfs;	/* USB-FS is active */
+	uint8_t			do_unlock;	/* USB enum unlock needed */
 };
 
 struct usb_fs_privdata {
diff --git a/sys/dev/usb/usb_device.c b/sys/dev/usb/usb_device.c
index fdc734c..1c40815 100644
--- a/sys/dev/usb/usb_device.c
+++ b/sys/dev/usb/usb_device.c
@@ -1546,9 +1546,6 @@ usb_alloc_device(device_t parent_dev, struct usb_bus *bus,
 		return (NULL);
 	}
 	/* initialise our SX-lock */
-	sx_init_flags(&udev->ctrl_sx, "USB device SX lock", SX_DUPOK);
-
-	/* 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);
 
@@ -2119,7 +2116,6 @@ usb_free_device(struct usb_device *udev, uint8_t flag)
 	    &udev->cs_msg[0], &udev->cs_msg[1]);
 	USB_BUS_UNLOCK(udev->bus);
 
-	sx_destroy(&udev->ctrl_sx);
 	sx_destroy(&udev->enum_sx);
 	sx_destroy(&udev->sr_sx);
 
diff --git a/sys/dev/usb/usb_device.h b/sys/dev/usb/usb_device.h
index 22a4652..b194256 100644
--- a/sys/dev/usb/usb_device.h
+++ b/sys/dev/usb/usb_device.h
@@ -181,7 +181,6 @@ union usb_device_scratch {
 struct usb_device {
 	struct usb_clear_stall_msg cs_msg[2];	/* generic clear stall
 						 * messages */
-	struct sx ctrl_sx;
 	struct sx enum_sx;
 	struct sx sr_sx;
 	struct mtx device_mtx;
diff --git a/sys/dev/usb/usb_handle_request.c b/sys/dev/usb/usb_handle_request.c
index a775d67..7f3cdc6 100644
--- a/sys/dev/usb/usb_handle_request.c
+++ b/sys/dev/usb/usb_handle_request.c
@@ -149,6 +149,7 @@ usb_handle_set_config(struct usb_xfer *xfer, uint8_t conf_no)
 {
 	struct usb_device *udev = xfer->xroot->udev;
 	usb_error_t err = 0;
+	uint8_t do_unlock;
 
 	/*
 	 * We need to protect against other threads doing probe and
@@ -156,7 +157,8 @@ usb_handle_set_config(struct usb_xfer *xfer, uint8_t conf_no)
 	 */
 	USB_XFER_UNLOCK(xfer);
 
-	usbd_enum_lock(udev);
+	/* Prevent re-enumeration */
+	do_unlock = usbd_enum_lock(udev);
 
 	if (conf_no == USB_UNCONFIG_NO) {
 		conf_no = USB_UNCONFIG_INDEX;
@@ -179,7 +181,8 @@ usb_handle_set_config(struct usb_xfer *xfer, uint8_t conf_no)
 		goto done;
 	}
 done:
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 	USB_XFER_LOCK(xfer);
 	return (err);
 }
@@ -221,6 +224,7 @@ usb_handle_iface_request(struct usb_xfer *xfer,
 	int error;
 	uint8_t iface_index;
 	uint8_t temp_state;
+	uint8_t do_unlock;
 
 	if ((req.bmRequestType & 0x1F) == UT_INTERFACE) {
 		iface_index = req.wIndex[0];	/* unicast */
@@ -234,7 +238,8 @@ usb_handle_iface_request(struct usb_xfer *xfer,
 	 */
 	USB_XFER_UNLOCK(xfer);
 
-	usbd_enum_lock(udev);
+	/* Prevent re-enumeration */
+	do_unlock = usbd_enum_lock(udev);
 
 	error = ENXIO;
 
@@ -350,17 +355,20 @@ tr_repeat:
 		goto tr_stalled;
 	}
 tr_valid:
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 	USB_XFER_LOCK(xfer);
 	return (0);
 
 tr_short:
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 	USB_XFER_LOCK(xfer);
 	return (USB_ERR_SHORT_XFER);
 
 tr_stalled:
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 	USB_XFER_LOCK(xfer);
 	return (USB_ERR_STALLED);
 }
diff --git a/sys/dev/usb/usb_hub.c b/sys/dev/usb/usb_hub.c
index 1d87a3f..a359ee2 100644
--- a/sys/dev/usb/usb_hub.c
+++ b/sys/dev/usb/usb_hub.c
@@ -243,7 +243,9 @@ uhub_explore_sub(struct uhub_softc *sc, struct usb_port *up)
 	/* check if device should be re-enumerated */
 
 	if (child->flags.usb_mode == USB_MODE_HOST) {
-		usbd_enum_lock(child);
+		uint8_t do_unlock;
+		
+		do_unlock = usbd_enum_lock(child);
 		if (child->re_enumerate_wait) {
 			err = usbd_set_config_index(child,
 			    USB_UNCONFIG_INDEX);
@@ -262,7 +264,8 @@ uhub_explore_sub(struct uhub_softc *sc, struct usb_port *up)
 			child->re_enumerate_wait = 0;
 			err = 0;
 		}
-		usbd_enum_unlock(child);
+		if (do_unlock)
+			usbd_enum_unlock(child);
 	}
 
 	/* check if probe and attach should be done */
@@ -716,6 +719,7 @@ uhub_explore(struct usb_device *udev)
 	usb_error_t err;
 	uint8_t portno;
 	uint8_t x;
+	uint8_t do_unlock;
 
 	hub = udev->hub;
 	sc = hub->hubsoftc;
@@ -737,7 +741,7 @@ uhub_explore(struct usb_device *udev)
 	 * Make sure we don't race against user-space applications
 	 * like LibUSB:
 	 */
-	usbd_enum_lock(udev);
+	do_unlock = usbd_enum_lock(udev);
 
 	for (x = 0; x != hub->nports; x++) {
 		up = hub->ports + x;
@@ -817,7 +821,8 @@ uhub_explore(struct usb_device *udev)
 		up->restartcnt = 0;
 	}
 
-	usbd_enum_unlock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 
 	/* initial status checked */
 	sc->sc_flags |= UHUB_FLAG_DID_EXPLORE;
diff --git a/sys/dev/usb/usb_request.c b/sys/dev/usb/usb_request.c
index 4db5b7d..1a366d6 100644
--- a/sys/dev/usb/usb_request.c
+++ b/sys/dev/usb/usb_request.c
@@ -389,9 +389,8 @@ usbd_get_hr_func(struct usb_device *udev)
  * than 30 seconds is treated like a 30 second timeout. This USB stack
  * does not allow control requests without a timeout.
  *
- * NOTE: This function is thread safe. All calls to
- * "usbd_do_request_flags" will be serialised by the use of an
- * internal "sx_lock".
+ * NOTE: This function is thread safe. All calls to "usbd_do_request_flags"
+ * will be serialized by the use of the USB device enumeration lock.
  *
  * Returns:
  *    0: Success
@@ -415,7 +414,7 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx,
 	uint16_t length;
 	uint16_t temp;
 	uint16_t acttemp;
-	uint8_t enum_locked;
+	uint8_t do_unlock;
 
 	if (timeout < 50) {
 		/* timeout is too small */
@@ -427,8 +426,6 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx,
 	}
 	length = UGETW(req->wLength);
 
-	enum_locked = usbd_enum_is_locked(udev);
-
 	DPRINTFN(5, "udev=%p bmRequestType=0x%02x bRequest=0x%02x "
 	    "wValue=0x%02x%02x wIndex=0x%02x%02x wLength=0x%02x%02x\n",
 	    udev, req->bmRequestType, req->bRequest,
@@ -459,17 +456,16 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx,
 	}
 
 	/*
-	 * We need to allow suspend and resume at this point, else the
-	 * control transfer will timeout if the device is suspended!
+	 * Grab the USB device enumeration SX-lock serialization is
+	 * achieved when multiple threads are involved:
 	 */
-	if (enum_locked)
-		usbd_sr_unlock(udev);
+	do_unlock = usbd_enum_lock(udev);
 
 	/*
-	 * Grab the default sx-lock so that serialisation
-	 * is achieved when multiple threads are involved:
+	 * We need to allow suspend and resume at this point, else the
+	 * control transfer will timeout if the device is suspended!
 	 */
-	sx_xlock(&udev->ctrl_sx);
+	usbd_sr_unlock(udev);
 
 	hr_func = usbd_get_hr_func(udev);
 
@@ -713,10 +709,10 @@ usbd_do_request_flags(struct usb_device *udev, struct mtx *mtx,
 	USB_XFER_UNLOCK(xfer);
 
 done:
-	sx_xunlock(&udev->ctrl_sx);
+	usbd_sr_lock(udev);
 
-	if (enum_locked)
-		usbd_sr_lock(udev);
+	if (do_unlock)
+		usbd_enum_unlock(udev);
 
 	if ((mtx != NULL) && (mtx != &Giant))
 		mtx_lock(mtx);