[PATCH v4 Cover Letter] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[PATCH v4 Cover Letter] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mohammad Rahimi]

Dear Mika,

V4 of this patch resolves the styling issues identified in v3 by ./scripts/checkpatch.pl.
Apologies for the inconvenience.

Best regards,
Mohammad Rahimi

[PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mohammad Rahimi]

Updates the Connection Manager to disable the Gen 4 Link Recovery
flow before transitioning from a Symmetric Link to an Asymmetric
Link, as specified in recent changes to the USB4 v2 specification.

According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
published in September 2024, the rationale for this change is:

  "Since the default value of the Target Asymmetric Link might be
  different than Symmetric Link and Gen 4 Link Recovery flow checks
  this field to make sure it matched the actual Negotiated Link Width,
  we’re removing the condition for a Disconnect in the Gen 4 Link
  Recovery flow when Target Asymmetric Link doesn’t match the actual
  Link width and adding a Connection Manager note to Disable Gen 4 Link
  Recovery flow before doing Asymmetric Transitions."

Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
---
 drivers/thunderbolt/tb.c      | 18 ++++++--
 drivers/thunderbolt/tb.h      |  3 ++
 drivers/thunderbolt/tb_regs.h |  1 +
 drivers/thunderbolt/usb4.c    | 77 +++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index a7c6919fbf97..a3ccfb398797 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -1013,6 +1013,7 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
 			     struct tb_port *dst_port, int requested_up,
 			     int requested_down)
 {
+	bool link_recovery_up = false, link_recovery_down = false;
 	bool clx = false, clx_disabled = false, downstream;
 	struct tb_switch *sw;
 	struct tb_port *up;
@@ -1075,15 +1076,19 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
 			continue;
 
 		/*
-		 * Disable CL states before doing any transitions. We
-		 * delayed it until now that we know there is a real
-		 * transition taking place.
+		 * Disable CL states and Link Recovery before doing any
+		 * transitions. We delayed it until now that we know
+		 * there is a real transition taking place.
 		 */
 		if (!clx_disabled) {
 			clx = tb_disable_clx(sw);
 			clx_disabled = true;
 		}
 
+		/* Ignore non-critical errors of disabling Link Recovery */
+		link_recovery_up = (usb4_port_link_recovery_disable(up) > 0);
+		link_recovery_down = (usb4_port_link_recovery_disable(down) > 0);
+
 		tb_sw_dbg(up->sw, "configuring asymmetric link\n");
 
 		/*
@@ -1091,6 +1096,13 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
 		 * transtion the link into asymmetric now.
 		 */
 		ret = tb_switch_set_link_width(up->sw, width_up);
+
+		/* Re-enable Link Recovery if they were previously enabled */
+		if (link_recovery_up)
+			usb4_port_link_recovery_enable(up);
+		if (link_recovery_down)
+			usb4_port_link_recovery_enable(down);
+
 		if (ret) {
 			tb_sw_warn(up->sw, "failed to set link width\n");
 			break;
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index ddbf0cd78377..d37d778082fc 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1332,6 +1332,9 @@ int usb4_port_router_online(struct tb_port *port);
 int usb4_port_enumerate_retimers(struct tb_port *port);
 bool usb4_port_clx_supported(struct tb_port *port);
 
+int usb4_port_link_recovery_enable(struct tb_port *port);
+int usb4_port_link_recovery_disable(struct tb_port *port);
+
 bool usb4_port_asym_supported(struct tb_port *port);
 int usb4_port_asym_set_link_width(struct tb_port *port, enum tb_link_width width);
 int usb4_port_asym_start(struct tb_port *port);
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 4e43b47f9f11..085139e1a958 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -398,6 +398,7 @@ struct tb_regs_port_header {
 #define PORT_CS_19_WOD				BIT(17)
 #define PORT_CS_19_WOU4				BIT(18)
 #define PORT_CS_19_START_ASYM			BIT(24)
+#define PORT_CS_19_ELR				BIT(31)
 
 /* Display Port adapter registers */
 #define ADP_DP_CS_0				0x00
diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
index e51d01671d8e..450e3920f20c 100644
--- a/drivers/thunderbolt/usb4.c
+++ b/drivers/thunderbolt/usb4.c
@@ -10,6 +10,7 @@
 #include <linux/delay.h>
 #include <linux/ktime.h>
 #include <linux/units.h>
+#include <linux/string_helpers.h>
 
 #include "sb_regs.h"
 #include "tb.h"
@@ -1518,6 +1519,82 @@ bool usb4_port_clx_supported(struct tb_port *port)
 	return !!(val & PORT_CS_18_CPS);
 }
 
+static int __usb4_port_link_recovery_enable(struct tb_port *port, bool enable)
+{
+	bool was_enable;
+	int ret;
+	u32 val;
+
+	if (!port->cap_usb4)
+		return -EINVAL;
+
+	ret = tb_port_read(port, &val, TB_CFG_PORT,
+			   port->cap_usb4 + PORT_CS_19, 1);
+	if (ret)
+		return ret;
+
+	was_enable = !!(val & PORT_CS_19_ELR);
+
+	if (enable)
+		val |= PORT_CS_19_ELR;
+	else
+		val &= ~PORT_CS_19_ELR;
+
+	ret = tb_port_write(port, &val, TB_CFG_PORT,
+			    port->cap_usb4 + PORT_CS_19, 1);
+	if (ret)
+		return ret;
+
+	tb_port_dbg(port, "link recovery %s\n", str_enabled_disabled(enable));
+	return was_enable ? 1 : 0;
+}
+
+/**
+ * usb4_port_link_recovery_enable() - Enable the Link Recovery
+ * @port: USB4 port
+ *
+ * Enables the Link Recovery for @port.
+ *
+ *	Returns:
+ *	* %-EINVAL		- Capability not present.
+ *	* %-ENODEV		- Switch unplugged.
+ *	* %-ETIMEDOUT		- Config space access timed out.
+ *	* %0			- Successfully enabled.
+ *	* %1			- Already enabled.
+ */
+int usb4_port_link_recovery_enable(struct tb_port *port)
+{
+	int ret = __usb4_port_link_recovery_enable(port, true);
+
+	if (ret < 0)
+		tb_port_warn(port, "failed to enable link recovery\n");
+
+	return ret;
+}
+
+/**
+ * usb4_port_link_recovery_disable() - Disable the Link Recovery
+ * @port: USB4 port
+ *
+ * Disables the Link Recovery for @port.
+ *
+ *	Returns:
+ *	* %-EINVAL		- Capability not present.
+ *	* %-ENODEV		- Switch unplugged.
+ *	* %-ETIMEDOUT		- Config space access timed out.
+ *	* %0			- Already disabled.
+ *	* %1			- Successfully disabled.
+ */
+int usb4_port_link_recovery_disable(struct tb_port *port)
+{
+	int ret = __usb4_port_link_recovery_enable(port, false);
+
+	if (ret < 0)
+		tb_port_warn(port, "failed to disable link recovery\n");
+
+	return ret;
+}
+
 /**
  * usb4_port_asym_supported() - If the port supports asymmetric link
  * @port: USB4 port
-- 
2.45.2

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mika Westerberg]

Hi,

On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> Updates the Connection Manager to disable the Gen 4 Link Recovery
> flow before transitioning from a Symmetric Link to an Asymmetric
> Link, as specified in recent changes to the USB4 v2 specification.
> 
> According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> published in September 2024, the rationale for this change is:
> 
>   "Since the default value of the Target Asymmetric Link might be
>   different than Symmetric Link and Gen 4 Link Recovery flow checks
>   this field to make sure it matched the actual Negotiated Link Width,
>   we’re removing the condition for a Disconnect in the Gen 4 Link
>   Recovery flow when Target Asymmetric Link doesn’t match the actual
>   Link width and adding a Connection Manager note to Disable Gen 4 Link
>   Recovery flow before doing Asymmetric Transitions."
> 
> Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>

I did some minor modifications and applied to thunderbolt.git/next. Please
let me know if I missed something.

I tested on Intel reference hardware as well but on those link recovery is
already disabled so it pretty much does nothing. Below are the steps that I
followed:

1. Boot with "thunderbolt.asym_threshold=10000" so that the driver switches
   links to asymmetric already upon first DP connect.

2. Connect USB4 v2 hub.
3. Check link and PORT_CS_19:

# tblist -v
Domain 0 Route 0: 8087:5781 Intel Barlow Host Router
  Type: Router
  UUID: c9680000...
  Generation: USB4
  NVM version: 56.81

Domain 0 Route 1: 8087:1234 Intel Barlow HUB Router
  Type: Router
  Speed (Rx/Tx): 80/80 Gb/s
  Authorized: No
  UUID: e9688780...
  Generation: USB4
  NVM version: 56.81

# tbdump -r 0 -a 1 -vv -N 1 PORT_CS_19
0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19     
  [00:00]        0x0 Downstream Port Reset (DPR)
  [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
  [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
  [03:03]        0x1 USB4 Port is Configured (PC)
  [04:04]        0x0 USB4 Port is Inter-Domain (PID)
  [16:16]        0x0 Enable Wake on Connect
  [17:17]        0x0 Enable Wake on Disconnect
  [18:18]        0x1 Enable Wake on USB4 Wake
  [19:19]        0x0 Enable Wake on Inter-Domain
  [24:24]        0x0 StartAsymmetricFlow
  [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
  [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)

# tbdump -r 1 -a 1 -vv -N 1 PORT_CS_19
0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19     
  [00:00]        0x0 Downstream Port Reset (DPR)
  [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
  [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
  [03:03]        0x1 USB4 Port is Configured (PC)
  [04:04]        0x0 USB4 Port is Inter-Domain (PID)
  [16:16]        0x0 Enable Wake on Connect
  [17:17]        0x0 Enable Wake on Disconnect
  [18:18]        0x1 Enable Wake on USB4 Wake
  [19:19]        0x0 Enable Wake on Inter-Domain
  [24:24]        0x0 StartAsymmetricFlow
  [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
  [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)

3. Connect monitor to USB4 v2 hub.
4. Check link and PORT_CS_19.

# tblist -v
Domain 0 Route 0: 8087:5781 Intel Barlow Host Router
  Type: Router
  UUID: c9680000...
  Generation: USB4
  NVM version: 56.81

Domain 0 Route 1: 8087:1234 Intel Barlow HUB Router
  Type: Router
  Speed (Rx/Tx): 120/40 Gb/s
  Authorized: No
  UUID: e9688780...
  Generation: USB4
  NVM version: 56.81

# tbdump -r 0 -a 1 -vv -N 1 PORT_CS_19
0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19
  [00:00]        0x0 Downstream Port Reset (DPR)
  [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
  [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
  [03:03]        0x1 USB4 Port is Configured (PC)
  [04:04]        0x0 USB4 Port is Inter-Domain (PID)
  [16:16]        0x0 Enable Wake on Connect
  [17:17]        0x0 Enable Wake on Disconnect
  [18:18]        0x1 Enable Wake on USB4 Wake
  [19:19]        0x0 Enable Wake on Inter-Domain
  [24:24]        0x0 StartAsymmetricFlow
  [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
  [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)

# tbdump -r 1 -a 1 -vv -N 1 PORT_CS_19
0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19
  [00:00]        0x0 Downstream Port Reset (DPR)
  [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
  [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
  [03:03]        0x1 USB4 Port is Configured (PC)
  [04:04]        0x0 USB4 Port is Inter-Domain (PID)
  [16:16]        0x0 Enable Wake on Connect
  [17:17]        0x0 Enable Wake on Disconnect
  [18:18]        0x1 Enable Wake on USB4 Wake
  [19:19]        0x0 Enable Wake on Inter-Domain
  [24:24]        0x0 StartAsymmetricFlow
  [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
  [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mohammad Rahimi]

Hello.

On Mon, Feb 03, 2025 at 10:42:24AM GMT, Mika Westerberg wrote:
> Hi,
> 
> On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> > Updates the Connection Manager to disable the Gen 4 Link Recovery
> > flow before transitioning from a Symmetric Link to an Asymmetric
> > Link, as specified in recent changes to the USB4 v2 specification.
> > 
> > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> > published in September 2024, the rationale for this change is:
> > 
> >   "Since the default value of the Target Asymmetric Link might be
> >   different than Symmetric Link and Gen 4 Link Recovery flow checks
> >   this field to make sure it matched the actual Negotiated Link Width,
> >   we’re removing the condition for a Disconnect in the Gen 4 Link
> >   Recovery flow when Target Asymmetric Link doesn’t match the actual
> >   Link width and adding a Connection Manager note to Disable Gen 4 Link
> >   Recovery flow before doing Asymmetric Transitions."
> > 
> > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> 
> I did some minor modifications and applied to thunderbolt.git/next. Please
> let me know if I missed something.
> 

Looks great. Just one question:

In tb_configure_asym(), if tb_switch_set_link_width() fails, we won’t restore
the link recovery status. Is that okay?

> I tested on Intel reference hardware as well but on those link recovery is
> already disabled so it pretty much does nothing. Below are the steps that I
> followed:
> 
> 1. Boot with "thunderbolt.asym_threshold=10000" so that the driver switches
>    links to asymmetric already upon first DP connect.
> 
> 2. Connect USB4 v2 hub.
> 3. Check link and PORT_CS_19:
> 
> # tblist -v
> Domain 0 Route 0: 8087:5781 Intel Barlow Host Router
>   Type: Router
>   UUID: c9680000...
>   Generation: USB4
>   NVM version: 56.81
> 
> Domain 0 Route 1: 8087:1234 Intel Barlow HUB Router
>   Type: Router
>   Speed (Rx/Tx): 80/80 Gb/s
>   Authorized: No
>   UUID: e9688780...
>   Generation: USB4
>   NVM version: 56.81
> 
> # tbdump -r 0 -a 1 -vv -N 1 PORT_CS_19
> 0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19     
>   [00:00]        0x0 Downstream Port Reset (DPR)
>   [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
>   [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
>   [03:03]        0x1 USB4 Port is Configured (PC)
>   [04:04]        0x0 USB4 Port is Inter-Domain (PID)
>   [16:16]        0x0 Enable Wake on Connect
>   [17:17]        0x0 Enable Wake on Disconnect
>   [18:18]        0x1 Enable Wake on USB4 Wake
>   [19:19]        0x0 Enable Wake on Inter-Domain
>   [24:24]        0x0 StartAsymmetricFlow
>   [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
>   [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)
> 
> # tbdump -r 1 -a 1 -vv -N 1 PORT_CS_19
> 0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19     
>   [00:00]        0x0 Downstream Port Reset (DPR)
>   [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
>   [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
>   [03:03]        0x1 USB4 Port is Configured (PC)
>   [04:04]        0x0 USB4 Port is Inter-Domain (PID)
>   [16:16]        0x0 Enable Wake on Connect
>   [17:17]        0x0 Enable Wake on Disconnect
>   [18:18]        0x1 Enable Wake on USB4 Wake
>   [19:19]        0x0 Enable Wake on Inter-Domain
>   [24:24]        0x0 StartAsymmetricFlow
>   [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
>   [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)
> 
> 3. Connect monitor to USB4 v2 hub.
> 4. Check link and PORT_CS_19.
> 
> # tblist -v
> Domain 0 Route 0: 8087:5781 Intel Barlow Host Router
>   Type: Router
>   UUID: c9680000...
>   Generation: USB4
>   NVM version: 56.81
> 
> Domain 0 Route 1: 8087:1234 Intel Barlow HUB Router
>   Type: Router
>   Speed (Rx/Tx): 120/40 Gb/s
>   Authorized: No
>   UUID: e9688780...
>   Generation: USB4
>   NVM version: 56.81
> 
> # tbdump -r 0 -a 1 -vv -N 1 PORT_CS_19
> 0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19
>   [00:00]        0x0 Downstream Port Reset (DPR)
>   [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
>   [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
>   [03:03]        0x1 USB4 Port is Configured (PC)
>   [04:04]        0x0 USB4 Port is Inter-Domain (PID)
>   [16:16]        0x0 Enable Wake on Connect
>   [17:17]        0x0 Enable Wake on Disconnect
>   [18:18]        0x1 Enable Wake on USB4 Wake
>   [19:19]        0x0 Enable Wake on Inter-Domain
>   [24:24]        0x0 StartAsymmetricFlow
>   [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
>   [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)
> 
> # tbdump -r 1 -a 1 -vv -N 1 PORT_CS_19
> 0x00d3 0x0004000e 0b00000000 00000100 00000000 00001110 .... PORT_CS_19
>   [00:00]        0x0 Downstream Port Reset (DPR)
>   [01:01]        0x1 Request RS-FEC Gen 2 (RS2)
>   [02:02]        0x1 Request RS-FEC Gen 3 (RS3)
>   [03:03]        0x1 USB4 Port is Configured (PC)
>   [04:04]        0x0 USB4 Port is Inter-Domain (PID)
>   [16:16]        0x0 Enable Wake on Connect
>   [17:17]        0x0 Enable Wake on Disconnect
>   [18:18]        0x1 Enable Wake on USB4 Wake
>   [19:19]        0x0 Enable Wake on Inter-Domain
>   [24:24]        0x0 StartAsymmetricFlow
>   [30:30]        0x0 Initiate Gen 4 Link Recovery (ILR)
>   [31:31]        0x0 Enable Gen 4 Link Recovery (ELR)

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mika Westerberg]

Hi,

On Mon, Feb 03, 2025 at 12:25:21PM +0000, Mohammad Rahimi wrote:
> Hello.
> 
> On Mon, Feb 03, 2025 at 10:42:24AM GMT, Mika Westerberg wrote:
> > Hi,
> > 
> > On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> > > Updates the Connection Manager to disable the Gen 4 Link Recovery
> > > flow before transitioning from a Symmetric Link to an Asymmetric
> > > Link, as specified in recent changes to the USB4 v2 specification.
> > > 
> > > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> > > published in September 2024, the rationale for this change is:
> > > 
> > >   "Since the default value of the Target Asymmetric Link might be
> > >   different than Symmetric Link and Gen 4 Link Recovery flow checks
> > >   this field to make sure it matched the actual Negotiated Link Width,
> > >   we’re removing the condition for a Disconnect in the Gen 4 Link
> > >   Recovery flow when Target Asymmetric Link doesn’t match the actual
> > >   Link width and adding a Connection Manager note to Disable Gen 4 Link
> > >   Recovery flow before doing Asymmetric Transitions."
> > > 
> > > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> > 
> > I did some minor modifications and applied to thunderbolt.git/next. Please
> > let me know if I missed something.
> > 
> 
> Looks great. Just one question:
> 
> In tb_configure_asym(), if tb_switch_set_link_width() fails, we won’t restore
> the link recovery status. Is that okay?

Good question :) Looking at the ECR, does it actually say anywhere that the
CM should needs to re-enable it? I'm thinking that we could just disable it
and be done with it?

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mohammad Rahimi]

Hello again.

On Mon, Feb 03, 2025 at 02:39:44PM GMT, Mika Westerberg wrote:
> Hi,
> 
> On Mon, Feb 03, 2025 at 12:25:21PM +0000, Mohammad Rahimi wrote:
> > Hello.
> > 
> > On Mon, Feb 03, 2025 at 10:42:24AM GMT, Mika Westerberg wrote:
> > > Hi,
> > > 
> > > On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> > > > Updates the Connection Manager to disable the Gen 4 Link Recovery
> > > > flow before transitioning from a Symmetric Link to an Asymmetric
> > > > Link, as specified in recent changes to the USB4 v2 specification.
> > > > 
> > > > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> > > > published in September 2024, the rationale for this change is:
> > > > 
> > > >   "Since the default value of the Target Asymmetric Link might be
> > > >   different than Symmetric Link and Gen 4 Link Recovery flow checks
> > > >   this field to make sure it matched the actual Negotiated Link Width,
> > > >   we’re removing the condition for a Disconnect in the Gen 4 Link
> > > >   Recovery flow when Target Asymmetric Link doesn’t match the actual
> > > >   Link width and adding a Connection Manager note to Disable Gen 4 Link
> > > >   Recovery flow before doing Asymmetric Transitions."
> > > > 
> > > > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> > > 
> > > I did some minor modifications and applied to thunderbolt.git/next. Please
> > > let me know if I missed something.
> > > 
> > 
> > Looks great. Just one question:
> > 
> > In tb_configure_asym(), if tb_switch_set_link_width() fails, we won’t restore
> > the link recovery status. Is that okay?
> 
> Good question :) Looking at the ECR, does it actually say anywhere that the
> CM should needs to re-enable it? I'm thinking that we could just disable it
> and be done with it?

Right. Thanks for clearing that up.

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mika Westerberg]

On Mon, Feb 03, 2025 at 12:43:04PM +0000, Mohammad Rahimi wrote:
> Hello again.
> 
> On Mon, Feb 03, 2025 at 02:39:44PM GMT, Mika Westerberg wrote:
> > Hi,
> > 
> > On Mon, Feb 03, 2025 at 12:25:21PM +0000, Mohammad Rahimi wrote:
> > > Hello.
> > > 
> > > On Mon, Feb 03, 2025 at 10:42:24AM GMT, Mika Westerberg wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> > > > > Updates the Connection Manager to disable the Gen 4 Link Recovery
> > > > > flow before transitioning from a Symmetric Link to an Asymmetric
> > > > > Link, as specified in recent changes to the USB4 v2 specification.
> > > > > 
> > > > > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> > > > > published in September 2024, the rationale for this change is:
> > > > > 
> > > > >   "Since the default value of the Target Asymmetric Link might be
> > > > >   different than Symmetric Link and Gen 4 Link Recovery flow checks
> > > > >   this field to make sure it matched the actual Negotiated Link Width,
> > > > >   we’re removing the condition for a Disconnect in the Gen 4 Link
> > > > >   Recovery flow when Target Asymmetric Link doesn’t match the actual
> > > > >   Link width and adding a Connection Manager note to Disable Gen 4 Link
> > > > >   Recovery flow before doing Asymmetric Transitions."
> > > > > 
> > > > > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> > > > 
> > > > I did some minor modifications and applied to thunderbolt.git/next. Please
> > > > let me know if I missed something.
> > > > 
> > > 
> > > Looks great. Just one question:
> > > 
> > > In tb_configure_asym(), if tb_switch_set_link_width() fails, we won’t restore
> > > the link recovery status. Is that okay?
> > 
> > Good question :) Looking at the ECR, does it actually say anywhere that the
> > CM should needs to re-enable it? I'm thinking that we could just disable it
> > and be done with it?
> 
> Right. Thanks for clearing that up.

I'll check with the HW/FW folks still if they have any suggestions. I'll
keep you updated.

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mika Westerberg]

On Mon, Feb 03, 2025 at 02:59:07PM +0200, Mika Westerberg wrote:
> On Mon, Feb 03, 2025 at 12:43:04PM +0000, Mohammad Rahimi wrote:
> > Hello again.
> > 
> > On Mon, Feb 03, 2025 at 02:39:44PM GMT, Mika Westerberg wrote:
> > > Hi,
> > > 
> > > On Mon, Feb 03, 2025 at 12:25:21PM +0000, Mohammad Rahimi wrote:
> > > > Hello.
> > > > 
> > > > On Mon, Feb 03, 2025 at 10:42:24AM GMT, Mika Westerberg wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> > > > > > Updates the Connection Manager to disable the Gen 4 Link Recovery
> > > > > > flow before transitioning from a Symmetric Link to an Asymmetric
> > > > > > Link, as specified in recent changes to the USB4 v2 specification.
> > > > > > 
> > > > > > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> > > > > > published in September 2024, the rationale for this change is:
> > > > > > 
> > > > > >   "Since the default value of the Target Asymmetric Link might be
> > > > > >   different than Symmetric Link and Gen 4 Link Recovery flow checks
> > > > > >   this field to make sure it matched the actual Negotiated Link Width,
> > > > > >   we’re removing the condition for a Disconnect in the Gen 4 Link
> > > > > >   Recovery flow when Target Asymmetric Link doesn’t match the actual
> > > > > >   Link width and adding a Connection Manager note to Disable Gen 4 Link
> > > > > >   Recovery flow before doing Asymmetric Transitions."
> > > > > > 
> > > > > > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> > > > > 
> > > > > I did some minor modifications and applied to thunderbolt.git/next. Please
> > > > > let me know if I missed something.
> > > > > 
> > > > 
> > > > Looks great. Just one question:
> > > > 
> > > > In tb_configure_asym(), if tb_switch_set_link_width() fails, we won’t restore
> > > > the link recovery status. Is that okay?
> > > 
> > > Good question :) Looking at the ECR, does it actually say anywhere that the
> > > CM should needs to re-enable it? I'm thinking that we could just disable it
> > > and be done with it?
> > 
> > Right. Thanks for clearing that up.
> 
> I'll check with the HW/FW folks still if they have any suggestions. I'll
> keep you updated.

Did not hear anything from them yet but I adjusted the patch slightly to
restore ELR if tb_switch_set_link_width() and also added back the check > 0
which I missed. Let me know if you find issues with this one. Thanks!

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mika Westerberg]

Hi Again,

On Tue, Feb 04, 2025 at 01:16:25PM +0200, Mika Westerberg wrote:
> On Mon, Feb 03, 2025 at 02:59:07PM +0200, Mika Westerberg wrote:
> > On Mon, Feb 03, 2025 at 12:43:04PM +0000, Mohammad Rahimi wrote:
> > > Hello again.
> > > 
> > > On Mon, Feb 03, 2025 at 02:39:44PM GMT, Mika Westerberg wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Feb 03, 2025 at 12:25:21PM +0000, Mohammad Rahimi wrote:
> > > > > Hello.
> > > > > 
> > > > > On Mon, Feb 03, 2025 at 10:42:24AM GMT, Mika Westerberg wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> > > > > > > Updates the Connection Manager to disable the Gen 4 Link Recovery
> > > > > > > flow before transitioning from a Symmetric Link to an Asymmetric
> > > > > > > Link, as specified in recent changes to the USB4 v2 specification.
> > > > > > > 
> > > > > > > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> > > > > > > published in September 2024, the rationale for this change is:
> > > > > > > 
> > > > > > >   "Since the default value of the Target Asymmetric Link might be
> > > > > > >   different than Symmetric Link and Gen 4 Link Recovery flow checks
> > > > > > >   this field to make sure it matched the actual Negotiated Link Width,
> > > > > > >   we’re removing the condition for a Disconnect in the Gen 4 Link
> > > > > > >   Recovery flow when Target Asymmetric Link doesn’t match the actual
> > > > > > >   Link width and adding a Connection Manager note to Disable Gen 4 Link
> > > > > > >   Recovery flow before doing Asymmetric Transitions."
> > > > > > > 
> > > > > > > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> > > > > > 
> > > > > > I did some minor modifications and applied to thunderbolt.git/next. Please
> > > > > > let me know if I missed something.
> > > > > > 
> > > > > 
> > > > > Looks great. Just one question:
> > > > > 
> > > > > In tb_configure_asym(), if tb_switch_set_link_width() fails, we won’t restore
> > > > > the link recovery status. Is that okay?
> > > > 
> > > > Good question :) Looking at the ECR, does it actually say anywhere that the
> > > > CM should needs to re-enable it? I'm thinking that we could just disable it
> > > > and be done with it?
> > > 
> > > Right. Thanks for clearing that up.
> > 
> > I'll check with the HW/FW folks still if they have any suggestions. I'll
> > keep you updated.
> 
> Did not hear anything from them yet but I adjusted the patch slightly to
> restore ELR if tb_switch_set_link_width() and also added back the check > 0
> which I missed. Let me know if you find issues with this one. Thanks!

Okay got response now. It turns out this feature is something CM needs to
enable if needed (e.g it is not automatically enabled by any router) and we
don't do that which means this patch is not necessary, sorry. I'm going to
drop it for now. Let me know if you think otherwise. Thanks!

Re: [PATCH v4] thunderbolt: Disable Gen 4 Recovery on Asymmetric Transitions

[Mohammad Rahimi]

Hello dear Mika.

On Tue, Feb 04, 2025 at 01:39:55PM GMT, Mika Westerberg wrote:
> Hi Again,
> 
> On Tue, Feb 04, 2025 at 01:16:25PM +0200, Mika Westerberg wrote:
> > On Mon, Feb 03, 2025 at 02:59:07PM +0200, Mika Westerberg wrote:
> > > On Mon, Feb 03, 2025 at 12:43:04PM +0000, Mohammad Rahimi wrote:
> > > > Hello again.
> > > > 
> > > > On Mon, Feb 03, 2025 at 02:39:44PM GMT, Mika Westerberg wrote:
> > > > > Hi,
> > > > > 
> > > > > On Mon, Feb 03, 2025 at 12:25:21PM +0000, Mohammad Rahimi wrote:
> > > > > > Hello.
> > > > > > 
> > > > > > On Mon, Feb 03, 2025 at 10:42:24AM GMT, Mika Westerberg wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Fri, Jan 31, 2025 at 01:41:27AM +0000, Mohammad Rahimi wrote:
> > > > > > > > Updates the Connection Manager to disable the Gen 4 Link Recovery
> > > > > > > > flow before transitioning from a Symmetric Link to an Asymmetric
> > > > > > > > Link, as specified in recent changes to the USB4 v2 specification.
> > > > > > > > 
> > > > > > > > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM"
> > > > > > > > published in September 2024, the rationale for this change is:
> > > > > > > > 
> > > > > > > >   "Since the default value of the Target Asymmetric Link might be
> > > > > > > >   different than Symmetric Link and Gen 4 Link Recovery flow checks
> > > > > > > >   this field to make sure it matched the actual Negotiated Link Width,
> > > > > > > >   we’re removing the condition for a Disconnect in the Gen 4 Link
> > > > > > > >   Recovery flow when Target Asymmetric Link doesn’t match the actual
> > > > > > > >   Link width and adding a Connection Manager note to Disable Gen 4 Link
> > > > > > > >   Recovery flow before doing Asymmetric Transitions."
> > > > > > > > 
> > > > > > > > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> > > > > > > 
> > > > > > > I did some minor modifications and applied to thunderbolt.git/next. Please
> > > > > > > let me know if I missed something.
> > > > > > > 
> > > > > > 
> > > > > > Looks great. Just one question:
> > > > > > 
> > > > > > In tb_configure_asym(), if tb_switch_set_link_width() fails, we won’t restore
> > > > > > the link recovery status. Is that okay?
> > > > > 
> > > > > Good question :) Looking at the ECR, does it actually say anywhere that the
> > > > > CM should needs to re-enable it? I'm thinking that we could just disable it
> > > > > and be done with it?
> > > > 
> > > > Right. Thanks for clearing that up.
> > > 
> > > I'll check with the HW/FW folks still if they have any suggestions. I'll
> > > keep you updated.
> > 
> > Did not hear anything from them yet but I adjusted the patch slightly to
> > restore ELR if tb_switch_set_link_width() and also added back the check > 0
> > which I missed. Let me know if you find issues with this one. Thanks!
> 
> Okay got response now. It turns out this feature is something CM needs to
> enable if needed (e.g it is not automatically enabled by any router) and we
> don't do that which means this patch is not necessary, sorry. I'm going to
> drop it for now. Let me know if you think otherwise. Thanks!

Oh, I also overlooked that CM has full discretion to enable or disable it.
Anyway, thank you for keeping me in the loop and for all your help with this patch.