| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250129063148.GT3713119@black.fi.intel.com>
Date: Wed, 29 Jan 2025 08:31:48 +0200
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Mohammad Rahimi <rahimi.mhmmd@...il.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
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.
> 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);
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.
> +}
> +
> +/**
> + * 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.
> +
> +/**
> + * 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?
> + */
> +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