[PATCH 1/7] usb: xhci: applying XHCI_PME_STUCK_QUIRK to Intel BXT B0 host

[PATCH 1/7] usb: xhci: applying XHCI_PME_STUCK_QUIRK to Intel BXT B0 host

[Mathias Nyman]

From: Rafal Redzimski <rafal.f.redzimski@intel.com>

Broxton B0 also requires XHCI_PME_STUCK_QUIRK.
Adding PCI device ID for Broxton B and adding to quirk.

Cc: <stable@vger.kernel.org>
Signed-off-by: Rafal Redzimski <rafal.f.redzimski@intel.com>
Signed-off-by: Robert Dobrowolski <robert.dobrowolski@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index f0640b7..071b34a 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -48,6 +48,7 @@
 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_XHCI		0xa12f
 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI	0x9d2f
 #define PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI		0x0aa8
+#define PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI		0x1aa8
 
 static const char hcd_name[] = "xhci_hcd";
 
@@ -155,7 +156,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		(pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI ||
 		 pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_XHCI ||
 		 pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
-		 pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI)) {
+		 pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI ||
+		 pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI)) {
 		xhci->quirks |= XHCI_PME_STUCK_QUIRK;
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
-- 
1.9.1

[PATCH 2/7] xhci: resume USB 3 roothub first

[Mathias Nyman]

Give USB3 devices a better chance to enumerate at USB 3 speeds if
they are connected to a suspended host.
Solves an issue with NEC uPD720200 host hanging when partially
enumerating a USB3 device as USB2 after host controller runtime resume.

Cc: <stable@vger.kernel.org>
Tested-by: Mike Murdoch <main.haarp@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d51ee0c..b609288 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1108,8 +1108,8 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		/* Resume root hubs only when have pending events. */
 		status = readl(&xhci->op_regs->status);
 		if (status & STS_EINT) {
-			usb_hcd_resume_root_hub(hcd);
 			usb_hcd_resume_root_hub(xhci->shared_hcd);
+			usb_hcd_resume_root_hub(hcd);
 		}
 	}
 
@@ -1124,10 +1124,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 
 	/* Re-enable port polling. */
 	xhci_dbg(xhci, "%s: starting port polling.\n", __func__);
-	set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
-	usb_hcd_poll_rh_status(hcd);
 	set_bit(HCD_FLAG_POLL_RH, &xhci->shared_hcd->flags);
 	usb_hcd_poll_rh_status(xhci->shared_hcd);
+	set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
+	usb_hcd_poll_rh_status(hcd);
 
 	return retval;
 }
-- 
1.9.1

[PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Mathias Nyman]

We don't want to runtime suspend a bus if there is an event pending.
The roothub on a NEC uPD720200 host with a single USB3 device connected
might go back to runtime suspend immediately after runtime resume as
hub might not yet see any port changes in resume.

Prevent this by checking if there is a unhandled event pending when
calling runtime suspend.

Cc: <stable@vger.kernel.org>
Tested-by: Mike Murdoch <main.haarp@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c | 6 ++++++
 drivers/usb/host/xhci.c     | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index d61fcc4..7e8999a 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 	__le32 __iomem **port_array;
 	struct xhci_bus_state *bus_state;
 	unsigned long flags;
+	u32 status;
 
 	max_ports = xhci_get_ports(hcd, &port_array);
 	bus_state = &xhci->bus_state[hcd_index(hcd)];
 
 	spin_lock_irqsave(&xhci->lock, flags);
 
+	/* Don't suspend root hub if there's an event pending. */
+	status = readl(&xhci->op_regs->status);
+	if (status & STS_EINT)
+		return -EBUSY;
+
 	if (hcd->self.root_hub->do_remote_wakeup) {
 		if (bus_state->resuming_ports ||	/* USB2 */
 		    bus_state->port_remote_wakeup) {	/* USB3 */
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b609288..16366a9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1107,6 +1107,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 	if (retval == 0) {
 		/* Resume root hubs only when have pending events. */
 		status = readl(&xhci->op_regs->status);
+		xhci_dbg(xhci, "xhci_resume status reg = 0x%x\n", status);
 		if (status & STS_EINT) {
 			usb_hcd_resume_root_hub(xhci->shared_hcd);
 			usb_hcd_resume_root_hub(hcd);
-- 
1.9.1

[PATCH 4/7] usb: host: xhci: add a new quirk XHCI_NO_64BIT_SUPPORT

[Mathias Nyman]

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

On some xHCI controllers (e.g. R-Car SoCs), the AC64 bit (bit 0) of
HCCPARAMS1 is set to 1. However, the xHCs don't support 64-bit
address memory pointers actually. So, in this case, this driver should
call dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) in xhci_gen_setup().
Otherwise, the xHCI controller will be died after a usb device is
connected if it runs on above 4GB physical memory environment.

So, this patch adds a new quirk XHCI_NO_64BIT_SUPPORT to resolve
such an issue.

Cc: <stable@vger.kernel.org>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.c | 10 ++++++++++
 drivers/usb/host/xhci.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 16366a9..31f73ed 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4949,6 +4949,16 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 		return retval;
 	xhci_dbg(xhci, "Reset complete\n");
 
+	/*
+	 * On some xHCI controllers (e.g. R-Car SoCs), the AC64 bit (bit 0)
+	 * of HCCPARAMS1 is set to 1. However, the xHCs don't support 64-bit
+	 * address memory pointers actually. So, this driver clears the AC64
+	 * bit of xhci->hcc_params to call dma_set_coherent_mask(dev,
+	 * DMA_BIT_MASK(32)) in this xhci_gen_setup().
+	 */
+	if (xhci->quirks & XHCI_NO_64BIT_SUPPORT)
+		xhci->hcc_params &= ~BIT(0);
+
 	/* Set dma_mask and coherent_dma_mask to 64-bits,
 	 * if xHC supports 64-bit addressing */
 	if (HCC_64BIT_ADDR(xhci->hcc_params) &&
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e293e09..70f215c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1641,6 +1641,7 @@ struct xhci_hcd {
 #define XHCI_PME_STUCK_QUIRK	(1 << 20)
 #define XHCI_MTK_HOST		(1 << 21)
 #define XHCI_SSIC_PORT_UNUSED	(1 << 22)
+#define XHCI_NO_64BIT_SUPPORT	(1 << 23)
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
 	/* There are two roothubs to keep track of bus suspend info for */
-- 
1.9.1

[PATCH 5/7] usb: host: xhci-plat: fix cannot work if R-Car Gen2/3 run on above 4GB phys

[Mathias Nyman]

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

This patch fixes an issue that cannot work if R-Car Gen2/3 run on
above 4GB physical memory environment to use a quirk XHCI_NO_64BIT_SUPPORT.

Cc: <stable@vger.kernel.org>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-plat.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 5c15e9b..474b5fa 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -39,12 +39,25 @@ static const struct xhci_driver_overrides xhci_plat_overrides __initconst = {
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {
+	struct usb_hcd *hcd = xhci_to_hcd(xhci);
+
 	/*
 	 * As of now platform drivers don't provide MSI support so we ensure
 	 * 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_PLAT;
+
+	/*
+	 * On R-Car Gen2 and Gen3, the AC64 bit (bit 0) of HCCPARAMS1 is set
+	 * to 1. However, these SoCs don't support 64-bit address memory
+	 * pointers. So, this driver clears the AC64 bit of xhci->hcc_params
+	 * to call dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) in
+	 * xhci_gen_setup().
+	 */
+	if (xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2) ||
+	    xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3))
+		xhci->quirks |= XHCI_NO_64BIT_SUPPORT;
 }
 
 /* called during probe() after chip reset completes */
-- 
1.9.1

[PATCH 6/7] usb: xhci: fix wild pointers in xhci_mem_cleanup

[Mathias Nyman]

From: Lu Baolu <baolu.lu@linux.intel.com>

This patch fixes some wild pointers produced by xhci_mem_cleanup.
These wild pointers will cause system crash if xhci_mem_cleanup()
is called twice.

Reported-and-tested-by: Pengcheng Li <lpc.li@hisilicon.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 80c1de2..bad0d1f 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1861,6 +1861,12 @@ no_bw:
 	kfree(xhci->rh_bw);
 	kfree(xhci->ext_caps);
 
+	xhci->usb2_ports = NULL;
+	xhci->usb3_ports = NULL;
+	xhci->port_array = NULL;
+	xhci->rh_bw = NULL;
+	xhci->ext_caps = NULL;
+
 	xhci->page_size = 0;
 	xhci->page_shift = 0;
 	xhci->bus_state[0].bus_suspended = 0;
-- 
1.9.1

[PATCH 7/7] xhci: fix 10 second timeout on removal of PCI hotpluggable xhci controllers

[Mathias Nyman]

PCI hotpluggable xhci controllers such as some Alpine Ridge solutions will
remove the xhci controller from the PCI bus when the last USB device is
disconnected.

Add a flag to indicate that the host is being removed to avoid queueing
configure_endpoint commands for the dropped endpoints.
For PCI hotplugged controllers this will prevent 5 second command timeouts
For static xhci controllers the configure_endpoint command is not needed
in the removal case as everything will be returned, freed, and the
controller is reset.

For now the flag is only set for PCI connected host controllers.

Cc: <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c  | 1 +
 drivers/usb/host/xhci-ring.c | 3 ++-
 drivers/usb/host/xhci.c      | 8 +++++---
 drivers/usb/host/xhci.h      | 1 +
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 071b34a..48672fa 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -304,6 +304,7 @@ static void xhci_pci_remove(struct pci_dev *dev)
 	struct xhci_hcd *xhci;
 
 	xhci = hcd_to_xhci(pci_get_drvdata(dev));
+	xhci->xhc_state |= XHCI_STATE_REMOVING;
 	if (xhci->shared_hcd) {
 		usb_remove_hcd(xhci->shared_hcd);
 		usb_put_hcd(xhci->shared_hcd);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7cf6621..99b4ff4 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -4004,7 +4004,8 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
 	int reserved_trbs = xhci->cmd_ring_reserved_trbs;
 	int ret;
 
-	if (xhci->xhc_state) {
+	if ((xhci->xhc_state & XHCI_STATE_DYING) ||
+		(xhci->xhc_state & XHCI_STATE_HALTED)) {
 		xhci_dbg(xhci, "xHCI dying or halted, can't queue_command\n");
 		return -ESHUTDOWN;
 	}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 31f73ed..679266b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -147,7 +147,8 @@ static int xhci_start(struct xhci_hcd *xhci)
 				"waited %u microseconds.\n",
 				XHCI_MAX_HALT_USEC);
 	if (!ret)
-		xhci->xhc_state &= ~(XHCI_STATE_HALTED | XHCI_STATE_DYING);
+		/* clear state flags. Including dying, halted or removing */
+		xhci->xhc_state = 0;
 
 	return ret;
 }
@@ -2774,7 +2775,8 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
 	if (ret <= 0)
 		return ret;
 	xhci = hcd_to_xhci(hcd);
-	if (xhci->xhc_state & XHCI_STATE_DYING)
+	if ((xhci->xhc_state & XHCI_STATE_DYING) ||
+		(xhci->xhc_state & XHCI_STATE_REMOVING))
 		return -ENODEV;
 
 	xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
@@ -3821,7 +3823,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 
 	mutex_lock(&xhci->mutex);
 
-	if (xhci->xhc_state)	/* dying or halted */
+	if (xhci->xhc_state)	/* dying, removing or halted */
 		goto out;
 
 	if (!udev->slot_id) {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 70f215c..6c629c9 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1605,6 +1605,7 @@ struct xhci_hcd {
  */
 #define XHCI_STATE_DYING	(1 << 0)
 #define XHCI_STATE_HALTED	(1 << 1)
+#define XHCI_STATE_REMOVING	(1 << 2)
 	/* Statistics */
 	int			error_bitmask;
 	unsigned int		quirks;
-- 
1.9.1

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Alan Stern]

On Wed, 6 Apr 2016, Mathias Nyman wrote:

> We don't want to runtime suspend a bus if there is an event pending.
> The roothub on a NEC uPD720200 host with a single USB3 device connected
> might go back to runtime suspend immediately after runtime resume as
> hub might not yet see any port changes in resume.
> 
> Prevent this by checking if there is a unhandled event pending when
> calling runtime suspend.
> 
> Cc: <stable@vger.kernel.org>
> Tested-by: Mike Murdoch <main.haarp@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-hub.c | 6 ++++++
>  drivers/usb/host/xhci.c     | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index d61fcc4..7e8999a 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>  	__le32 __iomem **port_array;
>  	struct xhci_bus_state *bus_state;
>  	unsigned long flags;
> +	u32 status;
>  
>  	max_ports = xhci_get_ports(hcd, &port_array);
>  	bus_state = &xhci->bus_state[hcd_index(hcd)];
>  
>  	spin_lock_irqsave(&xhci->lock, flags);
>  
> +	/* Don't suspend root hub if there's an event pending. */
> +	status = readl(&xhci->op_regs->status);
> +	if (status & STS_EINT)
> +		return -EBUSY;

Does anybody else see a problem here?

Alan Stern

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Bjørn Mork]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Wed, 6 Apr 2016, Mathias Nyman wrote:
>
>> We don't want to runtime suspend a bus if there is an event pending.
>> The roothub on a NEC uPD720200 host with a single USB3 device connected
>> might go back to runtime suspend immediately after runtime resume as
>> hub might not yet see any port changes in resume.
>> 
>> Prevent this by checking if there is a unhandled event pending when
>> calling runtime suspend.
>> 
>> Cc: <stable@vger.kernel.org>
>> Tested-by: Mike Murdoch <main.haarp@gmail.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>  drivers/usb/host/xhci-hub.c | 6 ++++++
>>  drivers/usb/host/xhci.c     | 1 +
>>  2 files changed, 7 insertions(+)
>> 
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index d61fcc4..7e8999a 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>>  	__le32 __iomem **port_array;
>>  	struct xhci_bus_state *bus_state;
>>  	unsigned long flags;
>> +	u32 status;
>>  
>>  	max_ports = xhci_get_ports(hcd, &port_array);
>>  	bus_state = &xhci->bus_state[hcd_index(hcd)];
>>  
>>  	spin_lock_irqsave(&xhci->lock, flags);
>>  
>> +	/* Don't suspend root hub if there's an event pending. */
>> +	status = readl(&xhci->op_regs->status);
>> +	if (status & STS_EINT)
>> +		return -EBUSY;
>
> Does anybody else see a problem here?

Like the EBUSY being propagated all the way to usb_suspend(), which is
called on PMSG_HIBERNATE, PMSG_FREEZE and PMSG_SUSPEND?

This will not only prevent runtime suspend, but also system suspend,
wont it?  Or did I miss something on the way?  The call chain between
the USB dev_pm_ops .suspend and xhci_bus_suspend() is quite long..


Bjørn

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Alan Stern]

On Wed, 6 Apr 2016, Bjørn Mork wrote:

> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > On Wed, 6 Apr 2016, Mathias Nyman wrote:
> >
> >> We don't want to runtime suspend a bus if there is an event pending.
> >> The roothub on a NEC uPD720200 host with a single USB3 device connected
> >> might go back to runtime suspend immediately after runtime resume as
> >> hub might not yet see any port changes in resume.
> >> 
> >> Prevent this by checking if there is a unhandled event pending when
> >> calling runtime suspend.
> >> 
> >> Cc: <stable@vger.kernel.org>
> >> Tested-by: Mike Murdoch <main.haarp@gmail.com>
> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> >> ---
> >>  drivers/usb/host/xhci-hub.c | 6 ++++++
> >>  drivers/usb/host/xhci.c     | 1 +
> >>  2 files changed, 7 insertions(+)
> >> 
> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> >> index d61fcc4..7e8999a 100644
> >> --- a/drivers/usb/host/xhci-hub.c
> >> +++ b/drivers/usb/host/xhci-hub.c
> >> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
> >>  	__le32 __iomem **port_array;
> >>  	struct xhci_bus_state *bus_state;
> >>  	unsigned long flags;
> >> +	u32 status;
> >>  
> >>  	max_ports = xhci_get_ports(hcd, &port_array);
> >>  	bus_state = &xhci->bus_state[hcd_index(hcd)];
> >>  
> >>  	spin_lock_irqsave(&xhci->lock, flags);
> >>  
> >> +	/* Don't suspend root hub if there's an event pending. */
> >> +	status = readl(&xhci->op_regs->status);
> >> +	if (status & STS_EINT)
> >> +		return -EBUSY;
> >
> > Does anybody else see a problem here?
> 
> Like the EBUSY being propagated all the way to usb_suspend(), which is
> called on PMSG_HIBERNATE, PMSG_FREEZE and PMSG_SUSPEND?
> 
> This will not only prevent runtime suspend, but also system suspend,
> wont it?  Or did I miss something on the way?  The call chain between
> the USB dev_pm_ops .suspend and xhci_bus_suspend() is quite long..

I was thinking of returning with a private spinlock held.

Alan Stern

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Bjørn Mork]

Alan Stern <stern@rowland.harvard.edu> writes:
> On Wed, 6 Apr 2016, Bjørn Mork wrote:
>> Alan Stern <stern@rowland.harvard.edu> writes:
>> > On Wed, 6 Apr 2016, Mathias Nyman wrote:
>> >
>> >> We don't want to runtime suspend a bus if there is an event pending.
>> >> The roothub on a NEC uPD720200 host with a single USB3 device connected
>> >> might go back to runtime suspend immediately after runtime resume as
>> >> hub might not yet see any port changes in resume.
>> >> 
>> >> Prevent this by checking if there is a unhandled event pending when
>> >> calling runtime suspend.
>> >> 
>> >> Cc: <stable@vger.kernel.org>
>> >> Tested-by: Mike Murdoch <main.haarp@gmail.com>
>> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> >> ---
>> >>  drivers/usb/host/xhci-hub.c | 6 ++++++
>> >>  drivers/usb/host/xhci.c     | 1 +
>> >>  2 files changed, 7 insertions(+)
>> >> 
>> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> >> index d61fcc4..7e8999a 100644
>> >> --- a/drivers/usb/host/xhci-hub.c
>> >> +++ b/drivers/usb/host/xhci-hub.c
>> >> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>> >>  	__le32 __iomem **port_array;
>> >>  	struct xhci_bus_state *bus_state;
>> >>  	unsigned long flags;
>> >> +	u32 status;
>> >>  
>> >>  	max_ports = xhci_get_ports(hcd, &port_array);
>> >>  	bus_state = &xhci->bus_state[hcd_index(hcd)];
>> >>  
>> >>  	spin_lock_irqsave(&xhci->lock, flags);
>> >>  
>> >> +	/* Don't suspend root hub if there's an event pending. */
>> >> +	status = readl(&xhci->op_regs->status);
>> >> +	if (status & STS_EINT)
>> >> +		return -EBUSY;
>> >
>> > Does anybody else see a problem here?
>> 
>> Like the EBUSY being propagated all the way to usb_suspend(), which is
>> called on PMSG_HIBERNATE, PMSG_FREEZE and PMSG_SUSPEND?
>> 
>> This will not only prevent runtime suspend, but also system suspend,
>> wont it?  Or did I miss something on the way?  The call chain between
>> the USB dev_pm_ops .suspend and xhci_bus_suspend() is quite long..
>
> I was thinking of returning with a private spinlock held.

No, I didn't see that. So I am happy to support Mathias' case by
demonstrating that it wasn't *that* bloody obvious :)


Bjørn

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Greg KH]

On Wed, Apr 06, 2016 at 04:08:04PM +0300, Mathias Nyman wrote:
> We don't want to runtime suspend a bus if there is an event pending.
> The roothub on a NEC uPD720200 host with a single USB3 device connected
> might go back to runtime suspend immediately after runtime resume as
> hub might not yet see any port changes in resume.
> 
> Prevent this by checking if there is a unhandled event pending when
> calling runtime suspend.

As Alan points out, you didn't actually test this :(

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Alan Stern]

On Wed, 6 Apr 2016, Greg KH wrote:

> On Wed, Apr 06, 2016 at 04:08:04PM +0300, Mathias Nyman wrote:
> > We don't want to runtime suspend a bus if there is an event pending.
> > The roothub on a NEC uPD720200 host with a single USB3 device connected
> > might go back to runtime suspend immediately after runtime resume as
> > hub might not yet see any port changes in resume.
> > 
> > Prevent this by checking if there is a unhandled event pending when
> > calling runtime suspend.
> 
> As Alan points out, you didn't actually test this :(

Also as Björn pointed out, failing a system suspend isn't a good idea.  
In general, you should do it only if wakeup is enabled and there is a 
pending wakeup event.

In this case there may be alternatives.  For example, you could just 
delay a few ms until the pending event has been handled.  Or, if you 
really just want to prevent runtime suspend, you should check to see if 
this was a runtime suspend and not a system suspend.  Or you could 
prevent the bus from being runtime suspended in the first place by 
doing a pm_runtime_get_sync().

Alan Stern

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Mathias Nyman]

On 07.04.2016 00:25, Alan Stern wrote:
> On Wed, 6 Apr 2016, Greg KH wrote:
>
>> On Wed, Apr 06, 2016 at 04:08:04PM +0300, Mathias Nyman wrote:
>>> We don't want to runtime suspend a bus if there is an event pending.
>>> The roothub on a NEC uPD720200 host with a single USB3 device connected
>>> might go back to runtime suspend immediately after runtime resume as
>>> hub might not yet see any port changes in resume.
>>>
>>> Prevent this by checking if there is a unhandled event pending when
>>> calling runtime suspend.
>>
>> As Alan points out, you didn't actually test this :(

No, I couldn't trigger this case at all.

Patches 2/7 and 3/7 were based on logs of failing cases seen by Mike Murdoch
on hist NEC host. These two patches solved it for him.
I'm guessing patch 2/7 changed the situation enough to not trigger the 3/7
case anymore, and it was really never exercised.

http://marc.info/?l=linux-usb&m=145477850706552&w=2

But I got no excuses. I really should have spotted that lock.
I'm glad Alan found it.

>
> Also as Björn pointed out, failing a system suspend isn't a good idea.
> In general, you should do it only if wakeup is enabled and there is a
> pending wakeup event.
>
> In this case there may be alternatives.  For example, you could just
> delay a few ms until the pending event has been handled.  Or, if you
> really just want to prevent runtime suspend, you should check to see if
> this was a runtime suspend and not a system suspend.  Or you could
> prevent the bus from being runtime suspended in the first place by
> doing a pm_runtime_get_sync().

I just want to prevent runtime suspend.
The pm_message_t msg is lost when hcd_bus_suspend() calls hcd->driver->bus_suspend(hcd).
Maybe it could be added?
Or is there some other easy way to figure out if it is runtime suspend?

-Mathias
  

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Alan Stern]

On Thu, 7 Apr 2016, Mathias Nyman wrote:

> > In this case there may be alternatives.  For example, you could just
> > delay a few ms until the pending event has been handled.  Or, if you
> > really just want to prevent runtime suspend, you should check to see if
> > this was a runtime suspend and not a system suspend.  Or you could
> > prevent the bus from being runtime suspended in the first place by
> > doing a pm_runtime_get_sync().
> 
> I just want to prevent runtime suspend.
> The pm_message_t msg is lost when hcd_bus_suspend() calls hcd->driver->bus_suspend(hcd).
> Maybe it could be added?

It could.  You'd just have to update all the existing callback routines 
to accept the additional argument.

> Or is there some other easy way to figure out if it is runtime suspend?

There isn't.  But I still think you might be better off using the 
existing runtime PM facilities to prevent runtime suspend at bad times 
-- if possible.

The commit message said something about waiting for a newly discovered 
hub to be probed.  Can you go into more detail?

Alan Stern

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Mathias Nyman]

On 07.04.2016 17:23, Alan Stern wrote:
> On Thu, 7 Apr 2016, Mathias Nyman wrote:
>
>>> In this case there may be alternatives.  For example, you could just
>>> delay a few ms until the pending event has been handled.  Or, if you
>>> really just want to prevent runtime suspend, you should check to see if
>>> this was a runtime suspend and not a system suspend.  Or you could
>>> prevent the bus from being runtime suspended in the first place by
>>> doing a pm_runtime_get_sync().
>>
>> I just want to prevent runtime suspend.
>> The pm_message_t msg is lost when hcd_bus_suspend() calls hcd->driver->bus_suspend(hcd).
>> Maybe it could be added?
>
> It could.  You'd just have to update all the existing callback routines
> to accept the additional argument.
>
>> Or is there some other easy way to figure out if it is runtime suspend?
>
> There isn't.  But I still think you might be better off using the
> existing runtime PM facilities to prevent runtime suspend at bad times
> -- if possible.
>
> The commit message said something about waiting for a newly discovered
> hub to be probed.  Can you go into more detail?


A USB3 device connected to a runtime suspended host woke up the host, polled the roothub,
found nothing and immediately tried to suspend again.

Logs show:
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: // Setting command ring address to 0xffffe001
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: xhci_resume: starting port polling.
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: xhci_hub_status_data: stopping port polling.
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: xhci_suspend: stopping port polling.
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: Port Status Change Event for port 3
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: resume root hub
..other entries..
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: Port Status Change Event for port 1
Feb 16 20:03:33 xhci_hcd 0000:0e:00.0: resume root hub


So what probably happened here was that connecting the usb device caused a
PCI PME# event to wake up the xhci PCI controller, calling
   xhci_pci_resume(), calling
     xhci_resume(), calling
       usb_hcd_resume_root_hub(both usb2 and usb3 hcds) and
       usb_hcd_poll_rh_status(both usb2 and usb3 hcd). polling rh status calls
         hcd->driver->hub_status_data() -> xhci_hub_status_data()
         xhci_hub_status_data() reads PORTSCs -> no change -> stop polling -> return 0
     
In core/hub.c hub_probe() autosuspend is set to 0 for roothubs with a long explanation why.
I assume that as xhci_hub_status_data clears the HCD_FLAG_POLL_RH flag and returns 0 (no changes)
we immediately runtime suspend the hub -> runtime suspend host.

-Mathias