Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 17 May 2010 23:45:32 +0000 (UTC)
From:      Andrew Thompson <thompsa@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r208221 - stable/8/sys/dev/usb
Message-ID:  <201005172345.o4HNjW2r074524@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: thompsa
Date: Mon May 17 23:45:31 2010
New Revision: 208221
URL: http://svn.freebsd.org/changeset/base/208221

Log:
  MFC r208008
  
   If a USB device is suspended and a USB set config request is issued when the
   USB enumeration lock is locked, then the USB stack fails to resume the device
   because locking the USB enumeration lock is part of the resume procedure. To
   solve this issue a new lock is introduced which only protects the suspend and
   resume callbacks, which can be dropped inside the usbd_do_request_flags()
   function, to allow suspend and resume during so-called enumeration operations.

Modified:
  stable/8/sys/dev/usb/usb_device.c
  stable/8/sys/dev/usb/usb_device.h
  stable/8/sys/dev/usb/usb_generic.c
  stable/8/sys/dev/usb/usb_hub.c
  stable/8/sys/dev/usb/usb_request.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/xen/xenpci/   (props changed)
  stable/8/sys/geom/sched/   (props changed)

Modified: stable/8/sys/dev/usb/usb_device.c
==============================================================================
--- stable/8/sys/dev/usb/usb_device.c	Mon May 17 23:44:52 2010	(r208220)
+++ stable/8/sys/dev/usb/usb_device.c	Mon May 17 23:45:31 2010	(r208221)
@@ -1380,7 +1380,7 @@ usb_suspend_resume(struct usb_device *ud
 	}
 	DPRINTFN(4, "udev=%p do_suspend=%d\n", udev, do_suspend);
 
-	sx_assert(&udev->enum_sx, SA_LOCKED);
+	sx_assert(&udev->sr_sx, SA_LOCKED);
 
 	USB_BUS_LOCK(udev->bus);
 	/* filter the suspend events */
@@ -1495,6 +1495,7 @@ usb_alloc_device(device_t parent_dev, st
 
 	/* 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_DUPOK);
 
 	cv_init(&udev->ctrlreq_cv, "WCTRL");
 	cv_init(&udev->ref_cv, "UGONE");
@@ -2038,6 +2039,7 @@ usb_free_device(struct usb_device *udev,
 
 	sx_destroy(&udev->ctrl_sx);
 	sx_destroy(&udev->enum_sx);
+	sx_destroy(&udev->sr_sx);
 
 	cv_destroy(&udev->ctrlreq_cv);
 	cv_destroy(&udev->ref_cv);
@@ -2188,12 +2190,12 @@ usbd_set_device_strings(struct usb_devic
 #ifdef USB_VERBOSE
 	const struct usb_knowndev *kdp;
 #endif
-	uint8_t *temp_ptr;
+	char *temp_ptr;
 	size_t temp_size;
 	uint16_t vendor_id;
 	uint16_t product_id;
 
-	temp_ptr = udev->bus->scratch[0].data;
+	temp_ptr = (char *)udev->bus->scratch[0].data;
 	temp_size = sizeof(udev->bus->scratch[0].data);
 
 	vendor_id = UGETW(udd->idVendor);
@@ -2589,6 +2591,7 @@ void
 usbd_enum_lock(struct usb_device *udev)
 {
 	sx_xlock(&udev->enum_sx);
+	sx_xlock(&udev->sr_sx);
 	/* 
 	 * NEWBUS LOCK NOTE: We should check if any parent SX locks
 	 * are locked before locking Giant. Else the lock can be
@@ -2604,6 +2607,30 @@ usbd_enum_unlock(struct usb_device *udev
 {
 	mtx_unlock(&Giant);
 	sx_xunlock(&udev->enum_sx);
+	sx_xunlock(&udev->sr_sx);
+}
+
+/* The following function locks suspend and resume. */
+
+void
+usbd_sr_lock(struct usb_device *udev)
+{
+	sx_xlock(&udev->sr_sx);
+	/* 
+	 * NEWBUS LOCK NOTE: We should check if any parent SX locks
+	 * are locked before locking Giant. Else the lock can be
+	 * locked multiple times.
+	 */
+	mtx_lock(&Giant);
+}
+
+/* The following function unlocks suspend and resume. */
+
+void
+usbd_sr_unlock(struct usb_device *udev)
+{
+	mtx_unlock(&Giant);
+	sx_xunlock(&udev->sr_sx);
 }
 
 /*

Modified: stable/8/sys/dev/usb/usb_device.h
==============================================================================
--- stable/8/sys/dev/usb/usb_device.h	Mon May 17 23:44:52 2010	(r208220)
+++ stable/8/sys/dev/usb/usb_device.h	Mon May 17 23:45:31 2010	(r208221)
@@ -115,6 +115,7 @@ struct usb_device {
 						 * messages */
 	struct sx ctrl_sx;
 	struct sx enum_sx;
+	struct sx sr_sx;
 	struct mtx device_mtx;
 	struct cv ctrlreq_cv;
 	struct cv ref_cv;
@@ -215,6 +216,8 @@ void	usb_set_device_state(struct usb_dev
 	    enum usb_dev_state state);
 void	usbd_enum_lock(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_enum_is_locked(struct usb_device *);
 
 #endif					/* _USB_DEVICE_H_ */

Modified: stable/8/sys/dev/usb/usb_generic.c
==============================================================================
--- stable/8/sys/dev/usb/usb_generic.c	Mon May 17 23:44:52 2010	(r208220)
+++ stable/8/sys/dev/usb/usb_generic.c	Mon May 17 23:45:31 2010	(r208221)
@@ -1735,14 +1735,34 @@ ugen_set_power_mode(struct usb_fifo *f, 
 		break;
 
 	case USB_POWER_MODE_RESUME:
-		err = usbd_req_clear_port_feature(udev->parent_hub,
-		    NULL, udev->port_no, UHF_PORT_SUSPEND);
+#if USB_HAVE_POWERD
+		/* let USB-powerd handle resume */
+		USB_BUS_LOCK(udev->bus);
+		udev->pwr_save.write_refs++;
+		udev->pwr_save.last_xfer_time = ticks;
+		USB_BUS_UNLOCK(udev->bus);
+
+		/* set new power mode */
+		usbd_set_power_mode(udev, USB_POWER_MODE_SAVE);
+
+		/* wait for resume to complete */
+		usb_pause_mtx(NULL, hz / 4);
+
+		/* clear write reference */
+		USB_BUS_LOCK(udev->bus);
+		udev->pwr_save.write_refs--;
+		USB_BUS_UNLOCK(udev->bus);
+#endif
 		mode = USB_POWER_MODE_SAVE;
 		break;
 
 	case USB_POWER_MODE_SUSPEND:
-		err = usbd_req_set_port_feature(udev->parent_hub,
-		    NULL, udev->port_no, UHF_PORT_SUSPEND);
+#if USB_HAVE_POWERD
+		/* let USB-powerd handle suspend */
+		USB_BUS_LOCK(udev->bus);
+		udev->pwr_save.last_xfer_time = ticks - (256 * hz);
+		USB_BUS_UNLOCK(udev->bus);
+#endif
 		mode = USB_POWER_MODE_SAVE;
 		break;
 

Modified: stable/8/sys/dev/usb/usb_hub.c
==============================================================================
--- stable/8/sys/dev/usb/usb_hub.c	Mon May 17 23:44:52 2010	(r208220)
+++ stable/8/sys/dev/usb/usb_hub.c	Mon May 17 23:45:31 2010	(r208221)
@@ -126,6 +126,7 @@ static usb_callback_t uhub_intr_callback
 
 static void usb_dev_resume_peer(struct usb_device *udev);
 static void usb_dev_suspend_peer(struct usb_device *udev);
+static uint8_t usb_peer_should_wakeup(struct usb_device *udev);
 
 static const struct usb_config uhub_config[UHUB_N_TRANSFER] = {
 
@@ -1706,8 +1707,8 @@ usbd_transfer_power_ref(struct usb_xfer 
 		udev->pwr_save.read_refs += val;
 		if (xfer->flags_int.usb_mode == USB_MODE_HOST) {
 			/*
-			 * it is not allowed to suspend during a control
-			 * transfer
+			 * It is not allowed to suspend during a
+			 * control transfer:
 			 */
 			udev->pwr_save.write_refs += val;
 		}
@@ -1717,19 +1718,21 @@ usbd_transfer_power_ref(struct usb_xfer 
 		udev->pwr_save.write_refs += val;
 	}
 
-	if (udev->flags.self_suspended)
-		needs_explore =
-		    (udev->pwr_save.write_refs != 0) ||
-		    ((udev->pwr_save.read_refs != 0) &&
-		    (usb_peer_can_wakeup(udev) == 0));
-	else
-		needs_explore = 0;
+	if (val > 0) {
+		if (udev->flags.self_suspended)
+			needs_explore = usb_peer_should_wakeup(udev);
+		else
+			needs_explore = 0;
 
-	if (!(udev->bus->hw_power_state & power_mask[xfer_type])) {
-		DPRINTF("Adding type %u to power state\n", xfer_type);
-		udev->bus->hw_power_state |= power_mask[xfer_type];
-		needs_hw_power = 1;
+		if (!(udev->bus->hw_power_state & power_mask[xfer_type])) {
+			DPRINTF("Adding type %u to power state\n", xfer_type);
+			udev->bus->hw_power_state |= power_mask[xfer_type];
+			needs_hw_power = 1;
+		} else {
+			needs_hw_power = 0;
+		}
 	} else {
+		needs_explore = 0;
 		needs_hw_power = 0;
 	}
 
@@ -1748,6 +1751,22 @@ usbd_transfer_power_ref(struct usb_xfer 
 #endif
 
 /*------------------------------------------------------------------------*
+ *	usb_peer_should_wakeup
+ *
+ * This function returns non-zero if the current device should wake up.
+ *------------------------------------------------------------------------*/
+static uint8_t
+usb_peer_should_wakeup(struct usb_device *udev)
+{
+	return ((udev->power_mode == USB_POWER_MODE_ON) ||
+	    (udev->pwr_save.type_refs[UE_ISOCHRONOUS] != 0) ||
+	    (udev->pwr_save.write_refs != 0) ||
+	    ((udev->pwr_save.read_refs != 0) &&
+	    (udev->flags.usb_mode == USB_MODE_HOST) &&
+	    (usb_peer_can_wakeup(udev) == 0)));
+}
+
+/*------------------------------------------------------------------------*
  *	usb_bus_powerd
  *
  * This function implements the USB power daemon and is called
@@ -1763,7 +1782,6 @@ usb_bus_powerd(struct usb_bus *bus)
 	usb_ticks_t mintime;
 	usb_size_t type_refs[5];
 	uint8_t x;
-	uint8_t rem_wakeup;
 
 	limit = usb_power_timeout;
 	if (limit == 0)
@@ -1788,30 +1806,23 @@ usb_bus_powerd(struct usb_bus *bus)
 		if (udev == NULL)
 			continue;
 
-		rem_wakeup = usb_peer_can_wakeup(udev);
-
 		temp = ticks - udev->pwr_save.last_xfer_time;
 
-		if ((udev->power_mode == USB_POWER_MODE_ON) ||
-		    (udev->pwr_save.type_refs[UE_ISOCHRONOUS] != 0) ||
-		    (udev->pwr_save.write_refs != 0) ||
-		    ((udev->pwr_save.read_refs != 0) &&
-		    (rem_wakeup == 0))) {
-
+		if (usb_peer_should_wakeup(udev)) {
 			/* check if we are suspended */
 			if (udev->flags.self_suspended != 0) {
 				USB_BUS_UNLOCK(bus);
 				usb_dev_resume_peer(udev);
 				USB_BUS_LOCK(bus);
 			}
-		} else if (temp >= limit) {
-
-			/* check if we are not suspended */
-			if (udev->flags.self_suspended == 0) {
-				USB_BUS_UNLOCK(bus);
-				usb_dev_suspend_peer(udev);
-				USB_BUS_LOCK(bus);
-			}
+		} else if ((temp >= limit) &&
+		    (udev->flags.usb_mode == USB_MODE_HOST) &&
+		    (udev->flags.self_suspended == 0)) {
+			/* try to do suspend */
+
+			USB_BUS_UNLOCK(bus);
+			usb_dev_suspend_peer(udev);
+			USB_BUS_LOCK(bus);
 		}
 	}
 
@@ -1920,6 +1931,9 @@ usb_dev_resume_peer(struct usb_device *u
 	/* resume parent hub first */
 	usb_dev_resume_peer(udev->parent_hub);
 
+	/* reduce chance of instant resume failure by waiting a little bit */
+	usb_pause_mtx(NULL, USB_MS_TO_TICKS(20));
+
 	/* resume current port (Valid in Host and Device Mode) */
 	err = usbd_req_clear_port_feature(udev->parent_hub,
 	    NULL, udev->port_no, UHF_PORT_SUSPEND);
@@ -1958,12 +1972,12 @@ usb_dev_resume_peer(struct usb_device *u
 		(bus->methods->set_hw_power) (bus);
 	}
 
-	usbd_enum_lock(udev);
+	usbd_sr_lock(udev);
 
 	/* notify all sub-devices about resume */
 	err = usb_suspend_resume(udev, 0);
 
-	usbd_enum_unlock(udev);
+	usbd_sr_unlock(udev);
 
 	/* check if peer has wakeup capability */
 	if (usb_peer_can_wakeup(udev)) {
@@ -2029,12 +2043,47 @@ repeat:
 		}
 	}
 
-	usbd_enum_lock(udev);
+	USB_BUS_LOCK(udev->bus);
+	/*
+	 * Checking for suspend condition and setting suspended bit
+	 * must be atomic!
+	 */
+	err = usb_peer_should_wakeup(udev);
+	if (err == 0) {
+		/*
+		 * Set that this device is suspended. This variable
+		 * must be set before calling USB controller suspend
+		 * callbacks.
+		 */
+		udev->flags.self_suspended = 1;
+	}
+	USB_BUS_UNLOCK(udev->bus);
+
+	if (err != 0) {
+		if (udev->flags.usb_mode == USB_MODE_DEVICE) {
+			/* resume parent HUB first */
+			usb_dev_resume_peer(udev->parent_hub);
+
+			/* reduce chance of instant resume failure by waiting a little bit */
+			usb_pause_mtx(NULL, USB_MS_TO_TICKS(20));
+
+			/* resume current port (Valid in Host and Device Mode) */
+			err = usbd_req_clear_port_feature(udev->parent_hub,
+			    NULL, udev->port_no, UHF_PORT_SUSPEND);
+
+			/* resume settle time */
+			usb_pause_mtx(NULL, USB_MS_TO_TICKS(USB_PORT_RESUME_DELAY));
+		}
+		DPRINTF("Suspend was cancelled!\n");
+		return;
+	}
+
+	usbd_sr_lock(udev);
 
 	/* notify all sub-devices about suspend */
 	err = usb_suspend_resume(udev, 1);
 
-	usbd_enum_unlock(udev);
+	usbd_sr_unlock(udev);
 
 	if (usb_peer_can_wakeup(udev)) {
 		/* allow device to do remote wakeup */
@@ -2045,13 +2094,6 @@ repeat:
 			    "remote wakeup failed\n");
 		}
 	}
-	USB_BUS_LOCK(udev->bus);
-	/*
-	 * Set that this device is suspended. This variable must be set
-	 * before calling USB controller suspend callbacks.
-	 */
-	udev->flags.self_suspended = 1;
-	USB_BUS_UNLOCK(udev->bus);
 
 	if (udev->bus->methods->device_suspend != NULL) {
 		usb_timeout_t temp;

Modified: stable/8/sys/dev/usb/usb_request.c
==============================================================================
--- stable/8/sys/dev/usb/usb_request.c	Mon May 17 23:44:52 2010	(r208220)
+++ stable/8/sys/dev/usb/usb_request.c	Mon May 17 23:45:31 2010	(r208221)
@@ -273,6 +273,7 @@ usbd_do_request_flags(struct usb_device 
 	usb_ticks_t max_ticks;
 	uint16_t length;
 	uint16_t temp;
+	uint8_t enum_locked;
 
 	if (timeout < 50) {
 		/* timeout is too small */
@@ -284,6 +285,8 @@ usbd_do_request_flags(struct usb_device 
 	}
 	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,
@@ -308,12 +311,18 @@ usbd_do_request_flags(struct usb_device 
 	if (flags & USB_USER_DATA_PTR)
 		return (USB_ERR_INVAL);
 #endif
-	if (mtx) {
+	if ((mtx != NULL) && (mtx != &Giant)) {
 		mtx_unlock(mtx);
-		if (mtx != &Giant) {
-			mtx_assert(mtx, MA_NOTOWNED);
-		}
+		mtx_assert(mtx, MA_NOTOWNED);
 	}
+
+	/*
+	 * We need to allow suspend and resume at this point, else the
+	 * control transfer will timeout if the device is suspended!
+	 */
+	if (enum_locked)
+		usbd_sr_unlock(udev);
+
 	/*
 	 * Grab the default sx-lock so that serialisation
 	 * is achieved when multiple threads are involved:
@@ -536,9 +545,12 @@ usbd_do_request_flags(struct usb_device 
 done:
 	sx_xunlock(&udev->ctrl_sx);
 
-	if (mtx) {
+	if (enum_locked)
+		usbd_sr_lock(udev);
+
+	if ((mtx != NULL) && (mtx != &Giant))
 		mtx_lock(mtx);
-	}
+
 	return ((usb_error_t)err);
 }
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201005172345.o4HNjW2r074524>