From aa5ceae24bf8dff1d6fe87c6c4b08e69c6d33550 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Tue, 30 Jul 2013 15:39:02 -0400 Subject: [PATCH 1/4] USB: handle LPM errors during device suspend correctly The hub driver's usb_port_suspend() routine doesn't handle errors related to Link Power Management properly. It always returns failure, it doesn't try to clean up the wakeup setting, (in the case of system sleep) it doesn't try to go ahead with the port suspend regardless, and it doesn't try to apply the new power-off mechanism. This patch fixes these problems. Note: Sarah fixed this patch to apply against 3.11, since the original commit (4fae6f0fa86f92e6bc7429371b1e177ad0aaac66 "USB: handle LPM errors during device suspend correctly") called usb_disable_remote_wakeup, which won't be added until 3.12. This patch should be backported to kernels as old as 3.5, that contain the commit 8306095fd2c1100e8244c09bf560f97aca5a311d "USB: Disable USB 3.0 LPM in critical sections.". There will be merge conflicts, since LTM wasn't added until 3.6. Signed-off-by: Alan Stern Signed-off-by: Sarah Sharp Cc: stable@vger.kernel.org --- drivers/usb/core/hub.c | 71 ++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 33 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 558313de4911..8287953e066c 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2918,7 +2918,6 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) { struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent); struct usb_port *port_dev = hub->ports[udev->portnum - 1]; - enum pm_qos_flags_status pm_qos_stat; int port1 = udev->portnum; int status; bool really_suspend = true; @@ -2956,7 +2955,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) status); /* bail if autosuspend is requested */ if (PMSG_IS_AUTO(msg)) - return status; + goto err_wakeup; } } @@ -2965,14 +2964,16 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) usb_set_usb2_hardware_lpm(udev, 0); if (usb_disable_ltm(udev)) { - dev_err(&udev->dev, "%s Failed to disable LTM before suspend\n.", - __func__); - return -ENOMEM; + dev_err(&udev->dev, "Failed to disable LTM before suspend\n."); + status = -ENOMEM; + if (PMSG_IS_AUTO(msg)) + goto err_ltm; } if (usb_unlocked_disable_lpm(udev)) { - dev_err(&udev->dev, "%s Failed to disable LPM before suspend\n.", - __func__); - return -ENOMEM; + dev_err(&udev->dev, "Failed to disable LPM before suspend\n."); + status = -ENOMEM; + if (PMSG_IS_AUTO(msg)) + goto err_lpm3; } /* see 7.1.7.6 */ @@ -3000,28 +3001,31 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) if (status) { dev_dbg(hub->intfdev, "can't suspend port %d, status %d\n", port1, status); - /* paranoia: "should not happen" */ - if (udev->do_remote_wakeup) { - if (!hub_is_superspeed(hub->hdev)) { - (void) usb_control_msg(udev, - usb_sndctrlpipe(udev, 0), - USB_REQ_CLEAR_FEATURE, - USB_RECIP_DEVICE, - USB_DEVICE_REMOTE_WAKEUP, 0, - NULL, 0, - USB_CTRL_SET_TIMEOUT); - } else - (void) usb_disable_function_remotewakeup(udev); - - } + /* Try to enable USB3 LPM and LTM again */ + usb_unlocked_enable_lpm(udev); + err_lpm3: + usb_enable_ltm(udev); + err_ltm: /* Try to enable USB2 hardware LPM again */ if (udev->usb2_hw_lpm_capable == 1) usb_set_usb2_hardware_lpm(udev, 1); - /* Try to enable USB3 LTM and LPM again */ - usb_enable_ltm(udev); - usb_unlocked_enable_lpm(udev); + if (udev->do_remote_wakeup) { + if (udev->speed < USB_SPEED_SUPER) + usb_control_msg(udev, usb_sndctrlpipe(udev, 0), + USB_REQ_CLEAR_FEATURE, + USB_RECIP_DEVICE, + USB_DEVICE_REMOTE_WAKEUP, 0, + NULL, 0, USB_CTRL_SET_TIMEOUT); + else + usb_control_msg(udev, usb_sndctrlpipe(udev, 0), + USB_REQ_CLEAR_FEATURE, + USB_RECIP_INTERFACE, + USB_INTRF_FUNC_SUSPEND, 0, + NULL, 0, USB_CTRL_SET_TIMEOUT); + } + err_wakeup: /* System sleep transitions should never fail */ if (!PMSG_IS_AUTO(msg)) @@ -3043,14 +3047,15 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) * Check whether current status meets the requirement of * usb port power off mechanism */ - pm_qos_stat = dev_pm_qos_flags(&port_dev->dev, - PM_QOS_FLAG_NO_POWER_OFF); - if (!udev->do_remote_wakeup - && pm_qos_stat != PM_QOS_FLAGS_ALL - && udev->persist_enabled - && !status) { - pm_runtime_put_sync(&port_dev->dev); - port_dev->did_runtime_put = true; + if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled) { + enum pm_qos_flags_status pm_qos_stat; + + pm_qos_stat = dev_pm_qos_flags(&port_dev->dev, + PM_QOS_FLAG_NO_POWER_OFF); + if (pm_qos_stat != PM_QOS_FLAGS_ALL) { + pm_runtime_put_sync(&port_dev->dev); + port_dev->did_runtime_put = true; + } } usb_mark_last_busy(hub->hdev); From 98a4f1ff7bea8002ab79d6776e30d27932e88244 Mon Sep 17 00:00:00 2001 From: Lan Tianyu Date: Wed, 3 Jul 2013 22:17:54 +0800 Subject: [PATCH 2/4] usb: don't check pm qos NO_POWER_OFF flag in usb_port_suspend() The pm qos NO_POWER_OFF flag is checked twice during usb device suspend to see if the usb port power off condition is met. This is redundant and also will prevent the port from being powered off if the NO_POWER_OFF flag is changed to 1 from 0 after the device was already suspended. More detail in the following link. http://marc.info/?l=linux-usb&m=136543949130865&w=2 This patch should be backported to kernels as old as 3.7, that contain the commit f7ac7787ad361e31a7972e2854ed8dc2eedfac3b "usb/acpi: Use ACPI methods to power off ports." Signed-off-by: Lan Tianyu Signed-off-by: Sarah Sharp Cc: stable@vger.kernel.org --- drivers/usb/core/hub.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 8287953e066c..17c3785241a4 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -3043,19 +3043,9 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) usb_set_device_state(udev, USB_STATE_SUSPENDED); } - /* - * Check whether current status meets the requirement of - * usb port power off mechanism - */ if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled) { - enum pm_qos_flags_status pm_qos_stat; - - pm_qos_stat = dev_pm_qos_flags(&port_dev->dev, - PM_QOS_FLAG_NO_POWER_OFF); - if (pm_qos_stat != PM_QOS_FLAGS_ALL) { - pm_runtime_put_sync(&port_dev->dev); - port_dev->did_runtime_put = true; - } + pm_runtime_put_sync(&port_dev->dev); + port_dev->did_runtime_put = true; } usb_mark_last_busy(hub->hdev); From d49dad3e11638f66be4e16573ffaa8c46a09e3b3 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Mon, 5 Aug 2013 18:58:15 -0700 Subject: [PATCH 3/4] usb: Don't fail port power resume on device disconnect. Userspace can tell the kernel to power off any USB port, including ones that are visible and connectible to users. When an attached USB device goes into suspend, the port will be powered off if the pm_qos_no_port_poweroff file for its port is set to 0, the device does not have remote wakeup enabled, and the device is marked as persistent. If the user disconnects the USB device while the port is powered off, the current code does not handle that properly. If you disconnect a device, and then run `lsusb -v -s` for the device, the device disconnect does not get handled by the USB core. The runtime resume of the port fails, because hub_port_debounce_be_connected() returns -ETIMEDOUT. This means the port resume fails and khubd doesn't handle the USB device disconnect. This leaves the device listed in lsusb, and the port's runtime_status will be permanently marked as "error". Fix this by ignoring the return value of hub_port_debounce_be_connected. Users can disconnect USB devices while the ports are powered off, and we must be able to handle that. This patch should be backported to kernels as old as 3.9, that contain the commit ad493e5e580546e6c3024b76a41535476da1546a "usb: add usb port auto power off mechanism" Signed-off-by: Sarah Sharp Cc: Lan Tianyu Cc: Alan Stern Cc: Rafael J. Wysocki Cc: stable@vger.kernel.org Signed-off-by: Sarah Sharp --- drivers/usb/core/port.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index d6b0fadf53e9..9909911665ce 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -89,22 +89,19 @@ static int usb_port_runtime_resume(struct device *dev) retval = usb_hub_set_port_power(hdev, hub, port1, true); if (port_dev->child && !retval) { /* - * Wait for usb hub port to be reconnected in order to make - * the resume procedure successful. + * Attempt to wait for usb hub port to be reconnected in order + * to make the resume procedure successful. The device may have + * disconnected while the port was powered off, so ignore the + * return status. */ retval = hub_port_debounce_be_connected(hub, port1); - if (retval < 0) { + if (retval < 0) dev_dbg(&port_dev->dev, "can't get reconnection after setting port power on, status %d\n", retval); - goto out; - } usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE); - - /* Set return value to 0 if debounce successful */ retval = 0; } -out: clear_bit(port1, hub->busy_bits); usb_autopm_put_interface(intf); return retval; From 52fb61250a7a132b0cfb9f4a1060a1f3c49e5a25 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Thu, 8 Aug 2013 10:08:34 -0700 Subject: [PATCH 4/4] xhci-plat: Don't enable legacy PCI interrupts. The xHCI platform driver calls into usb_add_hcd to register the irq for its platform device. It does not want the xHCI generic driver to register an interrupt for it at all. The original code did that by setting the XHCI_BROKEN_MSI quirk, which tells the xHCI driver to not enable MSI or MSI-X for a PCI host. Unfortunately, if CONFIG_PCI is enabled, and CONFIG_USB_DW3 is enabled, the xHCI generic driver will attempt to register a legacy PCI interrupt for the xHCI platform device in xhci_try_enable_msi(). This will result in a bogus irq being registered, since the underlying device is a platform_device, not a pci_device, and thus the pci_device->irq pointer will be bogus. Add a new quirk, XHCI_PLAT, so that the xHCI generic driver can distinguish between a PCI device that can't handle MSI or MSI-X, and a platform device that should not have its interrupts touched at all. This quirk may be useful in the future, in case other corner cases like this arise. This patch should be backported to kernels as old as 3.9, that contain the commit 00eed9c814cb8f281be6f0f5d8f45025dc0a97eb "USB: xhci: correctly enable interrupts". Signed-off-by: Sarah Sharp Reported-by: Yu Y Wang Tested-by: Yu Y Wang Reviewed-by: Felipe Balbi Cc: stable@vger.kernel.org --- drivers/usb/host/xhci-plat.c | 2 +- drivers/usb/host/xhci.c | 7 ++++++- drivers/usb/host/xhci.h | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 51e22bf89505..6eca5a536c0a 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -24,7 +24,7 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci) * here that the generic code does not try to make a pci_dev from our * dev struct in order to setup MSI */ - xhci->quirks |= XHCI_BROKEN_MSI; + xhci->quirks |= XHCI_PLAT; } /* called during probe() after chip reset completes */ diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 9478caa2f71f..ead3555d6dba 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -343,9 +343,14 @@ static void __maybe_unused xhci_msix_sync_irqs(struct xhci_hcd *xhci) static int xhci_try_enable_msi(struct usb_hcd *hcd) { struct xhci_hcd *xhci = hcd_to_xhci(hcd); - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); + struct pci_dev *pdev; int ret; + /* The xhci platform device has set up IRQs through usb_add_hcd. */ + if (xhci->quirks & XHCI_PLAT) + return 0; + + pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); /* * Some Fresco Logic host controllers advertise MSI, but fail to * generate interrupts. Don't even try to enable MSI. diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index c338741a675d..6ab1e600dc18 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1542,6 +1542,7 @@ struct xhci_hcd { #define XHCI_SPURIOUS_REBOOT (1 << 13) #define XHCI_COMP_MODE_QUIRK (1 << 14) #define XHCI_AVOID_BEI (1 << 15) +#define XHCI_PLAT (1 << 16) unsigned int num_active_eps; unsigned int limit_active_eps; /* There are two roothubs to keep track of bus suspend info for */