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

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

[Mohammad Rahimi]

Dear Mika,

In this revision, I have decided not to define the following functions,
despite our initial agreement in v1:

int tb_enable_link_recovery(struct tb_switch *sw);
int tb_disable_link_recovery(struct tb_switch *sw);

This decision was based on the following considerations:

1. If a write operation fails, there is no clear way to determine which port
the failure occurred on, the upstream port of the switch or its link partner.

2. To restore the exact Link Recovery state after an asymmetric transition, I
would need to track two boolean parameters and pass them to
tb_disable_link_recovery(). This would expose port state logic to
tb_configure_asym(), ending up with the same design we already have.

Additionally, I have chosen to ignore the return value from
usb4_port_link_recovery_enable() in tb_configure_asym(). At that stage, we
cannot take meaningful action based on the return value. Moreover, re-enabling
is performed immediately after tb_switch_set_link_width() to avoid introducing
unnecessary conditional branches.

Please let me know if you have any concerns or suggestions.

Best regards,
Mohammad Rahimi

[PATCH v2] 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      | 28 +++++++++++++++++--
 drivers/thunderbolt/tb.h      |  3 ++
 drivers/thunderbolt/tb_regs.h |  1 +
 drivers/thunderbolt/usb4.c    | 52 +++++++++++++++++++++++++++++++++++
 4 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index a7c6919fbf97..31a8269a5156 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,29 @@ 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 flow 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;
 		}
 
+		ret = usb4_port_link_recovery_disable(up);
+		if (ret < 0) {
+			tb_port_warn(up, "failed to disable the link recovery\n");
+			break;
+		}
+		link_recovery_up = ret > 0;
+
+		ret = usb4_port_link_recovery_disable(down);
+		if (ret < 0) {
+			tb_port_warn(down, "failed to disable the link recovery\n");
+			break;
+		}
+		link_recovery_down = ret > 0;
+
 		tb_sw_dbg(up->sw, "configuring asymmetric link\n");
 
 		/*
@@ -1091,6 +1106,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 flow if they were previosly 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..99fd6aa1704e 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,57 @@ 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;
+
+	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 -ERRNO on failure, otherwise the previous state of the Link Recovery.
+ */
+int usb4_port_link_recovery_enable(struct tb_port *port)
+{
+	return __usb4_port_link_recovery_enable(port, true);
+}
+
+/**
+ * usb4_port_link_recovery_disable() - Disable the Link Recovery
+ * @port: USB4 port
+ *
+ * Disables the Link Recovery for @port.
+ * Returns -ERRNO on failure, otherwise the previous state of the Link Recovery.
+ */
+int usb4_port_link_recovery_disable(struct tb_port *port)
+{
+	return __usb4_port_link_recovery_enable(port, false);
+}
+
 /**
  * usb4_port_asym_supported() - If the port supports asymmetric link
  * @port: USB4 port
-- 
2.45.2

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

[Mika Westerberg]

Hi,

On Thu, Jan 30, 2025 at 09:51:09AM +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."

Looks good in general few comments still see below.

> Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> ---
>  drivers/thunderbolt/tb.c      | 28 +++++++++++++++++--
>  drivers/thunderbolt/tb.h      |  3 ++
>  drivers/thunderbolt/tb_regs.h |  1 +
>  drivers/thunderbolt/usb4.c    | 52 +++++++++++++++++++++++++++++++++++
>  4 files changed, 81 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index a7c6919fbf97..31a8269a5156 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,29 @@ 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 flow 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;
>  		}
>  
> +		ret = usb4_port_link_recovery_disable(up);
> +		if (ret < 0) {
> +			tb_port_warn(up, "failed to disable the link recovery\n");

failed to disable link recovery

also should we still continue? How catastrophic this actually is? I mean
this now prevents the user from using her new UHBR monitor even though it
probably works just fine even if we don't do this step.

> +			break;
> +		}
> +		link_recovery_up = ret > 0;
> +
> +		ret = usb4_port_link_recovery_disable(down);
> +		if (ret < 0) {
> +			tb_port_warn(down, "failed to disable the link recovery\n");

failed to disable link recovery

and actually you can move this message into the function itself and drop
these messages here.

> +			break;
> +		}
> +		link_recovery_down = ret > 0;
> +
>  		tb_sw_dbg(up->sw, "configuring asymmetric link\n");
>  
>  		/*
> @@ -1091,6 +1106,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 flow if they were previosly enabled */

previously

> +		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..99fd6aa1704e 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,57 @@ 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;

I think you should check here

	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 -ERRNO on failure, otherwise the previous state of the Link Recovery.

Returns errno on failure..

or you can also specify which errors but then use %-EINVAL and so on.

Ditto to the other function.

> + */
> +int usb4_port_link_recovery_enable(struct tb_port *port)
> +{
> +	return __usb4_port_link_recovery_enable(port, true);
> +}
> +
> +/**
> + * usb4_port_link_recovery_disable() - Disable the Link Recovery
> + * @port: USB4 port
> + *
> + * Disables the Link Recovery for @port.
> + * Returns -ERRNO on failure, otherwise the previous state of the Link Recovery.
> + */
> +int usb4_port_link_recovery_disable(struct tb_port *port)
> +{
> +	return __usb4_port_link_recovery_enable(port, false);
> +}
> +
>  /**
>   * usb4_port_asym_supported() - If the port supports asymmetric link
>   * @port: USB4 port
> -- 
> 2.45.2

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

[Mohammad Rahimi]

Hello again, and thank you. I have three comments below:

On Thu, Jan 30, 2025 at 02:47:16PM GMT, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Jan 30, 2025 at 09:51:09AM +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."
> 
> Looks good in general few comments still see below.
> 
> > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@gmail.com>
> > ---
> >  drivers/thunderbolt/tb.c      | 28 +++++++++++++++++--
> >  drivers/thunderbolt/tb.h      |  3 ++
> >  drivers/thunderbolt/tb_regs.h |  1 +
> >  drivers/thunderbolt/usb4.c    | 52 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 81 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index a7c6919fbf97..31a8269a5156 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,29 @@ 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 flow 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;
> >  		}
> >  
> > +		ret = usb4_port_link_recovery_disable(up);
> > +		if (ret < 0) {
> > +			tb_port_warn(up, "failed to disable the link recovery\n");
> 
> failed to disable link recovery
> 
> also should we still continue? How catastrophic this actually is? I mean
> this now prevents the user from using her new UHBR monitor even though it
> probably works just fine even if we don't do this step.
> 

I followed your suggestion and here is my analysis:

Before this change, if an event occurred during the transition from
Symmetric to Asymmetric mode with ELR set, the link would enter the
Disconnect flow. According to section 4.4.5.2, the Router must then
reset the Target Link Width and Target Asymmetric Link to their
default values, effectively restarting the transition process.

This reset happens only if, during the transition, one of the conditions
for Link Recovery is met. Regardless of whether the link goes through
Link Recovery or the Disconnect flow, it will eventually resume normal operation.

The only problematic scenario I see is a device that consistently triggers
Link Recovery during the transition from Symmetric to Asymmetric, potentially
causing the LLSM (section 4.2) to enter an infinite loop. Such a condition
should have been caught during compliance testing.

To address this, I followed your suggestion and made errors from disabling
Link Recovery non-fatal during the transition.

> > +			break;
> > +		}
> > +		link_recovery_up = ret > 0;
> > +
> > +		ret = usb4_port_link_recovery_disable(down);
> > +		if (ret < 0) {
> > +			tb_port_warn(down, "failed to disable the link recovery\n");
> 
> failed to disable link recovery
> 
> and actually you can move this message into the function itself and drop
> these messages here.
> 
> > +			break;
> > +		}
> > +		link_recovery_down = ret > 0;
> > +
> >  		tb_sw_dbg(up->sw, "configuring asymmetric link\n");
> >  
> >  		/*
> > @@ -1091,6 +1106,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 flow if they were previosly enabled */
> 
> previously
> 

Thanks for catching that.

> > +		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..99fd6aa1704e 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,57 @@ 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;
> 
> I think you should check here
> 
> 	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 -ERRNO on failure, otherwise the previous state of the Link Recovery.
> 
> Returns errno on failure..
> 
> or you can also specify which errors but then use %-EINVAL and so on.
> 
> Ditto to the other function.
> 

I traced all the function calls and identified the following three error numbers:
-EINVAL
-ENODEV
-ETIMEDOUT

> > + */
> > +int usb4_port_link_recovery_enable(struct tb_port *port)
> > +{
> > +	return __usb4_port_link_recovery_enable(port, true);
> > +}
> > +
> > +/**
> > + * usb4_port_link_recovery_disable() - Disable the Link Recovery
> > + * @port: USB4 port
> > + *
> > + * Disables the Link Recovery for @port.
> > + * Returns -ERRNO on failure, otherwise the previous state of the Link Recovery.
> > + */
> > +int usb4_port_link_recovery_disable(struct tb_port *port)
> > +{
> > +	return __usb4_port_link_recovery_enable(port, false);
> > +}
> > +
> >  /**
> >   * usb4_port_asym_supported() - If the port supports asymmetric link
> >   * @port: USB4 port
> > -- 
> > 2.45.2