Hi Sarah,
On Thu, Aug 22, 2013 at 04:02:54PM -0700, Sarah Sharp wrote:
> Hi Huang,
>
> Thanks for notifying us of this issue and submitting a patch to attempt
> to fix it.
Thank you to your reply. Any comments are warm for me. :)
I am the replacement of Andiry Xu, will work on usb driver for AMD
platforms. If any issue you want to test or verify in different
platform in future, I'm pleasue to do 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.
>
Sorry, that's because I don't explain very clearly. I will try my best
to make you clear.
> 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.
>
As I explained the reproduce steps in last mail, the "subsequent sleep
cycle" means that execute S3 at the second time after use usb mouse
with clicking _intensely_ to wake up system from the first time to
access S3. If do it above steps, the system would auto wake
erroneously as soon as sleep in S3 at the second time.
That's because hardware can't hold or clear the resume signal if we do
it like above steps to wake from S3. So the patch is that reset the
port attached at mouse during the resume phase, then the resume signal
is cleared, so this issue never encountered again.
> > 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?
>
Only the mice would trigger this issue, high speed devices doesn't. I
will check with HW guys in my side.
> > 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?
Because I bisect the kernel and found this commit implement
find_raw_port_number which my patch called. I expected this fix would
backport to stable tree.
> 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?
Yes, you're right. This problem shows up in any kernel with only some
AMD xhci host.
> So why aren't you suggesting backporting to
> kernels that added older AMD quirks?
>
This quirk is dependent of older AMD quirk, why do you suggest that?
> More comments below.
>
> >
> > Cc <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> > ---
> > 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.
>
Got it, I will fix these in v2.
> > + } 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.
>
Yes, will change.
> > +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.
>
OK, so I should remove spinlock and handshake here and do xhci_writel
to set PORT_RESET directly. Am I right?
> > + }
> > + }
> > +
> > + 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?
>
Yes, I will check with HW guys that give a detailed explanation.
> 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.
>
OK, I just want to align at other quirks.
Thanks again for your kindly commments.
Rui
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
