[PATCH 0/6] thunderbolt: Improvements for PM and USB4 compatibility

[PATCH 0/6] thunderbolt: Improvements for PM and USB4 compatibility

[Mika Westerberg]

Hi all,

This series consists of improvements around power management and USB4
compatibility. We also add debug logging for the DisplayPort resource
allocation.

Mika Westerberg (6):
  thunderbolt: Runtime PM activate both ends of the device link
  thunderbolt: Tear down existing tunnels when resuming from hibernate
  thunderbolt: Runtime resume USB4 port when retimers are scanned
  thunderbolt: Do not allow subtracting more NFC credits than configured
  thunderbolt: Do not program path HopIDs for USB4 routers
  thunderbolt: Add debug logging of DisplayPort resource allocation

 drivers/thunderbolt/acpi.c    | 13 +++++++
 drivers/thunderbolt/path.c    | 42 +++++++++++++---------
 drivers/thunderbolt/retimer.c | 28 +++++++++------
 drivers/thunderbolt/switch.c  | 27 ++++++++++++--
 drivers/thunderbolt/tb.c      | 68 ++++++++++++++++++++++++++---------
 drivers/thunderbolt/tb.h      |  5 ++-
 drivers/thunderbolt/tunnel.c  | 27 ++++++++------
 drivers/thunderbolt/tunnel.h  |  9 +++--
 8 files changed, 159 insertions(+), 60 deletions(-)

-- 
2.33.0

[PATCH 1/6] thunderbolt: Runtime PM activate both ends of the device link

[Mika Westerberg]

If protocol tunnels are already up when the driver is loaded, for
instance if the boot firmware implements connection manager of its own,
runtime PM reference count of the consumer devices behind the tunnel
might have been increased already before the device link is created but
the supplier device runtime PM reference count is not. This leads to a
situation where the supplier (the Thunderbolt driver) can runtime
suspend even if it should not because the corresponding protocol tunnel
needs to be up causing the devices to be removed from the corresponding
native bus.

Prevent this from happening by making both sides of the link runtime PM
active briefly. The pm_runtime_put() for the consumer (PCIe
root/downstream port, xHCI) then allows it to runtime suspend again but
keeps the supplier runtime resumed the whole time it is runtime active.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/acpi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
index b67e72d5644b..7c9597a33929 100644
--- a/drivers/thunderbolt/acpi.c
+++ b/drivers/thunderbolt/acpi.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/pm_runtime.h>
 
 #include "tb.h"
 
@@ -74,8 +75,18 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
 		 pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM))) {
 		const struct device_link *link;
 
+		/*
+		 * Make them both active first to make sure the NHI does
+		 * not runtime suspend before the consumer. The
+		 * pm_runtime_put() below then allows the consumer to
+		 * runtime suspend again (which then allows NHI runtime
+		 * suspend too now that the device link is established).
+		 */
+		pm_runtime_get_sync(&pdev->dev);
+
 		link = device_link_add(&pdev->dev, &nhi->pdev->dev,
 				       DL_FLAG_AUTOREMOVE_SUPPLIER |
+				       DL_FLAG_RPM_ACTIVE |
 				       DL_FLAG_PM_RUNTIME);
 		if (link) {
 			dev_dbg(&nhi->pdev->dev, "created link from %s\n",
@@ -84,6 +95,8 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
 			dev_warn(&nhi->pdev->dev, "device link creation from %s failed\n",
 				 dev_name(&pdev->dev));
 		}
+
+		pm_runtime_put(&pdev->dev);
 	}
 
 out_put:
-- 
2.33.0

Re: [PATCH 1/6] thunderbolt: Runtime PM activate both ends of the device link

[Rafael J. Wysocki]

On Thursday, November 25, 2021 8:37:28 AM CET Mika Westerberg wrote:
> If protocol tunnels are already up when the driver is loaded, for
> instance if the boot firmware implements connection manager of its own,
> runtime PM reference count of the consumer devices behind the tunnel
> might have been increased already before the device link is created but
> the supplier device runtime PM reference count is not. This leads to a
> situation where the supplier (the Thunderbolt driver) can runtime
> suspend even if it should not because the corresponding protocol tunnel
> needs to be up causing the devices to be removed from the corresponding
> native bus.
> 
> Prevent this from happening by making both sides of the link runtime PM
> active briefly. The pm_runtime_put() for the consumer (PCIe
> root/downstream port, xHCI) then allows it to runtime suspend again but
> keeps the supplier runtime resumed the whole time it is runtime active.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks!

> ---
>  drivers/thunderbolt/acpi.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
> index b67e72d5644b..7c9597a33929 100644
> --- a/drivers/thunderbolt/acpi.c
> +++ b/drivers/thunderbolt/acpi.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "tb.h"
>  
> @@ -74,8 +75,18 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
>  		 pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM))) {
>  		const struct device_link *link;
>  
> +		/*
> +		 * Make them both active first to make sure the NHI does
> +		 * not runtime suspend before the consumer. The
> +		 * pm_runtime_put() below then allows the consumer to
> +		 * runtime suspend again (which then allows NHI runtime
> +		 * suspend too now that the device link is established).
> +		 */
> +		pm_runtime_get_sync(&pdev->dev);
> +
>  		link = device_link_add(&pdev->dev, &nhi->pdev->dev,
>  				       DL_FLAG_AUTOREMOVE_SUPPLIER |
> +				       DL_FLAG_RPM_ACTIVE |
>  				       DL_FLAG_PM_RUNTIME);
>  		if (link) {
>  			dev_dbg(&nhi->pdev->dev, "created link from %s\n",
> @@ -84,6 +95,8 @@ static acpi_status tb_acpi_add_link(acpi_handle handle, u32 level, void *data,
>  			dev_warn(&nhi->pdev->dev, "device link creation from %s failed\n",
>  				 dev_name(&pdev->dev));
>  		}
> +
> +		pm_runtime_put(&pdev->dev);
>  	}
>  
>  out_put:
> 

[PATCH 2/6] thunderbolt: Tear down existing tunnels when resuming from hibernate

[Mika Westerberg]

If the boot firmware implements connection manager of its own it may not
create the paths in the same way or order we do. For example it may
create first PCIe tunnel and the USB3 tunnel. When we restore our
tunnels (first de-activating them) we may be doing that over completely
different tunnel and that leaves them possible non-functional. For this
reason we re-use the tunnel discovery functionality and find out all the
existing tunnels, and tear them down. Once that is done we can restore
our tunnels.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/path.c   | 42 +++++++++++++---------
 drivers/thunderbolt/tb.c     | 68 +++++++++++++++++++++++++++---------
 drivers/thunderbolt/tb.h     |  5 ++-
 drivers/thunderbolt/tunnel.c | 27 ++++++++------
 drivers/thunderbolt/tunnel.h |  9 +++--
 5 files changed, 104 insertions(+), 47 deletions(-)

diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c
index 564e2f42cebd..299712accfe9 100644
--- a/drivers/thunderbolt/path.c
+++ b/drivers/thunderbolt/path.c
@@ -85,11 +85,12 @@ static int tb_path_find_src_hopid(struct tb_port *src,
  * @dst_hopid: HopID to the @dst (%-1 if don't care)
  * @last: Last port is filled here if not %NULL
  * @name: Name of the path
+ * @alloc_hopid: Allocate HopIDs for the ports
  *
  * Follows a path starting from @src and @src_hopid to the last output
- * port of the path. Allocates HopIDs for the visited ports. Call
- * tb_path_free() to release the path and allocated HopIDs when the path
- * is not needed anymore.
+ * port of the path. Allocates HopIDs for the visited ports (if
+ * @alloc_hopid is true). Call tb_path_free() to release the path and
+ * allocated HopIDs when the path is not needed anymore.
  *
  * Note function discovers also incomplete paths so caller should check
  * that the @dst port is the expected one. If it is not, the path can be
@@ -99,7 +100,8 @@ static int tb_path_find_src_hopid(struct tb_port *src,
  */
 struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
 				 struct tb_port *dst, int dst_hopid,
-				 struct tb_port **last, const char *name)
+				 struct tb_port **last, const char *name,
+				 bool alloc_hopid)
 {
 	struct tb_port *out_port;
 	struct tb_regs_hop hop;
@@ -156,6 +158,7 @@ struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
 	path->tb = src->sw->tb;
 	path->path_length = num_hops;
 	path->activated = true;
+	path->alloc_hopid = alloc_hopid;
 
 	path->hops = kcalloc(num_hops, sizeof(*path->hops), GFP_KERNEL);
 	if (!path->hops) {
@@ -177,13 +180,14 @@ struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
 			goto err;
 		}
 
-		if (tb_port_alloc_in_hopid(p, h, h) < 0)
+		if (alloc_hopid && tb_port_alloc_in_hopid(p, h, h) < 0)
 			goto err;
 
 		out_port = &sw->ports[hop.out_port];
 		next_hop = hop.next_hop;
 
-		if (tb_port_alloc_out_hopid(out_port, next_hop, next_hop) < 0) {
+		if (alloc_hopid &&
+		    tb_port_alloc_out_hopid(out_port, next_hop, next_hop) < 0) {
 			tb_port_release_in_hopid(p, h);
 			goto err;
 		}
@@ -263,6 +267,8 @@ struct tb_path *tb_path_alloc(struct tb *tb, struct tb_port *src, int src_hopid,
 		return NULL;
 	}
 
+	path->alloc_hopid = true;
+
 	in_hopid = src_hopid;
 	out_port = NULL;
 
@@ -345,17 +351,19 @@ struct tb_path *tb_path_alloc(struct tb *tb, struct tb_port *src, int src_hopid,
  */
 void tb_path_free(struct tb_path *path)
 {
-	int i;
-
-	for (i = 0; i < path->path_length; i++) {
-		const struct tb_path_hop *hop = &path->hops[i];
-
-		if (hop->in_port)
-			tb_port_release_in_hopid(hop->in_port,
-						 hop->in_hop_index);
-		if (hop->out_port)
-			tb_port_release_out_hopid(hop->out_port,
-						  hop->next_hop_index);
+	if (path->alloc_hopid) {
+		int i;
+
+		for (i = 0; i < path->path_length; i++) {
+			const struct tb_path_hop *hop = &path->hops[i];
+
+			if (hop->in_port)
+				tb_port_release_in_hopid(hop->in_port,
+							 hop->in_hop_index);
+			if (hop->out_port)
+				tb_port_release_out_hopid(hop->out_port,
+							  hop->next_hop_index);
+		}
 	}
 
 	kfree(path->hops);
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 2897a77d44c3..a231191b06c6 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -105,10 +105,11 @@ static void tb_remove_dp_resources(struct tb_switch *sw)
 	}
 }
 
-static void tb_discover_tunnels(struct tb_switch *sw)
+static void tb_switch_discover_tunnels(struct tb_switch *sw,
+				       struct list_head *list,
+				       bool alloc_hopids)
 {
 	struct tb *tb = sw->tb;
-	struct tb_cm *tcm = tb_priv(tb);
 	struct tb_port *port;
 
 	tb_switch_for_each_port(sw, port) {
@@ -116,24 +117,41 @@ static void tb_discover_tunnels(struct tb_switch *sw)
 
 		switch (port->config.type) {
 		case TB_TYPE_DP_HDMI_IN:
-			tunnel = tb_tunnel_discover_dp(tb, port);
+			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
 			break;
 
 		case TB_TYPE_PCIE_DOWN:
-			tunnel = tb_tunnel_discover_pci(tb, port);
+			tunnel = tb_tunnel_discover_pci(tb, port, alloc_hopids);
 			break;
 
 		case TB_TYPE_USB3_DOWN:
-			tunnel = tb_tunnel_discover_usb3(tb, port);
+			tunnel = tb_tunnel_discover_usb3(tb, port, alloc_hopids);
 			break;
 
 		default:
 			break;
 		}
 
-		if (!tunnel)
-			continue;
+		if (tunnel)
+			list_add_tail(&tunnel->list, list);
+	}
 
+	tb_switch_for_each_port(sw, port) {
+		if (tb_port_has_remote(port)) {
+			tb_switch_discover_tunnels(port->remote->sw, list,
+						   alloc_hopids);
+		}
+	}
+}
+
+static void tb_discover_tunnels(struct tb *tb)
+{
+	struct tb_cm *tcm = tb_priv(tb);
+	struct tb_tunnel *tunnel;
+
+	tb_switch_discover_tunnels(tb->root_switch, &tcm->tunnel_list, true);
+
+	list_for_each_entry(tunnel, &tcm->tunnel_list, list) {
 		if (tb_tunnel_is_pci(tunnel)) {
 			struct tb_switch *parent = tunnel->dst_port->sw;
 
@@ -146,13 +164,6 @@ static void tb_discover_tunnels(struct tb_switch *sw)
 			pm_runtime_get_sync(&tunnel->src_port->sw->dev);
 			pm_runtime_get_sync(&tunnel->dst_port->sw->dev);
 		}
-
-		list_add_tail(&tunnel->list, &tcm->tunnel_list);
-	}
-
-	tb_switch_for_each_port(sw, port) {
-		if (tb_port_has_remote(port))
-			tb_discover_tunnels(port->remote->sw);
 	}
 }
 
@@ -1369,7 +1380,7 @@ static int tb_start(struct tb *tb)
 	/* Full scan to discover devices added before the driver was loaded. */
 	tb_scan_switch(tb->root_switch);
 	/* Find out tunnels created by the boot firmware */
-	tb_discover_tunnels(tb->root_switch);
+	tb_discover_tunnels(tb);
 	/*
 	 * If the boot firmware did not create USB 3.x tunnels create them
 	 * now for the whole topology.
@@ -1429,6 +1440,8 @@ static int tb_resume_noirq(struct tb *tb)
 {
 	struct tb_cm *tcm = tb_priv(tb);
 	struct tb_tunnel *tunnel, *n;
+	unsigned int usb3_delay = 0;
+	LIST_HEAD(tunnels);
 
 	tb_dbg(tb, "resuming...\n");
 
@@ -1439,8 +1452,31 @@ static int tb_resume_noirq(struct tb *tb)
 	tb_free_invalid_tunnels(tb);
 	tb_free_unplugged_children(tb->root_switch);
 	tb_restore_children(tb->root_switch);
-	list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list)
+
+	/*
+	 * If we get here from suspend to disk the boot firmware or the
+	 * restore kernel might have created tunnels of its own. Since
+	 * we cannot be sure they are usable for us we find and tear
+	 * them down.
+	 */
+	tb_switch_discover_tunnels(tb->root_switch, &tunnels, false);
+	list_for_each_entry_safe_reverse(tunnel, n, &tunnels, list) {
+		if (tb_tunnel_is_usb3(tunnel))
+			usb3_delay = 500;
+		tb_tunnel_deactivate(tunnel);
+		tb_tunnel_free(tunnel);
+	}
+
+	/* Re-create our tunnels now */
+	list_for_each_entry_safe(tunnel, n, &tcm->tunnel_list, list) {
+		/* USB3 requires delay before it can be re-activated */
+		if (tb_tunnel_is_usb3(tunnel)) {
+			msleep(usb3_delay);
+			/* Only need to do it once */
+			usb3_delay = 0;
+		}
 		tb_tunnel_restart(tunnel);
+	}
 	if (!list_empty(&tcm->tunnel_list)) {
 		/*
 		 * the pcie links need some time to get going.
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 725104c83e3d..3fae40670b72 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -354,6 +354,7 @@ enum tb_path_port {
  *	      when deactivating this path
  * @hops: Path hops
  * @path_length: How many hops the path uses
+ * @alloc_hopid: Does this path consume port HopID
  *
  * A path consists of a number of hops (see &struct tb_path_hop). To
  * establish a PCIe tunnel two paths have to be created between the two
@@ -374,6 +375,7 @@ struct tb_path {
 	bool clear_fc;
 	struct tb_path_hop *hops;
 	int path_length;
+	bool alloc_hopid;
 };
 
 /* HopIDs 0-7 are reserved by the Thunderbolt protocol */
@@ -957,7 +959,8 @@ int tb_dp_port_enable(struct tb_port *port, bool enable);
 
 struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
 				 struct tb_port *dst, int dst_hopid,
-				 struct tb_port **last, const char *name);
+				 struct tb_port **last, const char *name,
+				 bool alloc_hopid);
 struct tb_path *tb_path_alloc(struct tb *tb, struct tb_port *src, int src_hopid,
 			      struct tb_port *dst, int dst_hopid, int link_nr,
 			      const char *name);
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index bb5cc480fc9a..a473cc7d9a8d 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -207,12 +207,14 @@ static int tb_pci_init_path(struct tb_path *path)
  * tb_tunnel_discover_pci() - Discover existing PCIe tunnels
  * @tb: Pointer to the domain structure
  * @down: PCIe downstream adapter
+ * @alloc_hopid: Allocate HopIDs from visited ports
  *
  * If @down adapter is active, follows the tunnel to the PCIe upstream
  * adapter and back. Returns the discovered tunnel or %NULL if there was
  * no tunnel.
  */
-struct tb_tunnel *tb_tunnel_discover_pci(struct tb *tb, struct tb_port *down)
+struct tb_tunnel *tb_tunnel_discover_pci(struct tb *tb, struct tb_port *down,
+					 bool alloc_hopid)
 {
 	struct tb_tunnel *tunnel;
 	struct tb_path *path;
@@ -233,7 +235,7 @@ struct tb_tunnel *tb_tunnel_discover_pci(struct tb *tb, struct tb_port *down)
 	 * case.
 	 */
 	path = tb_path_discover(down, TB_PCI_HOPID, NULL, -1,
-				&tunnel->dst_port, "PCIe Up");
+				&tunnel->dst_port, "PCIe Up", alloc_hopid);
 	if (!path) {
 		/* Just disable the downstream port */
 		tb_pci_port_enable(down, false);
@@ -244,7 +246,7 @@ struct tb_tunnel *tb_tunnel_discover_pci(struct tb *tb, struct tb_port *down)
 		goto err_free;
 
 	path = tb_path_discover(tunnel->dst_port, -1, down, TB_PCI_HOPID, NULL,
-				"PCIe Down");
+				"PCIe Down", alloc_hopid);
 	if (!path)
 		goto err_deactivate;
 	tunnel->paths[TB_PCI_PATH_DOWN] = path;
@@ -761,6 +763,7 @@ static int tb_dp_init_video_path(struct tb_path *path)
  * tb_tunnel_discover_dp() - Discover existing Display Port tunnels
  * @tb: Pointer to the domain structure
  * @in: DP in adapter
+ * @alloc_hopid: Allocate HopIDs from visited ports
  *
  * If @in adapter is active, follows the tunnel to the DP out adapter
  * and back. Returns the discovered tunnel or %NULL if there was no
@@ -768,7 +771,8 @@ static int tb_dp_init_video_path(struct tb_path *path)
  *
  * Return: DP tunnel or %NULL if no tunnel found.
  */
-struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in)
+struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in,
+					bool alloc_hopid)
 {
 	struct tb_tunnel *tunnel;
 	struct tb_port *port;
@@ -787,7 +791,7 @@ struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in)
 	tunnel->src_port = in;
 
 	path = tb_path_discover(in, TB_DP_VIDEO_HOPID, NULL, -1,
-				&tunnel->dst_port, "Video");
+				&tunnel->dst_port, "Video", alloc_hopid);
 	if (!path) {
 		/* Just disable the DP IN port */
 		tb_dp_port_enable(in, false);
@@ -797,14 +801,15 @@ struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in)
 	if (tb_dp_init_video_path(tunnel->paths[TB_DP_VIDEO_PATH_OUT]))
 		goto err_free;
 
-	path = tb_path_discover(in, TB_DP_AUX_TX_HOPID, NULL, -1, NULL, "AUX TX");
+	path = tb_path_discover(in, TB_DP_AUX_TX_HOPID, NULL, -1, NULL, "AUX TX",
+				alloc_hopid);
 	if (!path)
 		goto err_deactivate;
 	tunnel->paths[TB_DP_AUX_PATH_OUT] = path;
 	tb_dp_init_aux_path(tunnel->paths[TB_DP_AUX_PATH_OUT]);
 
 	path = tb_path_discover(tunnel->dst_port, -1, in, TB_DP_AUX_RX_HOPID,
-				&port, "AUX RX");
+				&port, "AUX RX", alloc_hopid);
 	if (!path)
 		goto err_deactivate;
 	tunnel->paths[TB_DP_AUX_PATH_IN] = path;
@@ -1343,12 +1348,14 @@ static void tb_usb3_init_path(struct tb_path *path)
  * tb_tunnel_discover_usb3() - Discover existing USB3 tunnels
  * @tb: Pointer to the domain structure
  * @down: USB3 downstream adapter
+ * @alloc_hopid: Allocate HopIDs from visited ports
  *
  * If @down adapter is active, follows the tunnel to the USB3 upstream
  * adapter and back. Returns the discovered tunnel or %NULL if there was
  * no tunnel.
  */
-struct tb_tunnel *tb_tunnel_discover_usb3(struct tb *tb, struct tb_port *down)
+struct tb_tunnel *tb_tunnel_discover_usb3(struct tb *tb, struct tb_port *down,
+					  bool alloc_hopid)
 {
 	struct tb_tunnel *tunnel;
 	struct tb_path *path;
@@ -1369,7 +1376,7 @@ struct tb_tunnel *tb_tunnel_discover_usb3(struct tb *tb, struct tb_port *down)
 	 * case.
 	 */
 	path = tb_path_discover(down, TB_USB3_HOPID, NULL, -1,
-				&tunnel->dst_port, "USB3 Down");
+				&tunnel->dst_port, "USB3 Down", alloc_hopid);
 	if (!path) {
 		/* Just disable the downstream port */
 		tb_usb3_port_enable(down, false);
@@ -1379,7 +1386,7 @@ struct tb_tunnel *tb_tunnel_discover_usb3(struct tb *tb, struct tb_port *down)
 	tb_usb3_init_path(tunnel->paths[TB_USB3_PATH_DOWN]);
 
 	path = tb_path_discover(tunnel->dst_port, -1, down, TB_USB3_HOPID, NULL,
-				"USB3 Up");
+				"USB3 Up", alloc_hopid);
 	if (!path)
 		goto err_deactivate;
 	tunnel->paths[TB_USB3_PATH_UP] = path;
diff --git a/drivers/thunderbolt/tunnel.h b/drivers/thunderbolt/tunnel.h
index eea14e24f7e0..03e56076b5bc 100644
--- a/drivers/thunderbolt/tunnel.h
+++ b/drivers/thunderbolt/tunnel.h
@@ -64,10 +64,12 @@ struct tb_tunnel {
 	int allocated_down;
 };
 
-struct tb_tunnel *tb_tunnel_discover_pci(struct tb *tb, struct tb_port *down);
+struct tb_tunnel *tb_tunnel_discover_pci(struct tb *tb, struct tb_port *down,
+					 bool alloc_hopid);
 struct tb_tunnel *tb_tunnel_alloc_pci(struct tb *tb, struct tb_port *up,
 				      struct tb_port *down);
-struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in);
+struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in,
+					bool alloc_hopid);
 struct tb_tunnel *tb_tunnel_alloc_dp(struct tb *tb, struct tb_port *in,
 				     struct tb_port *out, int max_up,
 				     int max_down);
@@ -77,7 +79,8 @@ struct tb_tunnel *tb_tunnel_alloc_dma(struct tb *tb, struct tb_port *nhi,
 				      int receive_ring);
 bool tb_tunnel_match_dma(const struct tb_tunnel *tunnel, int transmit_path,
 			 int transmit_ring, int receive_path, int receive_ring);
-struct tb_tunnel *tb_tunnel_discover_usb3(struct tb *tb, struct tb_port *down);
+struct tb_tunnel *tb_tunnel_discover_usb3(struct tb *tb, struct tb_port *down,
+					  bool alloc_hopid);
 struct tb_tunnel *tb_tunnel_alloc_usb3(struct tb *tb, struct tb_port *up,
 				       struct tb_port *down, int max_up,
 				       int max_down);
-- 
2.33.0

Re: [PATCH 2/6] thunderbolt: Tear down existing tunnels when resuming from hibernate

[Lukas Wunner]

On Thu, Nov 25, 2021 at 10:37:29AM +0300, Mika Westerberg wrote:
> If the boot firmware implements connection manager of its own it may not
> create the paths in the same way or order we do. For example it may
> create first PCIe tunnel and the USB3 tunnel. When we restore our
> tunnels (first de-activating them) we may be doing that over completely
> different tunnel and that leaves them possible non-functional. For this
> reason we re-use the tunnel discovery functionality and find out all the
> existing tunnels, and tear them down. Once that is done we can restore
> our tunnels.

Hm, what if the system is running from a Thunderbolt-attached drive?
Will the mount points survive tearing down and re-establishing the
tunnels to that drive?

Thanks,

Lukas

Re: [PATCH 2/6] thunderbolt: Tear down existing tunnels when resuming from hibernate

[Mika Westerberg]

Hi,

On Fri, Nov 26, 2021 at 09:01:50PM +0100, Lukas Wunner wrote:
> On Thu, Nov 25, 2021 at 10:37:29AM +0300, Mika Westerberg wrote:
> > If the boot firmware implements connection manager of its own it may not
> > create the paths in the same way or order we do. For example it may
> > create first PCIe tunnel and the USB3 tunnel. When we restore our
> > tunnels (first de-activating them) we may be doing that over completely
> > different tunnel and that leaves them possible non-functional. For this
> > reason we re-use the tunnel discovery functionality and find out all the
> > existing tunnels, and tear them down. Once that is done we can restore
> > our tunnels.
> 
> Hm, what if the system is running from a Thunderbolt-attached drive?
> Will the mount points survive tearing down and re-establishing the
> tunnels to that drive?

Yes, they should. PCI is waiting for the TBT to resume so it should not
notice this, and all the data is at point still synced out to the disks.

Re: [PATCH 2/6] thunderbolt: Tear down existing tunnels when resuming from hibernate

[Yehezkel Bernat]

On Mon, Nov 29, 2021 at 8:30 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Fri, Nov 26, 2021 at 09:01:50PM +0100, Lukas Wunner wrote:
> > On Thu, Nov 25, 2021 at 10:37:29AM +0300, Mika Westerberg wrote:
> > > If the boot firmware implements connection manager of its own it may not
> > > create the paths in the same way or order we do. For example it may
> > > create first PCIe tunnel and the USB3 tunnel. When we restore our

the -> then?

> > > tunnels (first de-activating them) we may be doing that over completely
> > > different tunnel and that leaves them possible non-functional. For this

tunnel -> tunnels? possible -> possibly?

> > > reason we re-use the tunnel discovery functionality and find out all the
> > > existing tunnels, and tear them down. Once that is done we can restore
> > > our tunnels.
> >
> > Hm, what if the system is running from a Thunderbolt-attached drive?
> > Will the mount points survive tearing down and re-establishing the
> > tunnels to that drive?
>
> Yes, they should. PCI is waiting for the TBT to resume so it should not
> notice this, and all the data is at point still synced out to the disks.

But the user will notice the screen flashing, probably?
Maybe we can continue using the already established tunnels after
discovering them?
Is this because the FW might not support the same set of functionality?

Re: [PATCH 2/6] thunderbolt: Tear down existing tunnels when resuming from hibernate

[Mika Westerberg]

Hi,

On Tue, Nov 30, 2021 at 08:25:40PM +0200, Yehezkel Bernat wrote:
> On Mon, Nov 29, 2021 at 8:30 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi,
> >
> > On Fri, Nov 26, 2021 at 09:01:50PM +0100, Lukas Wunner wrote:
> > > On Thu, Nov 25, 2021 at 10:37:29AM +0300, Mika Westerberg wrote:
> > > > If the boot firmware implements connection manager of its own it may not
> > > > create the paths in the same way or order we do. For example it may
> > > > create first PCIe tunnel and the USB3 tunnel. When we restore our
> 
> the -> then?
> 
> > > > tunnels (first de-activating them) we may be doing that over completely
> > > > different tunnel and that leaves them possible non-functional. For this
> 
> tunnel -> tunnels? possible -> possibly?

Indeed, I'll fix those :)

> 
> > > > reason we re-use the tunnel discovery functionality and find out all the
> > > > existing tunnels, and tear them down. Once that is done we can restore
> > > > our tunnels.
> > >
> > > Hm, what if the system is running from a Thunderbolt-attached drive?
> > > Will the mount points survive tearing down and re-establishing the
> > > tunnels to that drive?
> >
> > Yes, they should. PCI is waiting for the TBT to resume so it should not
> > notice this, and all the data is at point still synced out to the disks.
> 
> But the user will notice the screen flashing, probably?

They will notice flashing anyway because we jump from one kernel to
another (as this is suspend-to-disk which involves shutting down the
machine and booting to "fresh" resume kernel first). We actually tear
down all the DP tunnels before we even enter suspend-to-disk (see
81a2e3e49f1f ("thunderbolt: Tear down DP tunnels when suspending")).

> Maybe we can continue using the already established tunnels after
> discovering them?

Yes we could but that would require us to map the existing tunnels with
the ones we had prior, and also indentify any new tunnels or missing
ones. This makes it more complex, and the approach here seem to work
according to my testing :) I can look for that solution too if you think
it is necessary.

> Is this because the FW might not support the same set of functionality?

Yes, that too.

Re: [PATCH 2/6] thunderbolt: Tear down existing tunnels when resuming from hibernate

[Yehezkel Bernat]

On Wed, Dec 1, 2021 at 8:47 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> > > > > reason we re-use the tunnel discovery functionality and find out all the
> > > > > existing tunnels, and tear them down. Once that is done we can restore
> > > > > our tunnels.
> > > >
> > > > Hm, what if the system is running from a Thunderbolt-attached drive?
> > > > Will the mount points survive tearing down and re-establishing the
> > > > tunnels to that drive?
> > >
> > > Yes, they should. PCI is waiting for the TBT to resume so it should not
> > > notice this, and all the data is at point still synced out to the disks.
> >
> > But the user will notice the screen flashing, probably?
>
> They will notice flashing anyway because we jump from one kernel to
> another (as this is suspend-to-disk which involves shutting down the
> machine and booting to "fresh" resume kernel first). We actually tear
> down all the DP tunnels before we even enter suspend-to-disk (see
> 81a2e3e49f1f ("thunderbolt: Tear down DP tunnels when suspending")).
>

Ah, thanks.

> > Is this because the FW might not support the same set of functionality?
>
> Yes, that too.

OK

> > Maybe we can continue using the already established tunnels after
> > discovering them?
>
> Yes we could but that would require us to map the existing tunnels with
> the ones we had prior, and also indentify any new tunnels or missing
> ones. This makes it more complex, and the approach here seem to work
> according to my testing :) I can look for that solution too if you think
> it is necessary.

Following the previous points, I don't see any value in trying the
more complex solution.
Thanks!

[PATCH 3/6] thunderbolt: Runtime resume USB4 port when retimers are scanned

[Mika Westerberg]

Sometimes when plugging in a USB4 device we might see following error:

  thunderbolt 1-0:3.1: runtime PM trying to activate child device 1-0:3.1 but parent (usb4_port3) is not active

This happens because the parent USB4 port was still runtime suspended.
Fix this by runtime resuming the USB4 port before scanning the retimers
below it.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/retimer.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/thunderbolt/retimer.c b/drivers/thunderbolt/retimer.c
index 722694052f4a..8c29bd556ae0 100644
--- a/drivers/thunderbolt/retimer.c
+++ b/drivers/thunderbolt/retimer.c
@@ -324,15 +324,10 @@ struct device_type tb_retimer_type = {
 
 static int tb_retimer_add(struct tb_port *port, u8 index, u32 auth_status)
 {
-	struct usb4_port *usb4;
 	struct tb_retimer *rt;
 	u32 vendor, device;
 	int ret;
 
-	usb4 = port->usb4;
-	if (!usb4)
-		return -EINVAL;
-
 	ret = usb4_port_retimer_read(port, index, USB4_SB_VENDOR_ID, &vendor,
 				     sizeof(vendor));
 	if (ret) {
@@ -374,7 +369,7 @@ static int tb_retimer_add(struct tb_port *port, u8 index, u32 auth_status)
 	rt->port = port;
 	rt->tb = port->sw->tb;
 
-	rt->dev.parent = &usb4->dev;
+	rt->dev.parent = &port->usb4->dev;
 	rt->dev.bus = &tb_bus_type;
 	rt->dev.type = &tb_retimer_type;
 	dev_set_name(&rt->dev, "%s:%u.%u", dev_name(&port->sw->dev),
@@ -453,6 +448,13 @@ int tb_retimer_scan(struct tb_port *port, bool add)
 {
 	u32 status[TB_MAX_RETIMER_INDEX + 1] = {};
 	int ret, i, last_idx = 0;
+	struct usb4_port *usb4;
+
+	usb4 = port->usb4;
+	if (!usb4)
+		return 0;
+
+	pm_runtime_get_sync(&usb4->dev);
 
 	/*
 	 * Send broadcast RT to make sure retimer indices facing this
@@ -460,7 +462,7 @@ int tb_retimer_scan(struct tb_port *port, bool add)
 	 */
 	ret = usb4_port_enumerate_retimers(port);
 	if (ret)
-		return ret;
+		goto out;
 
 	/*
 	 * Enable sideband channel for each retimer. We can do this
@@ -490,8 +492,10 @@ int tb_retimer_scan(struct tb_port *port, bool add)
 			break;
 	}
 
-	if (!last_idx)
-		return 0;
+	if (!last_idx) {
+		ret = 0;
+		goto out;
+	}
 
 	/* Add on-board retimers if they do not exist already */
 	for (i = 1; i <= last_idx; i++) {
@@ -507,7 +511,11 @@ int tb_retimer_scan(struct tb_port *port, bool add)
 		}
 	}
 
-	return 0;
+out:
+	pm_runtime_mark_last_busy(&usb4->dev);
+	pm_runtime_put_autosuspend(&usb4->dev);
+
+	return ret;
 }
 
 static int remove_retimer(struct device *dev, void *data)
-- 
2.33.0

[PATCH 4/6] thunderbolt: Do not allow subtracting more NFC credits than configured

[Mika Westerberg]

This might happen if the boot firmware uses different amount of NFC
credits than what the router suggests, or we are dealing with pre-USB4
device.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/switch.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 3014146081c1..463cfdc0b42f 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -623,6 +623,9 @@ int tb_port_add_nfc_credits(struct tb_port *port, int credits)
 		return 0;
 
 	nfc_credits = port->config.nfc_credits & ADP_CS_4_NFC_BUFFERS_MASK;
+	if (credits < 0)
+		credits = max_t(int, -nfc_credits, credits);
+
 	nfc_credits += credits;
 
 	tb_port_dbg(port, "adding %d NFC credits to %lu", credits,
-- 
2.33.0

[PATCH 5/6] thunderbolt: Do not program path HopIDs for USB4 routers

[Mika Westerberg]

These fields are marked read-only for USB4 routers so do not touch them
in that case. Update the kernel-doc of tb_dp_port_set_hops() to reflect
this too.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/switch.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 463cfdc0b42f..e00f4b878b56 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1322,7 +1322,9 @@ int tb_dp_port_hpd_clear(struct tb_port *port)
  * @aux_tx: AUX TX Hop ID
  * @aux_rx: AUX RX Hop ID
  *
- * Programs specified Hop IDs for DP IN/OUT port.
+ * Programs specified Hop IDs for DP IN/OUT port. Can be called for USB4
+ * router DP adapters too but does not program the values as the fields
+ * are read-only.
  */
 int tb_dp_port_set_hops(struct tb_port *port, unsigned int video,
 			unsigned int aux_tx, unsigned int aux_rx)
@@ -1330,6 +1332,9 @@ int tb_dp_port_set_hops(struct tb_port *port, unsigned int video,
 	u32 data[2];
 	int ret;
 
+	if (tb_switch_is_usb4(port->sw))
+		return 0;
+
 	ret = tb_port_read(port, data, TB_CFG_PORT,
 			   port->cap_adap + ADP_DP_CS_0, ARRAY_SIZE(data));
 	if (ret)
-- 
2.33.0

[PATCH 6/6] thunderbolt: Add debug logging of DisplayPort resource allocation

[Mika Westerberg]

Add more debugging around DP resource allocation/de-allocation.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/thunderbolt/switch.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index e00f4b878b56..13f9230104d7 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -3056,9 +3056,20 @@ bool tb_switch_query_dp_resource(struct tb_switch *sw, struct tb_port *in)
  */
 int tb_switch_alloc_dp_resource(struct tb_switch *sw, struct tb_port *in)
 {
+	int ret;
+
 	if (tb_switch_is_usb4(sw))
-		return usb4_switch_alloc_dp_resource(sw, in);
-	return tb_lc_dp_sink_alloc(sw, in);
+		ret = usb4_switch_alloc_dp_resource(sw, in);
+	else
+		ret = tb_lc_dp_sink_alloc(sw, in);
+
+	if (ret)
+		tb_sw_warn(sw, "failed to allocate DP resource for port %d\n",
+			   in->port);
+	else
+		tb_sw_dbg(sw, "allocated DP resource for port %d\n", in->port);
+
+	return ret;
 }
 
 /**
@@ -3081,6 +3092,8 @@ void tb_switch_dealloc_dp_resource(struct tb_switch *sw, struct tb_port *in)
 	if (ret)
 		tb_sw_warn(sw, "failed to de-allocate DP resource for port %d\n",
 			   in->port);
+	else
+		tb_sw_dbg(sw, "released DP resource for port %d\n", in->port);
 }
 
 struct tb_sw_lookup {
-- 
2.33.0

Re: [PATCH 0/6] thunderbolt: Improvements for PM and USB4 compatibility

[Mika Westerberg]

On Thu, Nov 25, 2021 at 10:37:27AM +0300, Mika Westerberg wrote:
> Hi all,
> 
> This series consists of improvements around power management and USB4
> compatibility. We also add debug logging for the DisplayPort resource
> allocation.
> 
> Mika Westerberg (6):
>   thunderbolt: Runtime PM activate both ends of the device link
>   thunderbolt: Tear down existing tunnels when resuming from hibernate
>   thunderbolt: Runtime resume USB4 port when retimers are scanned
>   thunderbolt: Do not allow subtracting more NFC credits than configured
>   thunderbolt: Do not program path HopIDs for USB4 routers
>   thunderbolt: Add debug logging of DisplayPort resource allocation

Fixed the typos pointed out by Yehezkel and applied to
thunderbolt.git/next.