From 3824c1ddaf744be44b170a335332b9d6afe79254 Mon Sep 17 00:00:00 2001 From: Libor Pechacek Date: Fri, 20 May 2011 14:53:25 +0200 Subject: USB: core: Tolerate protocol stall during hub and port status read Protocol stall should not be fatal while reading port or hub status as it is transient state. Currently hub EP0 STALL during port status read results in failed device enumeration. This has been observed with ST-Ericsson (formerly Philips) USB 2.0 Hub (04cc:1521) after connecting keyboard. Signed-off-by: Libor Pechacek Acked-by: Alan Stern Cc: stable Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hub.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'drivers/usb/core/hub.c') diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 79a58c3a2e2a..90ae1753dda1 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -339,7 +339,8 @@ static int get_hub_status(struct usb_device *hdev, { int i, status = -ETIMEDOUT; - for (i = 0; i < USB_STS_RETRIES && status == -ETIMEDOUT; i++) { + for (i = 0; i < USB_STS_RETRIES && + (status == -ETIMEDOUT || status == -EPIPE); i++) { status = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_HUB, 0, 0, data, sizeof(*data), USB_STS_TIMEOUT); @@ -355,7 +356,8 @@ static int get_port_status(struct usb_device *hdev, int port1, { int i, status = -ETIMEDOUT; - for (i = 0; i < USB_STS_RETRIES && status == -ETIMEDOUT; i++) { + for (i = 0; i < USB_STS_RETRIES && + (status == -ETIMEDOUT || status == -EPIPE); i++) { status = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, port1, data, sizeof(*data), USB_STS_TIMEOUT); -- cgit v1.2.3 From fccf4e86200b8f5edd9a65da26f150e32ba79808 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Sun, 5 Jun 2011 23:22:22 -0700 Subject: USB: Free bandwidth when usb_disable_device is called. Tanya ran into an issue when trying to switch a UAS device from the BOT configuration to the UAS configuration via the bConfigurationValue sysfs file. Before installing the UAS configuration, set_bConfigurationValue() calls usb_disable_device(). That function is supposed to remove all host controller resources associated with that device, but it leaves some state in the xHCI host controller. Commit 0791971ba8fbc44e4f476079f856335ed45e6324 usb: allow drivers to use allocated bandwidth until unbound added a call to usb_disable_device() in usb_set_configuration(), before the xHCI bandwidth functions were invoked. That commit fixed a bug, but also introduced a bug that is triggered when a configured device is switched to a new configuration. usb_disable_device() goes through all the motions of unbinding the drivers attached to active interfaces and removing the USB core structures associated with those interfaces, but it doesn't actually remove the endpoints from the internal xHCI host controller bandwidth structures. When usb_disable_device() calls usb_disable_endpoint() with reset_hardware set to true, the entries in udev->ep_out and udev->ep_in will be set to NULL. Usually, when the USB core installs a new configuration, usb_hcd_alloc_bandwidth() will drop all non-NULL endpoints in udev->ep_out and udev->ep_in before adding any new endpoints. However, when the new UAS configuration was added, all those entries were null, so none of the old endpoints in the BOT configuration were dropped. The xHCI driver blindly added the UAS configuration endpoints, and some of the endpoint addresses overlapped with the old BOT configuration endpoints. This caused the xHCI host to reject the Configure Endpoint command. Now that the xHCI driver code is cleaned up to reject a double-add of active endpoints, we need to fix the USB core to properly drop old endpoints in usb_disable_device(). If the host controller driver needs bandwidth checking support, make usb_disable_device() call usb_disable_endpoint() with reset_hardware set to false, drop the endpoints from the xHCI host controller, and then call usb_disable_endpoint() again with reset_hardware set to true. The first call to usb_disable_endpoint() will cancel any pending URBs and wait on them to be freed in usb_hcd_disable_endpoint(), but will keep the pointers in udev->ep_out and udev->ep in intact. Then usb_hcd_alloc_bandwidth() will use those pointers to know which endpoints to drop. The final call to usb_disable_endpoint() will do two things: 1. It will call usb_hcd_disable_endpoint() again, which should be harmless since the ep->urb_list should be empty after the first call to usb_disable_endpoint() returns. 2. It will set the entries in udev->ep_out and udev->ep in to NULL, and call usb_hcd_disable_endpoint(). That call will have no effect, since the xHCI driver doesn't set the endpoint_disable function pointer. Note that usb_disable_device() will now need to be called with hcd->bandwidth_mutex held. This should be backported to kernels as old as 2.6.32. Signed-off-by: Sarah Sharp Reported-by: Tanya Brokhman Cc: ablay@codeaurora.org Cc: Alan Stern Cc: stable@kernel.org --- drivers/usb/core/hub.c | 3 +++ drivers/usb/core/message.c | 15 ++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) (limited to 'drivers/usb/core/hub.c') diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 90ae1753dda1..ca339bc799e4 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1634,6 +1634,7 @@ void usb_disconnect(struct usb_device **pdev) { struct usb_device *udev = *pdev; int i; + struct usb_hcd *hcd = bus_to_hcd(udev->bus); if (!udev) { pr_debug ("%s nodev\n", __func__); @@ -1661,7 +1662,9 @@ void usb_disconnect(struct usb_device **pdev) * so that the hardware is now fully quiesced. */ dev_dbg (&udev->dev, "unregistering device\n"); + mutex_lock(hcd->bandwidth_mutex); usb_disable_device(udev, 0); + mutex_unlock(hcd->bandwidth_mutex); usb_hcd_synchronize_unlinks(udev); usb_remove_ep_devs(&udev->ep0); diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 5701e857392b..64c7ab4702df 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -1135,10 +1135,13 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf, * Deallocates hcd/hardware state for the endpoints (nuking all or most * pending urbs) and usbcore state for the interfaces, so that usbcore * must usb_set_configuration() before any interfaces could be used. + * + * Must be called with hcd->bandwidth_mutex held. */ void usb_disable_device(struct usb_device *dev, int skip_ep0) { int i; + struct usb_hcd *hcd = bus_to_hcd(dev->bus); /* getting rid of interfaces will disconnect * any drivers bound to them (a key side effect) @@ -1172,6 +1175,16 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0) dev_dbg(&dev->dev, "%s nuking %s URBs\n", __func__, skip_ep0 ? "non-ep0" : "all"); + if (hcd->driver->check_bandwidth) { + /* First pass: Cancel URBs, leave endpoint pointers intact. */ + for (i = skip_ep0; i < 16; ++i) { + usb_disable_endpoint(dev, i, false); + usb_disable_endpoint(dev, i + USB_DIR_IN, false); + } + /* Remove endpoints from the host controller internal state */ + usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL); + /* Second pass: remove endpoint pointers */ + } for (i = skip_ep0; i < 16; ++i) { usb_disable_endpoint(dev, i, true); usb_disable_endpoint(dev, i + USB_DIR_IN, true); @@ -1727,6 +1740,7 @@ free_interfaces: /* if it's already configured, clear out old state first. * getting rid of old interfaces means unbinding their drivers. */ + mutex_lock(hcd->bandwidth_mutex); if (dev->state != USB_STATE_ADDRESS) usb_disable_device(dev, 1); /* Skip ep0 */ @@ -1739,7 +1753,6 @@ free_interfaces: * host controller will not allow submissions to dropped endpoints. If * this call fails, the device state is unchanged. */ - mutex_lock(hcd->bandwidth_mutex); ret = usb_hcd_alloc_bandwidth(dev, cp, NULL, NULL); if (ret < 0) { mutex_unlock(hcd->bandwidth_mutex); -- cgit v1.2.3 From cbb330045e5df8f665ac60227ff898421fc8fb92 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Wed, 15 Jun 2011 16:29:16 -0400 Subject: USB: don't let the hub driver prevent system sleep This patch (as1465) continues implementation of the policy that errors during suspend or hibernation should not prevent the system from going to sleep. In this case, failure to turn on the Suspend feature for a hub port shouldn't be reported as an error. There are situations where this does actually occur (such as when the device plugged into that port was disconnected in the recent past), and it turns out to be harmless. There's no reason for it to prevent a system sleep. Also, don't allow the hub driver to fail a system suspend if the downstream ports aren't all suspended. This is also harmless (and should never happen, given the change mentioned above); printing a warning message in the kernel log is all we really need to do. Signed-off-by: Alan Stern CC: Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hub.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'drivers/usb/core/hub.c') diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 90ae1753dda1..c2ac08755f27 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2362,6 +2362,10 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) USB_DEVICE_REMOTE_WAKEUP, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); + + /* System sleep transitions should never fail */ + if (!(msg.event & PM_EVENT_AUTO)) + status = 0; } else { /* device has up to 10 msec to fully suspend */ dev_dbg(&udev->dev, "usb %ssuspend\n", @@ -2611,16 +2615,15 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg) struct usb_device *hdev = hub->hdev; unsigned port1; - /* fail if children aren't already suspended */ + /* Warn if children aren't already suspended */ for (port1 = 1; port1 <= hdev->maxchild; port1++) { struct usb_device *udev; udev = hdev->children [port1-1]; if (udev && udev->can_submit) { - if (!(msg.event & PM_EVENT_AUTO)) - dev_dbg(&intf->dev, "port %d nyet suspended\n", - port1); - return -EBUSY; + dev_warn(&intf->dev, "port %d nyet suspended\n", port1); + if (msg.event & PM_EVENT_AUTO) + return -EBUSY; } } -- cgit v1.2.3