xhci fixes for usb-linus

PATCH New Mathias Nyman 1 participant 4 messages Aug 25, 2022

Subsystems (3)

Stable branch USB USB xHCI driver

lore

Share & tools

lore

Mathias Nyman

author

2022-08-25 15:07 UTC #0 lore raw

cover xhci fixes for usb-linus

Hi Greg

A few xhci fixes for usb-linus.

USB3 devices aren't visible immediately after xHC reset, so don't stop polling the roothub and suspend too early after xHC reset.

Also Revert the port poweroff patch due to regression, and fix a null pointer issue for xHC hosts with just one roothub.

-Mathias

Mathias Nyman (3):
xhci: Fix null pointer dereference in remove if xHC has only one
roothub
xhci: Add grace period after xHC start to prevent premature runtime
suspend.
Revert "xhci: turn off port power in shutdown"

 drivers/usb/host/xhci-hub.c  | 13 ++++++++++++-
 drivers/usb/host/xhci-plat.c | 11 ++++++++---
 drivers/usb/host/xhci.c      | 19 +++++--------------
 drivers/usb/host/xhci.h      |  4 +---
 4 files changed, 26 insertions(+), 21 deletions(-)

Signature · 1 line

2.25.1

Mathias Nyman

author

2022-08-25 15:07 UTC #1 lore raw

PATCH 1/3 xhci: Fix null pointer dereference in remove if xHC has only one roothub

The remove path in xhci platform driver tries to remove and put both main and shared hcds even if only a main hcd exists (one roothub)

This causes a null pointer dereference in reboot for those controllers.

Check that the shared_hcd exists before trying to remove it.

Fixes e0fe986972f5 ("usb: host: xhci-plat: prepare operation w/o shared hcd")

Reported-by Alexey Sheplyakov <[email protected]>

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/host/xhci-plat.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/usb/host/xhci-plat.c +8 -3

index 044855818cb1..a8641b6536ee 100644
@@ -398,12 +398,17 @@ static int xhci_plat_remove(struct platform_device *dev)
 	pm_runtime_get_sync(&dev->dev);
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
-	usb_remove_hcd(shared_hcd);
-	xhci->shared_hcd = NULL;
+	if (shared_hcd) {
+		usb_remove_hcd(shared_hcd);
+		xhci->shared_hcd = NULL;
+	}
+
 	usb_phy_shutdown(hcd->usb_phy);
 
 	usb_remove_hcd(hcd);
-	usb_put_hcd(shared_hcd);
+
+	if (shared_hcd)
+		usb_put_hcd(shared_hcd);
 
 	clk_disable_unprepare(clk);
 	clk_disable_unprepare(reg_clk);

Signature · 1 line

2.25.1

Mathias Nyman

author

2022-08-25 15:07 UTC #2 lore raw

PATCH 2/3 xhci: Add grace period after xHC start to prevent premature runtime suspend.

In reply to Mathias Nyman

After xHC controller is started, either in probe or resume, it can take a while before any of the connected usb devices are visible to the roothub due to link training.

It's possible xhci driver loads, sees no acivity and suspends the host before the USB device is visible.

In one testcase with a hotplugged xHC controller the host finally detected the connected USB device and generated a wake 500ms after host initial start.

If hosts didn't suspend the device duringe training it probablty wouldn't take up to 500ms to detect it, but looking at specs reveal USB3 link training has a couple long timeout values, such as 120ms RxDetectQuietTimeout, and 360ms PollingLFPSTimeout.

So Add a 500ms grace period that keeps polling the roothub for 500ms after start, preventing runtime suspend until USB devices are detected.

Cc [email protected]

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/host/xhci-hub.c | 11 +++++++++++
 drivers/usb/host/xhci.c     |  4 +++-
 drivers/usb/host/xhci.h     |  2 +-
 3 files changed, 15 insertions(+), 2 deletions(-)

drivers/usb/host/xhci-hub.c +11 -0

index 0fdc014c9401..b30298986a69 100644
@@ -1648,6 +1648,17 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
 
 	status = bus_state->resuming_ports;
 
+	/*
+	 * SS devices are only visible to roothub after link training completes.
+	 * Keep polling roothubs for a grace period after xHC start
+	 */
+	if (xhci->run_graceperiod) {
+		if (time_before(jiffies, xhci->run_graceperiod))
+			status = 1;
+		else
+			xhci->run_graceperiod = 0;
+	}
+
 	mask = PORT_CSC | PORT_PEC | PORT_OCC | PORT_PLC | PORT_WRC | PORT_CEC;
 
 	/* For each port, did anything change?  If so, set that bit in buf. */

drivers/usb/host/xhci.c +3 -1

index 65858f607437..1afd32beec99 100644
@@ -151,9 +151,11 @@ int xhci_start(struct xhci_hcd *xhci)
 		xhci_err(xhci, "Host took too long to start, "
 				"waited %u microseconds.\n",
 				XHCI_MAX_HALT_USEC);
-	if (!ret)
+	if (!ret) {
 		/* clear state flags. Including dying, halted or removing */
 		xhci->xhc_state = 0;
+		xhci->run_graceperiod = jiffies + msecs_to_jiffies(500);
+	}
 
 	return ret;
 }

drivers/usb/host/xhci.h +1 -1

index 1960b47acfb2..df6f2ebaff18 100644
@@ -1826,7 +1826,7 @@ struct xhci_hcd {
 
 	/* Host controller watchdog timer structures */
 	unsigned int		xhc_state;
-
+	unsigned long		run_graceperiod;
 	u32			command;
 	struct s3_save		s3;
 /* Host controller is dying - not responding to commands. "I'm not dead yet!"

Signature · 1 line

2.25.1

Mathias Nyman

author

2022-08-25 15:07 UTC #3 lore raw

PATCH 3/3 Revert "xhci: turn off port power in shutdown"

In reply to Mathias Nyman

This reverts commit 83810f84ecf11dfc5a9414a8b762c3501b328185.

Turning off port power in shutdown did cause issues such as a laptop not proprly powering off, and some specific usb devies failing to enumerate the subsequent boot after a warm reset.

So revert this.

Fixes 83810f84ecf1 ("xhci: turn off port power in shutdown")

Signed-off-by Mathias Nyman <[email protected]>

---
 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci.c     | 15 ++-------------
 drivers/usb/host/xhci.h     |  2 --
 3 files changed, 3 insertions(+), 16 deletions(-)

drivers/usb/host/xhci-hub.c +1 -1

index b30298986a69..4619d5e89d5b 100644
@@ -652,7 +652,7 @@ struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd)
  * It will release and re-aquire the lock while calling ACPI
  * method.
  */
-void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd,
+static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd,
 				u16 index, bool on, unsigned long *flags)
 	__must_hold(&xhci->lock)
 {

drivers/usb/host/xhci.c +2 ~~-13~~

index 1afd32beec99..38649284ff88 100644
@@ -793,8 +793,6 @@ static void xhci_stop(struct usb_hcd *hcd)
 void xhci_shutdown(struct usb_hcd *hcd)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-	unsigned long flags;
-	int i;
 
 	if (xhci->quirks & XHCI_SPURIOUS_REBOOT)
 		usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));

@@ -810,21 +808,12 @@ void xhci_shutdown(struct usb_hcd *hcd)
 		del_timer_sync(&xhci->shared_hcd->rh_timer);
 	}
 
-	spin_lock_irqsave(&xhci->lock, flags);
+	spin_lock_irq(&xhci->lock);
 	xhci_halt(xhci);
-
-	/* Power off USB2 ports*/
-	for (i = 0; i < xhci->usb2_rhub.num_ports; i++)
-		xhci_set_port_power(xhci, xhci->main_hcd, i, false, &flags);
-
-	/* Power off USB3 ports*/
-	for (i = 0; i < xhci->usb3_rhub.num_ports; i++)
-		xhci_set_port_power(xhci, xhci->shared_hcd, i, false, &flags);
-
 	/* Workaround for spurious wakeups at shutdown with HSW */
 	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
 		xhci_reset(xhci, XHCI_RESET_SHORT_USEC);
-	spin_unlock_irqrestore(&xhci->lock, flags);
+	spin_unlock_irq(&xhci->lock);
 
 	xhci_cleanup_msix(xhci);

drivers/usb/host/xhci.h +0 -2

index df6f2ebaff18..7caa0db5e826 100644
@@ -2196,8 +2196,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
 int xhci_hub_status_data(struct usb_hcd *hcd, char *buf);
 int xhci_find_raw_port_number(struct usb_hcd *hcd, int port1);
 struct xhci_hub *xhci_get_rhub(struct usb_hcd *hcd);
-void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, u16 index,
-			 bool on, unsigned long *flags);
 
 void xhci_hc_died(struct xhci_hcd *xhci);

Signature · 1 line

2.25.1