Message-ID: <tycnnkwktd4iryegey2xysib3przadada5o7xuqrt5qfxqdtdc@22gg5ssed6sc>
Date: Wed, 29 Jan 2025 11:24:33 +0000
From: Mohammad Rahimi <rahimi.mhmmd@...il.com>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: andreas.noever@...il.com, michael.jamet@...el.com, 
	YehezkelShB@...il.com, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] thunderbolt: Disable Gen 4 Recovery on Asymmetric
 Transitions

Hello! Thanks for the thorough review.

On Wed, Jan 29, 2025 at 08:31:48AM GMT, Mika Westerberg wrote:
> Hi,
> 
> On Tue, Jan 28, 2025 at 04:36:05PM +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@...il.com>
> > ---
> >  drivers/thunderbolt/tb.c      |  23 ++++---
> >  drivers/thunderbolt/tb.h      |   3 +
> >  drivers/thunderbolt/tb_regs.h |   1 +
> >  drivers/thunderbolt/usb4.c    | 125 ++++++++++++++++++++++++++++++++++
> >  4 files changed, 142 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index a7c6919fbf97..da53e4619eca 100644
> > --- a/drivers/thunderbolt/tb.c
> > +++ b/drivers/thunderbolt/tb.c
> > @@ -1013,7 +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 clx = false, clx_disabled = false, downstream;
> > +	bool clx_was_enable = false, lrf_was_enable = false, downstream;
> 
> Let's call it link_recovery instead of lrf.
> 
> >  	struct tb_switch *sw;
> >  	struct tb_port *up;
> >  	int ret = 0;
> > @@ -1075,14 +1075,12 @@ 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;
> > -		}
> > +		clx_was_enable = tb_disable_clx(sw);
> > +		lrf_was_enable = usb4_disable_lrf(sw);
> 
> The naming is off here and I suggest to do this per USB4 port not per
> router. See below.
> 

Sure, I'll handle it per port. Please see my question below.

> >  		tb_sw_dbg(up->sw, "configuring asymmetric link\n");
> >  
> > @@ -1097,9 +1095,14 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
> >  		}
> >  	}
> >  
> > -	/* Re-enable CL states if they were previosly enabled */
> > -	if (clx)
> > +	/*
> > +	 * Re-enable CL states and Link Recovery flow if
> > +	 * they were previosly enabled
> > +	 */
> > +	if (clx_was_enable)
> >  		tb_enable_clx(sw);
> > +	if (lrf_was_enable)
> > +		usb4_enable_lrf(sw);
> >  
> >  	return ret;
> >  }
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index ddbf0cd78377..3bec35f78d51 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -1336,6 +1336,9 @@ 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);
> >  
> > +bool usb4_enable_lrf(struct tb_switch *sw);
> 
> int usb4_port_link_recovery_enable(struct tb_port *port);
> 
> > +bool usb4_disable_lrf(struct tb_switch *sw);
> 
> void usb4_port_link_recovery_disable(struct tb_port *port);
> 

Here, you have suggested to return int for enabling but void for disabling.
Let’s also check my next comment.

> and move these above the asym port functions.
> 
> > +
> >  /**
> >   * enum tb_sb_target - Sideband transaction target
> >   * @USB4_SB_TARGET_ROUTER: Target is the router itself
> > 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..49dd3d201617 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,130 @@ bool usb4_port_clx_supported(struct tb_port *port)
> >  	return !!(val & PORT_CS_18_CPS);
> >  }
> >  
> > +static int __usb4_port_lrf_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, "ELR %s\n", str_enabled_disabled(enable));
> 
> tb_port_dbg(port, "link recovery %s\n", ...)
> 
> Ditto below.
> 
> > +	return 0;
> > +}
> > +
> > +static int usb4_switch_lrf_enable(struct tb_switch *sw)
> > +{
> > +	bool was_enable = false;
> > +	struct tb_port *up, *down;
> > +	int ret;
> 
> Reverse christmas tree:
> 
> 	struct tb_port *up, *down;
> 	bool was_enable = false;
> 	int ret;
> 
> Ditto everywhere.
> 
> > +
> > +	up = tb_upstream_port(sw);
> > +	down = tb_switch_downstream_port(sw);
> > +
> > +	ret = __usb4_port_lrf_enable(up, true, &was_enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __usb4_port_lrf_enable(down, true, &was_enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tb_sw_dbg(sw, "ELR %s\n", str_enabled_disabled(true));
> > +
> > +	return 0;
> > +}
> > +
> > +static int usb4_switch_lrf_disable(struct tb_switch *sw)
> > +{
> > +	bool was_enable = false;
> > +	struct tb_port *up, *down;
> > +	int ret;
> > +
> > +	up = tb_upstream_port(sw);
> > +	down = tb_switch_downstream_port(sw);
> > +
> > +	ret = __usb4_port_lrf_enable(up, false, &was_enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __usb4_port_lrf_enable(down, false, &was_enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	tb_sw_dbg(sw, "ELR %s\n", str_enabled_disabled(false));
> > +
> > +	/* At least one ELR has been disabled */
> > +	return was_enable ? 1 : 0;
> 
> Just return 0 in case of success.
> 

If we return 0 on success when disabling Link Recovery, we won’t be able
to restore ELR to its previous state before the asymmetric transition.
As a result, enabling Link Recovery will become a side effect of
asymmetric transitions. I’m not sure—is that acceptable?

> > +}
> > +
> > +/**
> > + * usb4_disable_lrf() - Enables Link Recovery flow up to host router
> > + * @sw: Router to start
> > + *
> > + * Enables Link Recovery flow from @sw up to the host router.
> > + * Returns true if every Link Recovery flow was successfully enabled.
> > + */
> > +bool usb4_enable_lrf(struct tb_switch *sw)
> > +{
> > +	bool enabled = true;
> > +
> > +	do {
> > +		if (usb4_switch_lrf_enable(sw) < 0) {
> > +			tb_sw_warn(sw, "failed to enable Link Recovery flow\n");
> > +			enabled = false;
> > +		}
> > +
> > +		sw = tb_switch_parent(sw);
> > +	} while (sw);
> > +
> > +	return enabled;
> > +}
> 
> This should go to tb.c and called tb_enable_link_recovery() analogous to
> tb_enable_clx() and it calls the low-level usb4_port_link_recovery..
> functions.
> 
> Ditto below.
> 

Just to confirm my understanding, here’s what I plan to do based on
your instructions:

I’ll add two port-level functions in usb4.c:

int usb4_port_link_recovery_enable(struct tb_port *port);
void usb4_port_link_recovery_disable(struct tb_port *port);

And two functions in tb.c:

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

Both will return 0 on success.

In tb_configure_asym(), I won’t be using the return value from
either of these two functions.

> > +
> > +/**
> > + * usb4_disable_lrf() - Disable Link Recovery flow up to host router
> > + * @sw: Router to start
> > + *
> > + * Disables Link Recovery flow from @sw up to the host router.
> > + * Returns true if any Link Recovery flow was disabled. This
> > + * can be used to figure out whether the link was setup by us
> > + * or the boot firmware so we don't accidentally enable them if
> > + * they were not enabled during discovery.
> 
> Okay I think you copied the CLx part here, no? How did you test this?
> 

The way we should handle Link Recovery seems quite similar to CL states.

I should have mentioned this in my first email, unfortunately,
I don’t have access to any Gen 4 hosts or devices for testing.

> > + */
> > +bool usb4_disable_lrf(struct tb_switch *sw)
> > +{
> > +	bool disabled = false;
> > +
> > +	do {
> > +		int ret;
> > +
> > +		ret = usb4_switch_lrf_disable(sw);
> > +		if (ret > 0)
> > +			disabled = true;
> > +		else if (ret < 0)
> > +			tb_sw_warn(sw, "Link Recovery flow cannot be disabled\n");
> > +
> > +		sw = tb_switch_parent(sw);
> > +	} while (sw);
> > +
> > +	return disabled;
> > +}
> > +
> >  /**
> >   * usb4_port_asym_supported() - If the port supports asymmetric link
> >   * @port: USB4 port
> > -- 
> > 2.45.2

Powered by blists - more mailing lists