Re: [PATCH 5.10 211/290] xhci: Fix repeated xhci wake after suspend due to uncleared internal wake state

Re: [PATCH 5.10 211/290] xhci: Fix repeated xhci wake after suspend due to uncleared internal wake state

[Pavan Kondeti]

+linux-usb

On Thu, Nov 18, 2021 at 03:21:54PM +0530, Rohith Kollalsi wrote:
> Hi Mathias,
> 
> This mail is regarding your upstream change in xhci file. There is a
> potential issue with your change which can cause noc error due to unclocked
> access.
> 
> Consider a case where a headset is connected to device and nothing is
> playing. As nothing is playing, usb bus wouldn't be active which leads to
> run time suspend ( i think this feature is not present upstream) as a part
> of which xhci_suspend will be called. Now all the clocks are also switched
> off. Now again when pm_suspend occurs due to disconnect, then again
> xhci_suspend occurs, but now registers can be accessed with clocks off
> leading to unclocked access. Can you please consider below change to prevent
> unclocked access.
> 
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> 
> index 2728027..aaf212d 100644
> 
> --- a/drivers/usb/host/xhci.c
> 
> +++ b/drivers/usb/host/xhci.c
> 
> @@ -1013,13 +1013,13 @@ int xhci_suspend(struct xhci_hcd *xhci, bool
> do_wakeup)
> 
> xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> 
>                 return -EINVAL;
> 
> +       if (!HCD_HW_ACCESSIBLE(hcd)) ---> this will prevent unclocked access
> when xhci_suspend occurs during second time during pm_suspend
> 
> + return 0;
> 
> +
> 
>         /* Clear root port wake on bits if wakeup not allowed. */
> 
>         xhci_disable_hub_port_wake(xhci, &xhci->usb3_rhub, do_wakeup);
> 
>         xhci_disable_hub_port_wake(xhci, &xhci->usb2_rhub, do_wakeup);
> 
> -       if (!HCD_HW_ACCESSIBLE(hcd))
> 
> -               return 0;
> 
> -
> 
>         xhci_dbc_suspend(xhci);
> 
> 
> Thanks,
> 
> Rohith

[PATCH 5.10 211/290] xhci: Fix repeated xhci wake after suspend due to uncleared internal wake state

[gregkh]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

From: Mathias Nyman <mathias.nyman@linux.intel.com>

commit d26c00e7276fc92b18c253d69e872f6b03832bad upstream.

If port terminations are detected in suspend, but link never reaches U0
then xHCI may have an internal uncleared wake state that will cause an
immediate wake after suspend.

This wake state is normally cleared when driver clears the PORT_CSC bit,
which is set after a device is enabled and in U0.

Write 1 to clear PORT_CSC for ports that don't have anything connected
when suspending. This makes sure any pending internal wake states in
xHCI are cleared.

Cc: stable@vger.kernel.org
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Link: https://lore.kernel.org/r/20210311115353.2137560-5-mathias.nyman@linux.intel.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/host/xhci.c |   62 +++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -883,44 +883,42 @@ static void xhci_clear_command_ring(stru
 	xhci_set_cmd_ring_deq(xhci);
 }
 
-static void xhci_disable_port_wake_on_bits(struct xhci_hcd *xhci)
+/*
+ * Disable port wake bits if do_wakeup is not set.
+ *
+ * Also clear a possible internal port wake state left hanging for ports that
+ * detected termination but never successfully enumerated (trained to 0U).
+ * Internal wake causes immediate xHCI wake after suspend. PORT_CSC write done
+ * at enumeration clears this wake, force one here as well for unconnected ports
+ */
+
+static void xhci_disable_hub_port_wake(struct xhci_hcd *xhci,
+				       struct xhci_hub *rhub,
+				       bool do_wakeup)
 {
-	struct xhci_port **ports;
-	int port_index;
 	unsigned long flags;
 	u32 t1, t2, portsc;
+	int i;
 
 	spin_lock_irqsave(&xhci->lock, flags);
 
-	/* disable usb3 ports Wake bits */
-	port_index = xhci->usb3_rhub.num_ports;
-	ports = xhci->usb3_rhub.ports;
-	while (port_index--) {
-		t1 = readl(ports[port_index]->addr);
-		portsc = t1;
-		t1 = xhci_port_state_to_neutral(t1);
-		t2 = t1 & ~PORT_WAKE_BITS;
-		if (t1 != t2) {
-			writel(t2, ports[port_index]->addr);
-			xhci_dbg(xhci, "disable wake bits port %d-%d, portsc: 0x%x, write: 0x%x\n",
-				 xhci->usb3_rhub.hcd->self.busnum,
-				 port_index + 1, portsc, t2);
-		}
-	}
+	for (i = 0; i < rhub->num_ports; i++) {
+		portsc = readl(rhub->ports[i]->addr);
+		t1 = xhci_port_state_to_neutral(portsc);
+		t2 = t1;
+
+		/* clear wake bits if do_wake is not set */
+		if (!do_wakeup)
+			t2 &= ~PORT_WAKE_BITS;
+
+		/* Don't touch csc bit if connected or connect change is set */
+		if (!(portsc & (PORT_CSC | PORT_CONNECT)))
+			t2 |= PORT_CSC;
 
-	/* disable usb2 ports Wake bits */
-	port_index = xhci->usb2_rhub.num_ports;
-	ports = xhci->usb2_rhub.ports;
-	while (port_index--) {
-		t1 = readl(ports[port_index]->addr);
-		portsc = t1;
-		t1 = xhci_port_state_to_neutral(t1);
-		t2 = t1 & ~PORT_WAKE_BITS;
 		if (t1 != t2) {
-			writel(t2, ports[port_index]->addr);
-			xhci_dbg(xhci, "disable wake bits port %d-%d, portsc: 0x%x, write: 0x%x\n",
-				 xhci->usb2_rhub.hcd->self.busnum,
-				 port_index + 1, portsc, t2);
+			writel(t2, rhub->ports[i]->addr);
+			xhci_dbg(xhci, "config port %d-%d wake bits, portsc: 0x%x, write: 0x%x\n",
+				 rhub->hcd->self.busnum, i + 1, portsc, t2);
 		}
 	}
 	spin_unlock_irqrestore(&xhci->lock, flags);
@@ -983,8 +981,8 @@ int xhci_suspend(struct xhci_hcd *xhci,
 		return -EINVAL;
 
 	/* Clear root port wake on bits if wakeup not allowed. */
-	if (!do_wakeup)
-		xhci_disable_port_wake_on_bits(xhci);
+	xhci_disable_hub_port_wake(xhci, &xhci->usb3_rhub, do_wakeup);
+	xhci_disable_hub_port_wake(xhci, &xhci->usb2_rhub, do_wakeup);
 
 	if (!HCD_HW_ACCESSIBLE(hcd))
 		return 0;