On Tue, Sep 10, 2013 at 08:24:51AM +0200, Takashi Iwai wrote:
> Haswell LynxPoint and LynxPoint-LP with the recent Intel BIOS show
> mysterious wakeups after shutdown occasionally. After discussing with
> BIOS engineers, they explained that the new BIOS expects that the
> wakeup sources are cleared and set to D3 for all wakeup devices when
> the system is going to sleep or power off, but the current xhci driver
> doesn't do this properly (partly intentionally).
>
> This patch introduces a new quirk, XHCI_HSW_SPURIOUS_WAKEUP, for
> fixing the spurious wakeups at S5 by calling xhci_stop() instead of
> its stripped version in the xhci shutdown ops, and setting the device
> to PCI D3 at shutdown and remove ops.
Which part of xhci_stop() causes the wakeups to cease? Is it the reset
of the host, or the disabling of the event ring interrupts, or both?
I'd like to understand exactly what caused the BIOS to wake up the
system.
Comments below.
> The PCI D3 call is based on the initial fix patch by Oliver Neukum.
>
> Cc: Oliver Neukum <oneukum@xxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> v1->v2: forgot to refresh the patch before submission, sorry.
>
> drivers/usb/host/xhci-pci.c | 14 ++++++++++++++
> drivers/usb/host/xhci.c | 18 +++++++++++++-----
> drivers/usb/host/xhci.h | 1 +
> include/linux/pci_ids.h | 2 ++
> 4 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index c2d4950..bfed043 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -110,6 +110,15 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> xhci->quirks |= XHCI_SPURIOUS_REBOOT;
> xhci->quirks |= XHCI_AVOID_BEI;
> }
> + if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> + (pdev->device == PCI_DEVICE_ID_INTEL_LYNXPOINT_XHCI ||
> + pdev->device == PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI)) {
> + /* Workaround for occasional spurious wakeups from S5 (or
> + * any other sleep) on Haswell machines with LPT and LPT-LP
> + * with the new Intel BIOS
> + */
> + xhci->quirks |= XHCI_HSW_SPURIOUS_WAKEUP;
> + }
> if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
> pdev->device == PCI_DEVICE_ID_ASROCK_P67) {
> xhci->quirks |= XHCI_RESET_ON_RESUME;
> @@ -217,6 +226,11 @@ static void xhci_pci_remove(struct pci_dev *dev)
> usb_put_hcd(xhci->shared_hcd);
> }
> usb_hcd_pci_remove(dev);
> +
> + /* Workaround for spurious wakeups at shutdown with HSW */
> + if (xhci->quirks & XHCI_HSW_SPURIOUS_WAKEUP)
> + pci_set_power_state(dev, PCI_D3cold);
> +
As far as I know, the LPT and LPT-LP hosts don't support D3cold. Is
there a particular reason you set the power state to D3cold here?
Sarah Sharp
> kfree(xhci);
> }
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 49b6edb..c6027ba 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -728,11 +728,19 @@ void xhci_shutdown(struct usb_hcd *hcd)
> if (xhci->quirks & XHCI_SPURIOUS_REBOOT)
> usb_disable_xhci_ports(to_pci_dev(hcd->self.controller));
>
> - spin_lock_irq(&xhci->lock);
> - xhci_halt(xhci);
> - spin_unlock_irq(&xhci->lock);
> -
> - xhci_cleanup_msix(xhci);
> + if (xhci->quirks & XHCI_HSW_SPURIOUS_WAKEUP) {
> + /* Workaround for spurious wakeups at shutdown with HSW:
> + * we must call xhci_stop() for clearing all wakeups, and
> + * set the device to D3.
> + */
> + xhci_stop(hcd);
> + pci_set_power_state(to_pci_dev(hcd->self.controller), PCI_D3cold);
> + } else {
> + spin_lock_irq(&xhci->lock);
> + xhci_halt(xhci);
> + spin_unlock_irq(&xhci->lock);
> + xhci_cleanup_msix(xhci);
> + }
>
> xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> "xhci_shutdown completed - status = %x",
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 46aa148..d71386b 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1538,6 +1538,7 @@ struct xhci_hcd {
> #define XHCI_COMP_MODE_QUIRK (1 << 14)
> #define XHCI_AVOID_BEI (1 << 15)
> #define XHCI_PLAT (1 << 16)
> +#define XHCI_HSW_SPURIOUS_WAKEUP (1 << 17)
> 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/pci_ids.h b/include/linux/pci_ids.h
> index bc95b2b..1b3ab7d 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2860,7 +2860,9 @@
> #define PCI_DEVICE_ID_INTEL_82454NX 0x84cb
> #define PCI_DEVICE_ID_INTEL_84460GX 0x84ea
> #define PCI_DEVICE_ID_INTEL_IXP4XX 0x8500
> +#define PCI_DEVICE_ID_INTEL_LYNXPOINT_XHCI 0x8c31
> #define PCI_DEVICE_ID_INTEL_IXP2800 0x9004
> +#define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI 0x9c31
> #define PCI_DEVICE_ID_INTEL_S21152BB 0xb152
>
> #define PCI_VENDOR_ID_SCALEMP 0x8686
> --
> 1.8.4
>
--
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
