Skip to contentSkip to conversation

usb: xhci: Free the right ring in xhci_add_endpoint()

PATCH In review Mathias Nyman 2 participants 9 messages Sep 18, 2017 1 message not in the archive

Subsystems (3)
Stable branch USB USB xHCI driver
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mathias Nyman

author
#1 lore raw

PATCH 1/8 usb: xhci: Free the right ring in xhci_add_endpoint()

From: Lu Baolu <[email protected]>

In the xhci_add_endpoint(), a new ring was allocated and saved at xhci_virt_ep->new_ring. Hence, when error happens, we need to free the allocated ring before returning error.

Current code frees xhci_virt_ep->ring instead of the new_ring. This patch fixes this.

Cc <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/xhci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/host/xhci.c +2 -1
index b2ff1ff..ee198ea 100644
@@ -1703,7 +1703,8 @@ static int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
 	if (xhci->quirks & XHCI_MTK_HOST) {
 		ret = xhci_mtk_add_ep_quirk(hcd, udev, ep);
 		if (ret < 0) {
-			xhci_free_endpoint_ring(xhci, virt_dev, ep_index);
+			xhci_ring_free(xhci, virt_dev->eps[ep_index].new_ring);
+			virt_dev->eps[ep_index].new_ring = NULL;
 			return ret;
 		}
 	}
Signature · 1 line
1.9.1
author
#2 lore raw

PATCH 2/8 xhci: fix finding correct bus_state structure for USB 3.1 hosts

In reply to a message not in the archive

xhci driver keeps a bus_state structure for each hcd (usb2 and usb3)

The structure is picked based on hcd speed, but driver only compared for HCD_USB3 speed, returning the wrong bus_state for HCD_USB31 hosts.

This caused null pointer dereference errors in bus_resume function.

Cc <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/xhci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/host/xhci.h +1 -1
index 2abaa4d..e96e90d 100644
@@ -1681,7 +1681,7 @@ struct xhci_bus_state {
 
 static inline unsigned int hcd_index(struct usb_hcd *hcd)
 {
-	if (hcd->speed == HCD_USB3)
+	if (hcd->speed >= HCD_USB3)
 		return 0;
 	else
 		return 1;
Signature · 1 line
1.9.1
author
#3 lore raw

PATCH 3/8 usb: pci-quirks.c: Corrected timeout values used in handshake

In reply to a message not in the archive

From: Jim Dickerson <[email protected]>

Servers were emitting failed handoff messages but were not waiting the full 1 second as designated in section 4.22.1 of the eXtensible Host Controller Interface specifications. The handshake was using wrong units so calls were made with milliseconds not microseconds. Comments referenced 5 seconds not 1 second as in specs.

The wrong units were also corrected in a second handshake call.

Cc <[email protected]>
Signed-off-by Jim Dickerson <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/pci-quirks.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/host/pci-quirks.c +4 -4
index 658d9d1..e02dbb1 100644
@@ -1022,7 +1022,7 @@ void usb_disable_xhci_ports(struct pci_dev *xhci_pdev)
  *
  * Takes care of the handoff between the Pre-OS (i.e. BIOS) and the OS.
  * It signals to the BIOS that the OS wants control of the host controller,
- * and then waits 5 seconds for the BIOS to hand over control.
+ * and then waits 1 second for the BIOS to hand over control.
  * If we timeout, assume the BIOS is broken and take control anyway.
  */
 static void quirk_usb_handoff_xhci(struct pci_dev *pdev)

@@ -1069,9 +1069,9 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	if (val & XHCI_HC_BIOS_OWNED) {
 		writel(val | XHCI_HC_OS_OWNED, base + ext_cap_offset);
 
-		/* Wait for 5 seconds with 10 microsecond polling interval */
+		/* Wait for 1 second with 10 microsecond polling interval */
 		timeout = handshake(base + ext_cap_offset, XHCI_HC_BIOS_OWNED,
-				0, 5000, 10);
+				0, 1000000, 10);
 
 		/* Assume a buggy BIOS and take HC ownership anyway */
 		if (timeout) {

@@ -1100,7 +1100,7 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	 * operational or runtime registers.  Wait 5 seconds and no more.
 	 */
 	timeout = handshake(op_reg_base + XHCI_STS_OFFSET, XHCI_STS_CNR, 0,
-			5000, 10);
+			5000000, 10);
 	/* Assume a buggy HC and start HC initialization anyway */
 	if (timeout) {
 		val = readl(op_reg_base + XHCI_STS_OFFSET);
Signature · 1 line
1.9.1
author
#4 lore raw

PATCH 4/8 xhci: fix wrong endpoint ESIT value shown in tracing

In reply to a message not in the archive

Read the endpiont ESIT from endpiont context using correct macro. Add a macro for reading the high bits of ESIT for Large ESIT Payload Capable hosts (LEC=1)

Cc <[email protected]> # 4.12
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/xhci.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/usb/host/xhci.h +4 -2
index e96e90d..7c87189 100644
@@ -735,6 +735,8 @@ struct xhci_ep_ctx {
 #define EP_MAXPSTREAMS(p)	(((p) << 10) & EP_MAXPSTREAMS_MASK)
 /* Endpoint is set up with a Linear Stream Array (vs. Secondary Stream Array) */
 #define	EP_HAS_LSA		(1 << 15)
+/* hosts with LEC=1 use bits 31:24 as ESIT high bits. */
+#define CTX_TO_MAX_ESIT_PAYLOAD_HI(p)	(((p) >> 24) & 0xff)
 
 /* ep_info2 bitmasks */
 /*

@@ -2540,8 +2542,8 @@ static inline const char *xhci_decode_ep_context(u32 info, u32 info2, u64 deq,
 	u8 lsa;
 	u8 hid;
 
-	esit = EP_MAX_ESIT_PAYLOAD_HI(info) << 16 |
-		EP_MAX_ESIT_PAYLOAD_LO(tx_info);
+	esit = CTX_TO_MAX_ESIT_PAYLOAD_HI(info) << 16 |
+		CTX_TO_MAX_ESIT_PAYLOAD(tx_info);
 
 	ep_state = info & EP_STATE_MASK;
 	max_pstr = info & EP_MAXPSTREAMS_MASK;
Signature · 1 line
1.9.1
author
#5 lore raw

PATCH 5/8 usb: host: xhci-plat: allow sysdev to inherit from ACPI

In reply to a message not in the archive

From: Adam Wallis <[email protected]>

Commit 4c39d4b949d3 ("usb: xhci: use bus->sysdev for DMA configuration") updated the method determining DMA for XHCI from sysdev. However, this patch broke the ability to enumerate the FWNODE from parent ACPI devices from the child plat XHCI device.

Currently, xhci_plat is not set up properly when the parent device is an ACPI node. The conditions that xhci_plat_probe should satisfy are

1. xhci_plat comes from firmware
2. xhci_plat is child of a device from firmware (dwc3-plat)
3. xhci_plat is grandchild of a pci device (dwc3-pci)

Case 2 is covered when the child is an OF node (by checking sysdev->parent->of_node), however, an ACPI parent will return NULL in the of_node check and will thus not result in sysdev being set to sysdev->parent

[   17.591549] xhci-hcd: probe of xhci-hcd.6.auto failed with error -5

This change adds a check for ACPI to completely allow for condition 2. This is done by first checking if the parent node is of type ACPI (e.g., dwc3-plat) and set sysdev to sysdev->parent if either of the two following conditions are met:

1: If fwnode is empty (in the case that platform_device_add_properties was not called on the allocated platform device)
2: fwnode exists but is not of type ACPI (this would happen if platform_device_add_properties was called on the allocated device. Instead of type FWNODE_ACPI, you would end up with FWNODE_PDATA)

Cc [email protected] #4.12.x
Cc [email protected] #4.13.x
Fixes 4c39d4b949d3 ("usb: xhci: use bus->sysdev for DMA configuration")
Tested-by Thang Q. Nguyen <[email protected]>
Signed-off-by Adam Wallis <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/xhci-plat.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/usb/host/xhci-plat.c +10 -6
index 163bafd..1cb6eae 100644
@@ -178,14 +178,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	 * 2. xhci_plat is child of a device from firmware (dwc3-plat)
 	 * 3. xhci_plat is grandchild of a pci device (dwc3-pci)
 	 */
-	sysdev = &pdev->dev;
-	if (sysdev->parent && !sysdev->of_node && sysdev->parent->of_node)
-		sysdev = sysdev->parent;
+	for (sysdev = &pdev->dev; sysdev; sysdev = sysdev->parent) {
+		if (is_of_node(sysdev->fwnode) ||
+			is_acpi_device_node(sysdev->fwnode))
+			break;
 #ifdef CONFIG_PCI
-	else if (sysdev->parent && sysdev->parent->parent &&
-		 sysdev->parent->parent->bus == &pci_bus_type)
-		sysdev = sysdev->parent->parent;
+		else if (sysdev->bus == &pci_bus_type)
+			break;
 #endif
+	}
+
+	if (!sysdev)
+		sysdev = &pdev->dev;
 
 	/* Try to set 64-bit DMA first */
 	if (WARN_ON(!sysdev->dma_mask))
Signature · 1 line
1.9.1
author
#6 lore raw

PATCH 6/8 xhci: Fix sleeping with spin_lock_irq() held in ASmedia 1042A workaround

In reply to a message not in the archive

The flow control workaround for ASM1042A xHC hosts sleeps between register polling. The workaround gets called in several places, among them with spin_lock_irq() held when xHC host is resumed or hoplug removed.

This was noticed as kernel panics at resume on a Dell XPS15 9550 with TB16 thunderbolt dock.

Avoid sleeping with spin_lock_irq() held, use udelay() instead

The original workaround was added to 4.9 and 4.12 stable releases, this patch needs to be applied to those as well.

Fixes 9da5a1092b13 ("xhci: Bad Ethernet performance plugged in ASM1042A host")
Cc <[email protected]> #4.9+
Reported-by Jose Marino <[email protected]>
Tested-by Jose Marino <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/pci-quirks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/host/pci-quirks.c +1 -1
index e02dbb1..6dda362 100644
@@ -447,7 +447,7 @@ static int usb_asmedia_wait_write(struct pci_dev *pdev)
 		if ((value & ASMT_CONTROL_WRITE_BIT) == 0)
 			return 0;
 
-		usleep_range(40, 60);
+		udelay(50);
 	}
 
 	dev_warn(&pdev->dev, "%s: check_write_ready timeout", __func__);
Signature · 1 line
1.9.1
author
#7 lore raw

PATCH 7/8 xhci: set missing SuperSpeedPlus Link Protocol bit in roothub descriptor

In reply to a message not in the archive

A SuperSpeedPlus roothub needs to have the Link Protocol (LP) bit set in the bmSublinkSpeedAttr[] entry of a SuperSpeedPlus descriptor.

If the xhci controller has an optional Protocol Speed ID (PSI) table then that will be used as a base to create the roothub SuperSpeedPlus descriptor.
The PSI table does not however necessary contain the LP bit so we need to set it manually.

Check the psi speed and set LP bit if speed is 10Gbps or higher. We're not setting it for 5 to 10Gbps as USB 3.1 specification always mention SuperSpeedPlus for 10Gbps or higher, and some SSIC USB 3.0 speeds can be over 5Gbps, such as SSIC-G3B-L1 at 5830 Mbps

Cc <[email protected]> # 4.6+
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/xhci-hub.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/usb/host/xhci-hub.c +10 -1
index ad89a6d..4dba2cf 100644
@@ -112,7 +112,7 @@ static int xhci_create_usb3_bos_desc(struct xhci_hcd *xhci, char *buf,
 
 	/* If PSI table exists, add the custom speed attributes from it */
 	if (usb3_1 && xhci->usb3_rhub.psi_count) {
-		u32 ssp_cap_base, bm_attrib, psi;
+		u32 ssp_cap_base, bm_attrib, psi, psi_mant, psi_exp;
 		int offset;
 
 		ssp_cap_base = USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE;

@@ -139,6 +139,15 @@ static int xhci_create_usb3_bos_desc(struct xhci_hcd *xhci, char *buf,
 		for (i = 0; i < xhci->usb3_rhub.psi_count; i++) {
 			psi = xhci->usb3_rhub.psi[i];
 			psi &= ~USB_SSP_SUBLINK_SPEED_RSVD;
+			psi_exp = XHCI_EXT_PORT_PSIE(psi);
+			psi_mant = XHCI_EXT_PORT_PSIM(psi);
+
+			/* Shift to Gbps and set SSP Link BIT(14) if 10Gpbs */
+			for (; psi_exp < 3; psi_exp++)
+				psi_mant /= 1000;
+			if (psi_mant >= 10)
+				psi |= BIT(14);
+
 			if ((psi & PLT_MASK) == PLT_SYM) {
 			/* Symmetric, create SSA RX and TX from one PSI entry */
 				put_unaligned_le32(psi, &buf[offset]);
Signature · 1 line
1.9.1
#8 lore raw

Re: [PATCH 7/8] xhci: set missing SuperSpeedPlus Link Protocol bit in roothub descriptor

Hello!

Only some grammar nits, could prolly be fixed when applying...

Quoted 7 lines · Mathias Nyman

A SuperSpeedPlus roothub needs to have the Link Protocol (LP) bit set in the bmSublinkSpeedAttr[] entry of a SuperSpeedPlus descriptor.

If the xhci controller has an optional Protocol Speed ID (PSI) table then that will be used as a base to create the roothub SuperSpeedPlus descriptor.
The PSI table does not however necessary contain the LP bit so we need

Necessarily.

Quoted 5 lines

to set it manually.

Check the psi speed and set LP bit if speed is 10Gbps or higher. We're not setting it for 5 to 10Gbps as USB 3.1 specification always mention SuperSpeedPlus for 10Gbps or higher, and some SSIC USB 3.0 speeds

Mentions.

Quoted 4 lines

can be over 5Gbps, such as SSIC-G3B-L1 at 5830 Mbps

Cc <[email protected]> # 4.6+
Signed-off-by Mathias Nyman <[email protected]>

[...]

MNR, Sergei

author
#9 lore raw

PATCH 8/8 Revert "xhci: Limit USB2 port wake support for AMD Promontory hosts"

In reply to a message not in the archive

From: Kai-Heng Feng <[email protected]>

This reverts commit dec08194ffeccfa1cf085906b53d301930eae18f.

Commit dec08194ffec ("xhci: Limit USB2 port wake support for AMD Promontory hosts") makes all high speed USB ports on ASUS PRIME B350M-A cease to function after enabling runtime PM.

All boards with this chipsets will be affected, so revert the commit.

The original patch was added to stable 4.9, 4.11 and 4.12 and needs to reverted from there as well

Cc <[email protected]> # 4.9+
Signed-off-by Kai-Heng Feng <[email protected]>
Signed-off-by Mathias Nyman <[email protected]>
---
 drivers/usb/host/xhci-hub.c |  3 ---
 drivers/usb/host/xhci-pci.c | 12 ------------
 drivers/usb/host/xhci.h     |  2 +-
 3 files changed, 1 insertion(+), 16 deletions(-)
drivers/usb/host/xhci-hub.c +0 -3
index 4dba2cf..da9158f 100644
@@ -1515,9 +1515,6 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 				t2 |= PORT_WKOC_E | PORT_WKCONN_E;
 				t2 &= ~PORT_WKDISC_E;
 			}
-			if ((xhci->quirks & XHCI_U2_DISABLE_WAKE) &&
-			    (hcd->speed < HCD_USB3))
-				t2 &= ~PORT_WAKE_BITS;
 		} else
 			t2 &= ~PORT_WAKE_BITS;
drivers/usb/host/xhci-pci.c +0 -12
index 8071c8f..76f3929 100644
@@ -54,11 +54,6 @@
 #define PCI_DEVICE_ID_INTEL_APL_XHCI			0x5aa8
 #define PCI_DEVICE_ID_INTEL_DNV_XHCI			0x19d0
 
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_4			0x43b9
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_3			0x43ba
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_2			0x43bb
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_1			0x43bc
-
 #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI		0x1142
 
 static const char hcd_name[] = "xhci_hcd";

@@ -142,13 +137,6 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	if (pdev->vendor == PCI_VENDOR_ID_AMD)
 		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 
-	if ((pdev->vendor == PCI_VENDOR_ID_AMD) &&
-		((pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_4) ||
-		(pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_3) ||
-		(pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_2) ||
-		(pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
-		xhci->quirks |= XHCI_U2_DISABLE_WAKE;
-
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 		xhci->quirks |= XHCI_INTEL_HOST;
drivers/usb/host/xhci.h +1 -1
index 7c87189..2b48aa4 100644
@@ -1828,7 +1828,7 @@ struct xhci_hcd {
 /* For controller with a broken Port Disable implementation */
 #define XHCI_BROKEN_PORT_PED	(1 << 25)
 #define XHCI_LIMIT_ENDPOINT_INTERVAL_7	(1 << 26)
-#define XHCI_U2_DISABLE_WAKE	(1 << 27)
+/* Reserved. It was XHCI_U2_DISABLE_WAKE */
 #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	(1 << 28)
 
 	unsigned int		num_active_eps;
Signature · 1 line
1.9.1