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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello, thank you for your prompt reply!

On Wed, Jan 29, 2025 at 01:48:04PM GMT, Mika Westerberg wrote:
> Hi,
> 
> On Wed, Jan 29, 2025 at 11:24:33AM +0000, Mohammad Rahimi wrote:
> > 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@xxxxxxxxx>
> > > > ---
> > > >  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?
> 
> I missed the fact that you are using the positive return value to indicated
> whether link recovery was enabled.
> 
> The port functions can return the previous value like:
> 
> 	ret = usb4_port_link_recovery_enable(up);
> 
> then ret contains < 0 in case of error, 0 if it was not enabled > 0 if it
> was.
> 
> > > > +}
> > > > +
> > > > +/**
> > > > + * 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);
> 
> Yes.
> 
> > And two functions in tb.c:
> > 
> > int tb_enable_link_recovery(struct tb_switch *sw);
> > int tb_disable_link_recovery(struct tb_switch *sw);
> > 
> 
> It can even be one function to avoid duplicating code.
> 
> > Both will return 0 on success.
> > 
> > In tb_configure_asym(), I won’t be using the return value from
> > either of these two functions.
> 
> It should be checking the the return value and act accordingly. For example
> what happens if disabling link recovery fails? Is that error situation or
> should we continue configuring asymmetric link? Think also from users'
> perspective.
> 

Thanks again for the clear explanation. I will do that.

> > > > +
> > > > +/**
> > > > + * 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.
> 
> Yeah but you copied
> 
> * 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.
> 
> and that does not even apply for USB4 v2 host routers by default since we
> don't do discovery by default.
> 

My bad. Didn't know.

> > I should have mentioned this in my first email, unfortunately,
> > I don’t have access to any Gen 4 hosts or devices for testing.
> 
> I think this needs to get some testing on real hardware. I have here some
> so I can try the next version.

Great! Could you let me know what you are using for testing?





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux