PERFORCE change 177195 for review
Hans Petter Selasky
hselasky at FreeBSD.org
Wed Apr 21 20:37:53 UTC 2010
http://p4web.freebsd.org/@@177195?ac=10
Change 177195 by hselasky at hselasky_laptop001 on 2010/04/21 20:37:28
USB CORE:
- fix a non-critical deadlock issue related to USB power save.
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.
(Attach/detach/set-config/set-alternate-index and more)
- improve suspend and resume IOCTL's so that these are
handled by the USB power daemon and do not directly write
to the USB port registers.
- if the power-save refcount is going down, we should not wake up or
set any USB power bits in the bus structure.
Found and patched by: HPS @
Affected files ...
.. //depot/projects/usb/src/sys/dev/usb/usb_device.c#65 edit
.. //depot/projects/usb/src/sys/dev/usb/usb_device.h#34 edit
.. //depot/projects/usb/src/sys/dev/usb/usb_generic.c#27 edit
.. //depot/projects/usb/src/sys/dev/usb/usb_hub.c#40 edit
.. //depot/projects/usb/src/sys/dev/usb/usb_request.c#29 edit
Differences ...
==== //depot/projects/usb/src/sys/dev/usb/usb_device.c#65 (text+ko) ====
@@ -1379,7 +1379,7 @@
}
DPRINTFN(4, "udev=%p do_suspend=%d\n", udev, do_suspend);
- sx_assert(udev->default_sx + 1, SA_LOCKED);
+ sx_assert(udev->default_sx + 2, SA_LOCKED);
USB_BUS_LOCK(udev->bus);
/* filter the suspend events */
@@ -1494,6 +1494,7 @@
/* initialise our SX-lock */
sx_init(udev->default_sx + 1, "0123456789ABCDEF - USB config SX lock" + depth);
+ sx_init(udev->default_sx + 2, "0123456789ABCDEF - USB suspend and resume SX lock" + depth);
cv_init(udev->default_cv, "WCTRL");
cv_init(udev->default_cv + 1, "UGONE");
@@ -2037,6 +2038,7 @@
sx_destroy(udev->default_sx);
sx_destroy(udev->default_sx + 1);
+ sx_destroy(udev->default_sx + 2);
cv_destroy(udev->default_cv);
cv_destroy(udev->default_cv + 1);
@@ -2497,6 +2499,7 @@
usbd_enum_lock(struct usb_device *udev)
{
sx_xlock(udev->default_sx + 1);
+ sx_xlock(udev->default_sx + 2);
/*
* NEWBUS LOCK NOTE: We should check if any parent SX locks
* are locked before locking Giant. Else the lock can be
@@ -2511,9 +2514,33 @@
usbd_enum_unlock(struct usb_device *udev)
{
mtx_unlock(&Giant);
+ sx_xunlock(udev->default_sx + 2);
sx_xunlock(udev->default_sx + 1);
}
+/* The following function locks suspend and resume. */
+
+void
+usbd_sr_lock(struct usb_device *udev)
+{
+ sx_xlock(udev->default_sx + 2);
+ /*
+ * 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->default_sx + 2);
+}
+
/*
* The following function checks the enumerating lock for the given
* USB device.
==== //depot/projects/usb/src/sys/dev/usb/usb_device.h#34 (text+ko) ====
@@ -113,7 +113,7 @@
struct usb_device {
struct usb_clear_stall_msg cs_msg[2]; /* generic clear stall
* messages */
- struct sx default_sx[2];
+ struct sx default_sx[3];
struct mtx default_mtx[1];
struct cv default_cv[2];
struct usb_interface *ifaces;
@@ -213,6 +213,8 @@
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_ */
==== //depot/projects/usb/src/sys/dev/usb/usb_generic.c#27 (text+ko) ====
@@ -1735,14 +1735,34 @@
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;
==== //depot/projects/usb/src/sys/dev/usb/usb_hub.c#40 (text+ko) ====
@@ -126,6 +126,7 @@
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 @@
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 @@
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 @@
#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_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 @@
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) {
+ } else if ((temp >= limit) &&
+ (udev->flags.usb_mode == USB_MODE_HOST) &&
+ (udev->flags.self_suspended == 0)) {
+ /* try to do suspend */
- /* 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);
- }
+ USB_BUS_UNLOCK(bus);
+ usb_dev_suspend_peer(udev);
+ USB_BUS_LOCK(bus);
}
}
@@ -1920,6 +1931,9 @@
/* 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 @@
(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 @@
}
}
- 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 @@
"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;
==== //depot/projects/usb/src/sys/dev/usb/usb_request.c#29 (text+ko) ====
@@ -386,6 +386,7 @@
uint16_t length;
uint16_t temp;
uint16_t acttemp;
+ uint8_t enum_locked;
if (timeout < 50) {
/* timeout is too small */
@@ -397,6 +398,8 @@
}
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,
@@ -421,17 +424,22 @@
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:
*/
-
sx_xlock(udev->default_sx);
hr_func = usbd_get_hr_func(udev);
@@ -678,9 +686,12 @@
done:
sx_xunlock(udev->default_sx);
- if (mtx) {
+ if (enum_locked)
+ usbd_sr_lock(udev);
+
+ if ((mtx != NULL) && (mtx != &Giant))
mtx_lock(mtx);
- }
+
return ((usb_error_t)err);
}
More information about the p4-projects
mailing list