Hi Huang,
Thanks for notifying us of this issue and submitting a patch to attempt
to fix it. However, the patches look very odd to me, and I don't
understand the details of what they're trying to fix, so you will have
to help me understand what you're trying to do.
On Thu, Aug 22, 2013 at 11:56:22PM +0800, Huang Rui wrote:
> The following patch is required to resolve remote wake issues with
> certain devices.
>
> Issue description:
> If the remote wake is issued from the device in a specific timing
> condition while the system is entering sleep state then it may cause
> system to auto wake on subsequent sleep cycle.
What do you mean by "subsequent sleep cycle"? Do you mean that if I
enable a device to wake the system on resume, and the system goes into
S3 while the device signals a wake, then the system doesn't wakeup? And
then the system wakes up erroneously on the next sleep cycle?
I'm confused, please explain this issue in more detail.
> Root cause:
> Host controller rebroadcasts the Resume signal > 100 µseconds after
> receiving the original resume event from the device. For proper
> function, some devices may require the rebroadcast of resume event
> within the USB spec of 100µS.
As Alan said, why only _some_ devices? The host isn't compliant with
the spec, and you see that behavior happen on low speed mice. That
doesn't mean the issue won't occur with other USB devices that implement
remote wakeup. Did you test with, say, a high speed 3G modem or a USB
bluetooth adapter to see if you can trigger the remote wakeup issue?
> This patch is only for xHCI driver and the quirk will be also added
> into OHCI driver too.
>
> This should be backported to kernels as old as 3.9, that contrain the
> commit 3f5eb14135ba9d97ba4b8514fc7ef5e0dac2abf4 "usb: add
> find_raw_port_number callback to struct hc_driver()".
Why that kernel in particular? That commit seems to have nothing to do
with your current patch, aside from the fact that it will make it
harder to back port. This problem will show up in any kernel with AMD
xHCI host support, right? So why aren't you suggesting backporting to
kernels that added older AMD quirks?
More comments below.
>
> Cc <sta...@vger.kernel.org>
> Signed-off-by: Huang Rui <ray.hu...@amd.com>
> ---
> drivers/usb/core/hub.c | 28 ++++++++++++++++++++
> drivers/usb/host/pci-quirks.c | 17 +++++++++++-
> drivers/usb/host/pci-quirks.h | 1 +
> drivers/usb/host/xhci-pci.c | 4 +++
> drivers/usb/host/xhci.c | 61
> +++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/xhci.h | 25 +++++++++---------
> include/linux/usb.h | 1 +
> 7 files changed, 124 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 175179e..117196c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -27,6 +27,7 @@
> #include <linux/freezer.h>
> #include <linux/random.h>
> #include <linux/pm_qos.h>
> +#include <linux/hid.h>
>
> #include <asm/uaccess.h>
> #include <asm/byteorder.h>
> @@ -5437,3 +5438,30 @@ acpi_handle usb_get_hub_port_acpi_handle(struct
> usb_device *hdev,
> return DEVICE_ACPI_HANDLE(&hub->ports[port1 - 1]->dev);
> }
> #endif
> +
> +bool is_usb_mouse(struct usb_device *udev)
> +{
> + struct usb_interface *intf;
> + struct usb_interface_descriptor intf_desc;
> +
> + if (!udev) {
> + dev_warn(&udev->dev, "Warn: no device attached!\n");
> + return false;
> + }
> +
> + intf = usb_ifnum_to_if(udev, 0);
> + intf_desc = intf->cur_altsetting->desc;
> + if (intf_desc.bInterfaceClass == USB_INTERFACE_CLASS_HID &&
> + intf_desc.bInterfaceSubClass ==
> + USB_INTERFACE_SUBCLASS_BOOT &&
> + intf_desc.bInterfaceProtocol ==
> + USB_INTERFACE_PROTOCOL_MOUSE) {
> + dev_dbg(&udev->dev, "The attached usb device is"
> + " mouse\n");
> + return true;
> + }
> +
> + dev_dbg(&udev->dev, "The attached usb device is NOT mouse\n");
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(is_usb_mouse);
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 2c76ef1..218e421 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -140,9 +140,11 @@ int usb_amd_find_chipset_info(void)
> rev = info.smbus_dev->revision;
> if (rev >= 0x11 && rev <= 0x18)
> info.sb_type = 2;
> + if (rev >= 0x39 && rev <= 0x3a)
> + info.sb_type = 4;
> }
>
> - if (info.sb_type == 0) {
> + if (info.sb_type == 0 || info.sb_type == 4) {
> if (info.smbus_dev) {
> pci_dev_put(info.smbus_dev);
> info.smbus_dev = NULL;
> @@ -197,6 +199,19 @@ commit:
> }
> EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
>
> +int usb_amd_remote_wakeup_quirk(void)
> +{
> + u8 ret;
> + if (amd_chipset.sb_type == 4) {
> + ret = 1;
> + printk(KERN_DEBUG "QUIRK: Enable AMD remote wakup"
> + " fix\n");
Don't split strings. It's OK to break the 80 character rule for
strings, because we want people to grep for them.
> + } else
> + ret = 0;
> + return ret;
> +}
why not:
if (amd_chipset.sb_type != 4)
return 0;
printk(KERN_DEBUG "QUIRK: Enable AMD remote wakup fix\n");
return 1;
Saves a variable, less lines of code.
> +EXPORT_SYMBOL_GPL(usb_amd_remote_wakeup_quirk);
> +
> /*
> * The hardware normally enables the A-link power management feature, which
> * lets the system lower the power consumption in idle states.
> diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
> index ed6700d..e28bbbe 100644
> --- a/drivers/usb/host/pci-quirks.h
> +++ b/drivers/usb/host/pci-quirks.h
> @@ -5,6 +5,7 @@
> void uhci_reset_hc(struct pci_dev *pdev, unsigned long base);
> int uhci_check_and_reset_hc(struct pci_dev *pdev, unsigned long base);
> int usb_amd_find_chipset_info(void);
> +int usb_amd_remote_wakeup_quirk(void);
> void usb_amd_dev_put(void);
> void usb_amd_quirk_pll_disable(void);
> void usb_amd_quirk_pll_enable(void);
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index c2d4950..cc2ff7e 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -90,6 +90,10 @@ static void xhci_pci_quirks(struct device *dev, struct
> xhci_hcd *xhci)
> /* AMD PLL quirk */
> if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_find_chipset_info())
> xhci->quirks |= XHCI_AMD_PLL_FIX;
> +
> + if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_remote_wakeup_quirk())
> + xhci->quirks |= XHCI_AMD_REMOTE_WAKEUP_QUIRK;
> +
> if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> xhci->quirks |= XHCI_LPM_SUPPORT;
> xhci->quirks |= XHCI_INTEL_HOST;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index ba0ec0a..dcf4552 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -831,6 +831,64 @@ static void xhci_clear_command_ring(struct xhci_hcd
> *xhci)
> }
>
> /*
> + * Reset port which attached with mouse.
> + *
> + * If the remote wake is issued from the device in a specific timing
> + * condition while the system is entering sleep state then it may
> + * cause system to auto wake on subsequent sleep cycle.
> + *
> + * Host controller rebroadcasts the Resume signal > 100 µseconds after
> + * receiving the original resume event from the device. For proper
> + * function, some devices may require the rebroadcast of resume event
> + * within the USB spec of 100µS.
> + *
> + * Without this quirk, some AMD platform doesn't hold the resume right
> + * away when there is a resume signal from LS device like mouse. So it
> + * should reset the port which attached with mouse.
> + */
> +static int xhci_reset_port_by_mouse(struct xhci_hcd *xhci)
> +{
> + __le32 __iomem *addr, *base_addr;
> + u32 temp;
> + struct usb_device *hdev, *child;
> + unsigned long flags;
> + int port1, raw_port;
> +
> + xhci_dbg(xhci, "%s, reset port attached with usb mouse\n",
> + __func__);
> +
> + base_addr = &xhci->op_regs->port_status_base;
> +
> + hdev = hcd_to_bus(xhci->main_hcd)->root_hub;
> +
> + usb_hub_for_each_child(hdev, port1, child) {
> + raw_port = xhci_find_raw_port_number(xhci->main_hcd,
> + port1);
> + addr = (raw_port - 1) * NUM_PORT_REGS + base_addr;
> +
> + temp = xhci_readl(xhci, addr);
> +
> + if (is_usb_mouse(child)) {
> + xhci_dbg(xhci, "Attached mouse on port %d.\n",
> + port1);
> + spin_lock_irqsave(&xhci->lock, flags);
> + temp |= PORT_RESET;
> + xhci_writel(xhci, temp, addr);
> + if (xhci_handshake(xhci, addr, PORT_RESET, 0,
> + 10 * 1000)) {
> + xhci_warn(xhci, "WARN: xHC port reset"
> + " timeout\n");
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + return -ETIMEDOUT;
> + }
> + spin_unlock_irqrestore(&xhci->lock, flags);
Think about this very carefully. You've called spin_lock_irqsave here,
so you're going to hold the xHCI spin lock with IRQs disabled for up to
10 microseconds. That means you're potentially starving all the other
devices and processes that need interrupts. Don't do this.
I know there are other places in the xHCI driver that hold a spin lock
and call xhci_handshake. Most only only delay for a very short amount
of time (125 microseconds) if they've disabled interrupts. There's one
place that calls xhci_handshake with a timeout of 10 ms while holding
the xhci lock, but it does so when interrupts are enabled, and the host
is resuming, so we don't have to worry about starving the xHCI interrupt
routine.
Basically: think very very carefully about doing any sort of delay while
holding a spinlock, and under no circumstances should you delay for 10
ms while interrupts are disabled.
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> * Stop HC (not bus-specific)
> *
> * This is called when the machine transition into S3/S4 mode.
> @@ -1043,6 +1101,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
> compliance_mode_recovery_timer_init(xhci);
>
> + if (xhci->quirks & XHCI_AMD_REMOTE_WAKEUP_QUIRK)
> + xhci_reset_port_by_mouse(xhci);
> +
So, basically, whenever the xHCI host resumes from S3, you're going to
reset mice attached to the ports. Again, why only mice?
Alan, is there a way to force all devices under a host to have a reset
resume quirk?
> /* Re-enable port polling. */
> xhci_dbg(xhci, "%s: starting port polling.\n", __func__);
> set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 46aa148..1862f57 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1526,18 +1526,19 @@ struct xhci_hcd {
> * commands, reset device commands, disable slot commands, and address device
> * commands.
> */
> -#define XHCI_EP_LIMIT_QUIRK (1 << 5)
> -#define XHCI_BROKEN_MSI (1 << 6)
> -#define XHCI_RESET_ON_RESUME (1 << 7)
> -#define XHCI_SW_BW_CHECKING (1 << 8)
> -#define XHCI_AMD_0x96_HOST (1 << 9)
> -#define XHCI_TRUST_TX_LENGTH (1 << 10)
> -#define XHCI_LPM_SUPPORT (1 << 11)
> -#define XHCI_INTEL_HOST (1 << 12)
> -#define XHCI_SPURIOUS_REBOOT (1 << 13)
> -#define XHCI_COMP_MODE_QUIRK (1 << 14)
> -#define XHCI_AVOID_BEI (1 << 15)
> -#define XHCI_PLAT (1 << 16)
> +#define XHCI_EP_LIMIT_QUIRK (1 << 5)
> +#define XHCI_BROKEN_MSI (1 << 6)
> +#define XHCI_RESET_ON_RESUME (1 << 7)
> +#define XHCI_SW_BW_CHECKING (1 << 8)
> +#define XHCI_AMD_0x96_HOST (1 << 9)
> +#define XHCI_TRUST_TX_LENGTH (1 << 10)
> +#define XHCI_LPM_SUPPORT (1 << 11)
> +#define XHCI_INTEL_HOST (1 << 12)
> +#define XHCI_SPURIOUS_REBOOT (1 << 13)
> +#define XHCI_COMP_MODE_QUIRK (1 << 14)
> +#define XHCI_AVOID_BEI (1 << 15)
> +#define XHCI_PLAT (1 << 16)
> +#define XHCI_AMD_REMOTE_WAKEUP_QUIRK (1 << 17)
If you're going to add an extra tab in these quirks macros, please
do so from XHCI_LINK_TRB_QUIRK.
> unsigned int num_active_eps;
> unsigned int limit_active_eps;
> /* There are two roothubs to keep track of bus suspend info for */
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 001629c..f7b1a6c 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -591,6 +591,7 @@ extern struct usb_device *usb_get_dev(struct usb_device
> *dev);
> extern void usb_put_dev(struct usb_device *dev);
> extern struct usb_device *usb_hub_find_child(struct usb_device *hdev,
> int port1);
> +extern bool is_usb_mouse(struct usb_device *udev);
>
> /**
> * usb_hub_for_each_child - iterate over all child devices on the hub
> --
> 1.7.11.7
>
>
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
