Skip to contentSkip to conversation

thunderbolt: A couple of fixes

PATCH In review Mika Westerberg 3 participants 10 messages Mar 6, 2023 – Mar 14, 2023

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover thunderbolt: A couple of fixes

Hi,

This series includes a couple of fixes for issues found. I'm planning to get these into v6.3-rc.

The first one is a memory leak fix.

The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing UNSET_INBOUND_SBTX after retimer access and this works in the lab.

@Christian, can you check that it solves the issue for you too?

The third one is a quirk that is needed for Intel hardware to limit the USB3 bandwidth accordingly.

Gil Fine (2):
thunderbolt: Add missing UNSET_INBOUND_SBTX for retimer access
thunderbolt: Limit USB3 bandwidth of certain Intel USB4 host routers

Mika Westerberg (3):
thunderbolt: Fix memory leak in margining
thunderbolt: Call tb_check_quirks() after initializing adapters
thunderbolt: Use scale field when allocating USB3 bandwidth

 drivers/thunderbolt/debugfs.c | 12 ++++----
 drivers/thunderbolt/quirks.c  | 31 ++++++++++++++++++++
 drivers/thunderbolt/retimer.c | 23 +++++++++++++--
 drivers/thunderbolt/sb_regs.h |  1 +
 drivers/thunderbolt/switch.c  |  4 +--
 drivers/thunderbolt/tb.h      |  4 +++
 drivers/thunderbolt/usb4.c    | 53 ++++++++++++++++++++++++++++++-----
 7 files changed, 111 insertions(+), 17 deletions(-)
Signature · 1 line
2.39.1
#1 lore raw

PATCH 1/5 thunderbolt: Fix memory leak in margining

Memory for the usb4->margining needs to be relased for the upstream port of the router as well, even though the debugfs directory gets released with the router device removal. Fix this.

Fixes d0f1e0c2a699 ("thunderbolt: Add support for receiver lane margining")
---
 drivers/thunderbolt/debugfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/thunderbolt/debugfs.c +6 -6
index 4339e706cc3a..f92ad71ef983 100644
@@ -942,7 +942,8 @@ static void margining_port_remove(struct tb_port *port)
 
 	snprintf(dir_name, sizeof(dir_name), "port%d", port->port);
 	parent = debugfs_lookup(dir_name, port->sw->debugfs_dir);
-	debugfs_remove_recursive(debugfs_lookup("margining", parent));
+	if (parent)
+		debugfs_remove_recursive(debugfs_lookup("margining", parent));
 
 	kfree(port->usb4->margining);
 	port->usb4->margining = NULL;
@@ -967,19 +968,18 @@ static void margining_switch_init(struct tb_switch *sw)
 
 static void margining_switch_remove(struct tb_switch *sw)
 {
+	struct tb_port *upstream, *downstream;
 	struct tb_switch *parent_sw;
-	struct tb_port *downstream;
 	u64 route = tb_route(sw);
 
 	if (!route)
 		return;
 
-	/*
-	 * Upstream is removed with the router itself but we need to
-	 * remove the downstream port margining directory.
-	 */
+	upstream = tb_upstream_port(sw);
 	parent_sw = tb_switch_parent(sw);
 	downstream = tb_port_at(route, parent_sw);
+
+	margining_port_remove(upstream);
 	margining_port_remove(downstream);
 }
 
Signature · 1 line
2.39.1
#2 lore raw

PATCH 2/5 thunderbolt: Add missing UNSET_INBOUND_SBTX for retimer access

In reply to Mika Westerberg

From: Gil Fine <[email protected]>

According to USB4 retimer specification, the process of firmware update sequence requires issuing a SET_INBOUND_SBTX port operation that later shall be followed by UNSET_INBOUND_SBTX port operation. This last step is not currently issued by the driver but it is necessary to make sure the retimers are put back to passthrough mode even during enumeration.

If this step is missing the link may not come up properly after soft-reboot for example.

For this reason issue UNSET_INBOUND_SBTX after SET_INBOUND_SBTX for enumeration and also when the NVM upgrade is run.

---
 drivers/thunderbolt/retimer.c | 23 +++++++++++++++++++++--
 drivers/thunderbolt/sb_regs.h |  1 +
 drivers/thunderbolt/tb.h      |  1 +
 drivers/thunderbolt/usb4.c    | 14 ++++++++++++++
 4 files changed, 37 insertions(+), 2 deletions(-)
drivers/thunderbolt/retimer.c +21 -2
index 56008eb91e2e..9cc28197dbc4 100644
@@ -187,6 +187,22 @@ static ssize_t nvm_authenticate_show(struct device *dev,
 	return ret;
 }
 
+static void tb_retimer_set_inbound_sbtx(struct tb_port *port)
+{
+	int i;
+
+	for (i = 1; i <= TB_MAX_RETIMER_INDEX; i++)
+		usb4_port_retimer_set_inbound_sbtx(port, i);
+}
+
+static void tb_retimer_unset_inbound_sbtx(struct tb_port *port)
+{
+	int i;
+
+	for (i = TB_MAX_RETIMER_INDEX; i >= 1; i--)
+		usb4_port_retimer_unset_inbound_sbtx(port, i);
+}
+
 static ssize_t nvm_authenticate_store(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
 {
@@ -213,6 +229,7 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 	rt->auth_status = 0;
 
 	if (val) {
+		tb_retimer_set_inbound_sbtx(rt->port);
 		if (val == AUTHENTICATE_ONLY) {
 			ret = tb_retimer_nvm_authenticate(rt, true);
 		} else {
@@ -232,6 +249,7 @@ static ssize_t nvm_authenticate_store(struct device *dev,
 	}
 
 exit_unlock:
+	tb_retimer_unset_inbound_sbtx(rt->port);
 	mutex_unlock(&rt->tb->lock);
 exit_rpm:
 	pm_runtime_mark_last_busy(&rt->dev);
@@ -440,8 +458,7 @@ int tb_retimer_scan(struct tb_port *port, bool add)
 	 * Enable sideband channel for each retimer. We can do this
 	 * regardless whether there is device connected or not.
 	 */
-	for (i = 1; i <= TB_MAX_RETIMER_INDEX; i++)
-		usb4_port_retimer_set_inbound_sbtx(port, i);
+	tb_retimer_set_inbound_sbtx(port);
 
 	/*
 	 * Before doing anything else, read the authentication status.
@@ -464,6 +481,8 @@ int tb_retimer_scan(struct tb_port *port, bool add)
 			break;
 	}
 
+	tb_retimer_unset_inbound_sbtx(port);
+
 	if (!last_idx)
 		return 0;
 
drivers/thunderbolt/sb_regs.h +1 -0
index 5185cf3e4d97..f37a4320f10a 100644
@@ -20,6 +20,7 @@ enum usb4_sb_opcode {
 	USB4_SB_OPCODE_ROUTER_OFFLINE = 0x4e45534c,		/* "LSEN" */
 	USB4_SB_OPCODE_ENUMERATE_RETIMERS = 0x4d554e45,		/* "ENUM" */
 	USB4_SB_OPCODE_SET_INBOUND_SBTX = 0x5055534c,		/* "LSUP" */
+	USB4_SB_OPCODE_UNSET_INBOUND_SBTX = 0x50555355,		/* "USUP" */
 	USB4_SB_OPCODE_QUERY_LAST_RETIMER = 0x5453414c,		/* "LAST" */
 	USB4_SB_OPCODE_GET_NVM_SECTOR_SIZE = 0x53534e47,	/* "GNSS" */
 	USB4_SB_OPCODE_NVM_SET_OFFSET = 0x53504f42,		/* "BOPS" */
drivers/thunderbolt/tb.h +1 -0
index 64968c162ec7..b3cd13dc783b 100644
@@ -1242,6 +1242,7 @@ int usb4_port_sw_margin(struct tb_port *port, unsigned int lanes, bool timing,
 int usb4_port_sw_margin_errors(struct tb_port *port, u32 *errors);
 
 int usb4_port_retimer_set_inbound_sbtx(struct tb_port *port, u8 index);
+int usb4_port_retimer_unset_inbound_sbtx(struct tb_port *port, u8 index);
 int usb4_port_retimer_read(struct tb_port *port, u8 index, u8 reg, void *buf,
 			   u8 size);
 int usb4_port_retimer_write(struct tb_port *port, u8 index, u8 reg,
drivers/thunderbolt/usb4.c +14 -0
index 1e5e9c147a31..95ff02395822 100644
@@ -1578,6 +1578,20 @@ int usb4_port_retimer_set_inbound_sbtx(struct tb_port *port, u8 index)
 				    500);
 }
 
+/**
+ * usb4_port_retimer_unset_inbound_sbtx() - Disable sideband channel transactions
+ * @port: USB4 port
+ * @index: Retimer index
+ *
+ * Disables sideband channel transations on SBTX. The reverse of
+ * usb4_port_retimer_set_inbound_sbtx().
+ */
+int usb4_port_retimer_unset_inbound_sbtx(struct tb_port *port, u8 index)
+{
+	return usb4_port_retimer_op(port, index,
+				    USB4_SB_OPCODE_UNSET_INBOUND_SBTX, 500);
+}
+
 /**
  * usb4_port_retimer_read() - Read from retimer sideband registers
  * @port: USB4 port
Signature · 1 line
2.39.1
#3 lore raw

PATCH 3/5 thunderbolt: Call tb_check_quirks() after initializing adapters

In reply to Mika Westerberg

In order to apply quirks based on certain adapter types move call to tb_check_quirks() happen after the adapters are initialized. This should not affect the existing quirks.

---
 drivers/thunderbolt/switch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/thunderbolt/switch.c +2 -2
index 3370e18ba05f..da373ac38285 100644
@@ -2968,8 +2968,6 @@ int tb_switch_add(struct tb_switch *sw)
 			dev_warn(&sw->dev, "reading DROM failed: %d\n", ret);
 		tb_sw_dbg(sw, "uid: %#llx\n", sw->uid);
 
-		tb_check_quirks(sw);
-
 		ret = tb_switch_set_uuid(sw);
 		if (ret) {
 			dev_err(&sw->dev, "failed to set UUID\n");
@@ -2988,6 +2986,8 @@ int tb_switch_add(struct tb_switch *sw)
 			}
 		}
 
+		tb_check_quirks(sw);
+
 		tb_switch_default_link_ports(sw);
 
 		ret = tb_switch_update_link_attributes(sw);
Signature · 1 line
2.39.1
#4 lore raw

PATCH 4/5 thunderbolt: Limit USB3 bandwidth of certain Intel USB4 host routers

In reply to Mika Westerberg

From: Gil Fine <[email protected]>

Current Intel USB4 host routers have hardware limitation that the USB3 bandwidth cannot go higher than 16376 Mb/s. Work this around by adding a new quirk that limits the bandwidth for the affected host routers.

---
 drivers/thunderbolt/quirks.c | 31 +++++++++++++++++++++++++++++++
 drivers/thunderbolt/tb.h     |  3 +++
 drivers/thunderbolt/usb4.c   | 17 +++++++++++++++--
 3 files changed, 49 insertions(+), 2 deletions(-)
drivers/thunderbolt/quirks.c +31 -0
index ae28a03fa890..1157b8869bcc 100644
@@ -26,6 +26,19 @@ static void quirk_clx_disable(struct tb_switch *sw)
 	tb_sw_dbg(sw, "disabling CL states\n");
 }
 
+static void quirk_usb3_maximum_bandwidth(struct tb_switch *sw)
+{
+	struct tb_port *port;
+
+	tb_switch_for_each_port(sw, port) {
+		if (!tb_port_is_usb3_down(port))
+			continue;
+		port->max_bw = 16376;
+		tb_port_dbg(port, "USB3 maximum bandwidth limited to %u Mb/s\n",
+			    port->max_bw);
+	}
+}
+
 struct tb_quirk {
 	u16 hw_vendor_id;
 	u16 hw_device_id;
@@ -43,6 +56,24 @@ static const struct tb_quirk tb_quirks[] = {
 	 * DP buffers.
 	 */
 	{ 0x8087, 0x0b26, 0x0000, 0x0000, quirk_dp_credit_allocation },
+	/*
+	 * Limit the maximum USB3 bandwidth for the following Intel USB4
+	 * host routers due to a hardware issue.
+	 */
+	{ 0x8087, PCI_DEVICE_ID_INTEL_ADL_NHI0, 0x0000, 0x0000,
+		  quirk_usb3_maximum_bandwidth },
+	{ 0x8087, PCI_DEVICE_ID_INTEL_ADL_NHI1, 0x0000, 0x0000,
+		  quirk_usb3_maximum_bandwidth },
+	{ 0x8087, PCI_DEVICE_ID_INTEL_RPL_NHI0, 0x0000, 0x0000,
+		  quirk_usb3_maximum_bandwidth },
+	{ 0x8087, PCI_DEVICE_ID_INTEL_RPL_NHI1, 0x0000, 0x0000,
+		  quirk_usb3_maximum_bandwidth },
+	{ 0x8087, PCI_DEVICE_ID_INTEL_MTL_M_NHI0, 0x0000, 0x0000,
+		  quirk_usb3_maximum_bandwidth },
+	{ 0x8087, PCI_DEVICE_ID_INTEL_MTL_P_NHI0, 0x0000, 0x0000,
+		  quirk_usb3_maximum_bandwidth },
+	{ 0x8087, PCI_DEVICE_ID_INTEL_MTL_P_NHI1, 0x0000, 0x0000,
+		  quirk_usb3_maximum_bandwidth },
 	/*
 	 * CLx is not supported on AMD USB4 Yellow Carp and Pink Sardine platforms.
 	 */
drivers/thunderbolt/tb.h +3 -0
index b3cd13dc783b..275ff5219a3a 100644
@@ -272,6 +272,8 @@ struct tb_bandwidth_group {
  * @group: Bandwidth allocation group the adapter is assigned to. Only
  *	   used for DP IN adapters for now.
  * @group_list: The adapter is linked to the group's list of ports through this
+ * @max_bw: Maximum possible bandwidth through this adapter if set to
+ *	    non-zero.
  *
  * In USB4 terminology this structure represents an adapter (protocol or
  * lane adapter).
@@ -299,6 +301,7 @@ struct tb_port {
 	unsigned int dma_credits;
 	struct tb_bandwidth_group *group;
 	struct list_head group_list;
+	unsigned int max_bw;
 };
 
 /**
drivers/thunderbolt/usb4.c +15 -2
index 95ff02395822..6e87cf993c68 100644
@@ -1882,6 +1882,15 @@ int usb4_port_retimer_nvm_read(struct tb_port *port, u8 index,
 				usb4_port_retimer_nvm_read_block, &info);
 }
 
+static inline unsigned int
+usb4_usb3_port_max_bandwidth(const struct tb_port *port, unsigned int bw)
+{
+	/* Take the possible bandwidth limitation into account */
+	if (port->max_bw)
+		return min(bw, port->max_bw);
+	return bw;
+}
+
 /**
  * usb4_usb3_port_max_link_rate() - Maximum support USB3 link rate
  * @port: USB3 adapter port
@@ -1903,7 +1912,9 @@ int usb4_usb3_port_max_link_rate(struct tb_port *port)
 		return ret;
 
 	lr = (val & ADP_USB3_CS_4_MSLR_MASK) >> ADP_USB3_CS_4_MSLR_SHIFT;
-	return lr == ADP_USB3_CS_4_MSLR_20G ? 20000 : 10000;
+	ret = lr == ADP_USB3_CS_4_MSLR_20G ? 20000 : 10000;
+
+	return usb4_usb3_port_max_bandwidth(port, ret);
 }
 
 /**
@@ -1930,7 +1941,9 @@ int usb4_usb3_port_actual_link_rate(struct tb_port *port)
 		return 0;
 
 	lr = val & ADP_USB3_CS_4_ALR_MASK;
-	return lr == ADP_USB3_CS_4_ALR_20G ? 20000 : 10000;
+	ret = lr == ADP_USB3_CS_4_ALR_20G ? 20000 : 10000;
+
+	return usb4_usb3_port_max_bandwidth(port, ret);
 }
 
 static int usb4_usb3_port_cm_request(struct tb_port *port, bool request)
Signature · 1 line
2.39.1
#5 lore raw

PATCH 5/5 thunderbolt: Use scale field when allocating USB3 bandwidth

In reply to Mika Westerberg

When tunneling aggregated USB3 (20 Gb/s) the bandwidth values that are programmed to the ADP_USB3_CS_2 go higher than 4096 and that does not fit anymmore to the 12-bit field. Fix this by scaling the value using the scale field accordingly.

Fixes 3b1d8d577ca8 ("thunderbolt: Implement USB3 bandwidth negotiation routines")
---
 drivers/thunderbolt/usb4.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/thunderbolt/usb4.c +17 -5
index 6e87cf993c68..a0996cb2893c 100644
@@ -2094,18 +2094,30 @@ static int usb4_usb3_port_write_allocated_bandwidth(struct tb_port *port,
 						    int downstream_bw)
 {
 	u32 val, ubw, dbw, scale;
-	int ret;
+	int ret, max_bw;
 
-	/* Read the used scale, hardware default is 0 */
-	ret = tb_port_read(port, &scale, TB_CFG_PORT,
-			   port->cap_adap + ADP_USB3_CS_3, 1);
+	/* Figure out suitable scale */
+	scale = 0;
+	max_bw = max(upstream_bw, downstream_bw);
+	while (scale < 64) {
+		if (mbps_to_usb3_bw(max_bw, scale) < 4096)
+			break;
+		scale++;
+	}
+
+	if (WARN_ON(scale >= 64))
+		return -EINVAL;
+
+	ret = tb_port_write(port, &scale, TB_CFG_PORT,
+			    port->cap_adap + ADP_USB3_CS_3, 1);
 	if (ret)
 		return ret;
 
-	scale &= ADP_USB3_CS_3_SCALE_MASK;
 	ubw = mbps_to_usb3_bw(upstream_bw, scale);
 	dbw = mbps_to_usb3_bw(downstream_bw, scale);
 
+	tb_port_dbg(port, "scaled bandwidth %u/%u, scale %u\n", ubw, dbw, scale);
+
 	ret = tb_port_read(port, &val, TB_CFG_PORT,
 			   port->cap_adap + ADP_USB3_CS_2, 1);
 	if (ret)
Signature · 1 line
2.39.1
maintainer
#6 lore raw

Re: [PATCH 5/5] thunderbolt: Use scale field when allocating USB3 bandwidth

Quoted 4 lines · Mika Westerberg

When tunneling aggregated USB3 (20 Gb/s) the bandwidth values that are programmed to the ADP_USB3_CS_2 go higher than 4096 and that does not fit anymmore to the 12-bit field. Fix this by scaling the value using

typo: anymore

#7 lore raw
In reply to Mika Westerberg
Quoted 20 lines · Mika Westerberg

Hi,

This series includes a couple of fixes for issues found. I'm planning to get these into v6.3-rc.

The first one is a memory leak fix.

The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing UNSET_INBOUND_SBTX after retimer access and this works in the lab.

@Christian, can you check that it solves the issue for you too?

The third one is a quirk that is needed for Intel hardware to limit the USB3 bandwidth accordingly.

Quoted 8 lines

Gil Fine (2):
thunderbolt: Add missing UNSET_INBOUND_SBTX for retimer access
thunderbolt: Limit USB3 bandwidth of certain Intel USB4 host routers

Mika Westerberg (3):
thunderbolt: Fix memory leak in margining
thunderbolt: Call tb_check_quirks() after initializing adapters
thunderbolt: Use scale field when allocating USB3 bandwidth

Fixed the typo reported by Yehezkel and applied all to thunderbolt.git/fixes.

#8 lore raw

Hi Mika,

Quoted 9 lines
Quoted 9 lines

The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing UNSET_INBOUND_SBTX after retimer access and this works in the lab.

@Christian, can you check that it solves the issue for you too?

I was told today that the patch solves the issue on the Elitebook! (don't have the hardware here in my office any more...).

Thanks for fixing this!
Best regards,
Christian

#9 lore raw
Quoted 11 lines · Christian Schaubschläger

Hi Mika,

Quoted 8 lines
Quoted 8 lines

The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing UNSET_INBOUND_SBTX after retimer access and this works in the lab.

Quoted 3 lines
Quoted 1 line
Quoted 1 line

@Christian, can you check that it solves the issue for you too?

I was told today that the patch solves the issue on the Elitebook!

Okay good to know :)