[PATCH 0/5] thunderbolt: Fix DP tunnel teardown while an async DPRX read is running

[PATCH 0/5] thunderbolt: Fix DP tunnel teardown while an async DPRX read is running

[Sven Peter]

Hi,

This series contains a bunch of fixes related to teardown while an async
DPRX read is running. I ran into some of these while bringing up the
Apple Silicon NHI and just haven't implemented DP tunneling support yet
such that the DPRX capabilities read can never complete and will always
time out. 

None of it is specific to that hardware though so I figured it
makes sense to already send them out.

Best,

Sven

Signed-off-by: Sven Peter <sven@kernel.org>
---
Sven Peter (5):
      thunderbolt: Fix tunnel reference leak when the DPRX work is not started
      thunderbolt: Hold a switch reference for each path hop
      thunderbolt: Fix domain reference leak when DPRX read is canceled
      thunderbolt: Don't access a DP tunnel after its DPRX read was canceled
      thunderbolt: Cancel the DPRX read when the domain is stopped

 drivers/thunderbolt/path.c   | 21 +++++++++++++++++++++
 drivers/thunderbolt/tb.c     | 23 +++++++++++++++++------
 drivers/thunderbolt/test.c   | 12 ++++++++++++
 drivers/thunderbolt/tunnel.c | 43 ++++++++++++++++++++++++++++++++-----------
 drivers/thunderbolt/tunnel.h |  1 +
 5 files changed, 83 insertions(+), 17 deletions(-)
---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260815-b4-tbt-fixes-a2689186016d

Best regards,
--  
Sven Peter <sven@kernel.org>

[PATCH 1/5] thunderbolt: Fix tunnel reference leak when the DPRX work is not started

[Sven Peter]

tb_dp_dprx_start always takes a tunnel reference which is only dropped
by dprx_work eventually. Tunnels that have no callback don't ever queue
that work and tb_dp_dprx_stop then has nothing to cancel. It however only
releases the reference if cancel_delayed_work returned true and the
reference is leaked then.

Fix this by only taking the reference when dprx_work is actually queued.

Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
Cc: stable@vger.kernel.org
Signed-off-by: Sven Peter <sven@kernel.org>
---
I didn't actually hit this on hardware but found it while fixing a domain
leak in the same area and that fix depends on this one.
---
 drivers/thunderbolt/tunnel.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index b7f32305f14a..50580ebdac4b 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -1113,15 +1113,14 @@ static void tb_dp_dprx_work(struct work_struct *work)
 
 static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
 {
-	/*
-	 * Bump up the reference to keep the tunnel around. It will be
-	 * dropped in tb_dp_dprx_stop() once the tunnel is deactivated.
-	 */
-	tb_tunnel_get(tunnel);
-
-	tunnel->dprx_started = true;
-
 	if (tunnel->callback) {
+		/*
+		 * Bump up the reference to keep the tunnel around until the
+		 * work has run or has been canceled.
+		 */
+		tb_tunnel_get(tunnel);
+
+		tunnel->dprx_started = true;
 		tunnel->dprx_timeout = dprx_timeout_to_ktime(dprx_timeout);
 		queue_delayed_work(tunnel->tb->wq, &tunnel->dprx_work, 0);
 		return -EINPROGRESS;

-- 
2.55.0

[PATCH 2/5] thunderbolt: Hold a switch reference for each path hop

[Sven Peter]

tb_stop drops the reference to all DP tunnels but does not deactivate
them, thus nothing cancels a dprx_work still in flight (which holds its
own tunnel reference) and the tunnel can outlive tb_switch_remove. The
HopID releases in tb_path_free then operate on freed IDAs and trigger
warnings like

  ida_free called for id=8 which is not allocated.

Take or release a reference for both ports of each hop whenever the
HopIDs are allocated or released to ensure they have the same lifetime.

The KUnit tests allocate their switches without ever registering them so
initialize the embedded struct device there as well to make these
references work.

Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
Cc: stable@vger.kernel.org
Signed-off-by: Sven Peter <sven@kernel.org>
---
 drivers/thunderbolt/path.c | 21 +++++++++++++++++++++
 drivers/thunderbolt/test.c | 12 ++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c
index b2c322e76b8a..02c5e7a2101e 100644
--- a/drivers/thunderbolt/path.c
+++ b/drivers/thunderbolt/path.c
@@ -196,6 +196,12 @@ struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
 		path->hops[i].out_port = out_port;
 		path->hops[i].next_hop_index = next_hop;
 
+		/* Keep the ports alive, see tb_path_free() */
+		if (alloc_hopid) {
+			tb_switch_get(path->hops[i].in_port->sw);
+			tb_switch_get(path->hops[i].out_port->sw);
+		}
+
 		tb_dump_hop(&path->hops[i], &hop);
 
 		h = next_hop;
@@ -323,6 +329,10 @@ struct tb_path *tb_path_alloc(struct tb *tb, struct tb_port *src, int src_hopid,
 		path->hops[i].out_port = out_port;
 		path->hops[i].next_hop_index = out_hopid;
 
+		/* Keep the ports alive, see tb_path_free() */
+		tb_switch_get(path->hops[i].in_port->sw);
+		tb_switch_get(path->hops[i].out_port->sw);
+
 		in_hopid = out_hopid;
 	}
 
@@ -356,6 +366,17 @@ void tb_path_free(struct tb_path *path)
 			if (hop->out_port)
 				tb_port_release_out_hopid(hop->out_port,
 							  hop->next_hop_index);
+			/*
+			 * Only drop the switch references after both HopIDs
+			 * have been released: the path may be freed after the
+			 * switch was already removed (e.g. asynchronous DP
+			 * tunnel teardown) and these references are what
+			 * keeps the ports and their HopID IDAs alive.
+			 */
+			if (hop->in_port)
+				tb_switch_put(hop->in_port->sw);
+			if (hop->out_port)
+				tb_switch_put(hop->out_port->sw);
 		}
 	}
 
diff --git a/drivers/thunderbolt/test.c b/drivers/thunderbolt/test.c
index 05652ee82fbf..034c56845380 100644
--- a/drivers/thunderbolt/test.c
+++ b/drivers/thunderbolt/test.c
@@ -33,6 +33,11 @@ static void kunit_ida_init(struct kunit *test, struct ida *ida)
 	kunit_alloc_resource(test, __ida_init, __ida_destroy, GFP_KERNEL, ida);
 }
 
+static void tb_test_switch_release(struct device *dev)
+{
+	/* The memory is owned by KUnit, nothing to do here */
+}
+
 static struct tb_switch *alloc_switch(struct kunit *test, u64 route,
 				      u8 upstream_port, u8 max_port_number)
 {
@@ -44,6 +49,13 @@ static struct tb_switch *alloc_switch(struct kunit *test, u64 route,
 	if (!sw)
 		return NULL;
 
+	/*
+	 * The paths take a reference to their switches and those devices
+	 * have to be initialized for that to work.
+	 */
+	sw->dev.release = tb_test_switch_release;
+	device_initialize(&sw->dev);
+
 	sw->config.upstream_port_number = upstream_port;
 	sw->config.depth = tb_route_length(route);
 	sw->config.route_hi = upper_32_bits(route);

-- 
2.55.0

[PATCH 3/5] thunderbolt: Fix domain reference leak when DPRX read is canceled

[Sven Peter]

tb_tunnel_one_dp takes a domain reference which is only dropped once
tb_dp_tunnel_active has run on the work queue. If that work is
cancelled that reference is leaked. Since
commit f5cc545f5969 ("thunderbolt: Wait for tb_domain_release() to
complete when driver is removed") instead of just leaking memory this
now also blocks in the completion wait forever when unbinding the
driver.

That reference only exists to keep the domain around while the DPRX work
is scheduled so let the work itself own it: take it in tb_dp_dprx_start
and drop it in both places that end the work. Get/put are then paired
inside the same file and it doesn't matter anymore if the callback ever
runs.

Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
Cc: stable@vger.kernel.org
Signed-off-by: Sven Peter <sven@kernel.org>
---
 drivers/thunderbolt/tb.c     |  6 +-----
 drivers/thunderbolt/tunnel.c | 12 +++++++++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index f43f2d952372..fb9da53fe391 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -1964,8 +1964,6 @@ static void tb_dp_tunnel_active(struct tb_tunnel *tunnel, void *data)
 		tb_dp_resource_unavailable(tb, in, "DPRX negotiation failed");
 	}
 	mutex_unlock(&tb->lock);
-
-	tb_domain_put(tb);
 }
 
 static void tb_tunnel_one_dp(struct tb *tb, struct tb_port *in,
@@ -2026,8 +2024,7 @@ static void tb_tunnel_one_dp(struct tb *tb, struct tb_port *in,
 	       available_up, available_down);
 
 	tunnel = tb_tunnel_alloc_dp(tb, in, out, link_nr, available_up,
-				    available_down, tb_dp_tunnel_active,
-				    tb_domain_get(tb));
+				    available_down, tb_dp_tunnel_active, tb);
 	if (!tunnel) {
 		tb_port_dbg(out, "could not allocate DP tunnel\n");
 		goto err_reclaim_usb;
@@ -2048,7 +2045,6 @@ static void tb_tunnel_one_dp(struct tb *tb, struct tb_port *in,
 	tb_tunnel_put(tunnel);
 err_reclaim_usb:
 	tb_reclaim_usb3_bandwidth(tb, in, out);
-	tb_domain_put(tb);
 err_detach_group:
 	tb_detach_bandwidth_group(in);
 err_dealloc_dp:
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 50580ebdac4b..82d9c0b556dd 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -1109,16 +1109,18 @@ static void tb_dp_dprx_work(struct work_struct *work)
 	if (tunnel->callback)
 		tunnel->callback(tunnel, tunnel->callback_data);
 	tb_tunnel_put(tunnel);
+	tb_domain_put(tb);
 }
 
 static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
 {
 	if (tunnel->callback) {
 		/*
-		 * Bump up the reference to keep the tunnel around until the
-		 * work has run or has been canceled.
+		 * Bump up the references to keep the tunnel and the domain
+		 * around until the work has run or has been canceled.
 		 */
 		tb_tunnel_get(tunnel);
+		tb_domain_get(tunnel->tb);
 
 		tunnel->dprx_started = true;
 		tunnel->dprx_timeout = dprx_timeout_to_ktime(dprx_timeout);
@@ -1132,11 +1134,15 @@ static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
 
 static void tb_dp_dprx_stop(struct tb_tunnel *tunnel)
 {
+	struct tb *tb = tunnel->tb;
+
 	if (tunnel->dprx_started) {
 		tunnel->dprx_started = false;
 		tunnel->dprx_canceled = true;
-		if (cancel_delayed_work(&tunnel->dprx_work))
+		if (cancel_delayed_work(&tunnel->dprx_work)) {
 			tb_tunnel_put(tunnel);
+			tb_domain_put(tb);
+		}
 	}
 }
 

-- 
2.55.0

[PATCH 4/5] thunderbolt: Don't access a DP tunnel after its DPRX read was canceled

[Sven Peter]

tb_dp_dprx_work checks dprx_canceled before it takes tb->lock so it
misses a tb_dp_dprx_stop that could not cancel the already running
work. It then polls the DPRX capabilities and runs the callback for a
tunnel that is being torn down and touches routers that may already be
gone after an unplug.

Check the flag with tb->lock held instead and check it again in
tb_dp_tunnel_active because the callback runs after the lock has been
dropped again.

Also clear the flag in tb_dp_dprx_start so that it only ever describes
the work that is currently in flight.

Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
Cc: stable@vger.kernel.org
Signed-off-by: Sven Peter <sven@kernel.org>
---
 drivers/thunderbolt/tb.c     | 12 ++++++++++++
 drivers/thunderbolt/tunnel.c | 11 +++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index fb9da53fe391..e368a6b53f64 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -1910,6 +1910,18 @@ static void tb_dp_tunnel_active(struct tb_tunnel *tunnel, void *data)
 	struct tb *tb = data;
 
 	mutex_lock(&tb->lock);
+
+	/*
+	 * If the DPRX read was canceled the tunnel is already being torn
+	 * down by whoever canceled it. Do not touch the adapters here
+	 * because the routers may be gone by now.
+	 */
+	if (tunnel->dprx_canceled) {
+		tb_tunnel_dbg(tunnel, "DPRX read canceled, not activating\n");
+		mutex_unlock(&tb->lock);
+		return;
+	}
+
 	if (tb_tunnel_is_active(tunnel)) {
 		int consumed_up, consumed_down, ret;
 
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 82d9c0b556dd..52fa90786ff8 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -1090,8 +1090,14 @@ static void tb_dp_dprx_work(struct work_struct *work)
 	struct tb_tunnel *tunnel = container_of(work, typeof(*tunnel), dprx_work.work);
 	struct tb *tb = tunnel->tb;
 
+	/*
+	 * The DPRX read can be canceled while this work is waiting for
+	 * tb->lock. Check the flag only once it is held: while the lock is
+	 * held the tunnel cannot be torn down under us and the adapters are
+	 * safe to access.
+	 */
+	mutex_lock(&tb->lock);
 	if (!tunnel->dprx_canceled) {
-		mutex_lock(&tb->lock);
 		if (tb_dp_is_usb4(tunnel->src_port->sw) &&
 		    tb_dp_wait_dprx(tunnel, TB_DPRX_WAIT_TIMEOUT)) {
 			if (ktime_before(ktime_get(), tunnel->dprx_timeout)) {
@@ -1103,8 +1109,8 @@ static void tb_dp_dprx_work(struct work_struct *work)
 		} else {
 			tb_tunnel_set_active(tunnel, true);
 		}
-		mutex_unlock(&tb->lock);
 	}
+	mutex_unlock(&tb->lock);
 
 	if (tunnel->callback)
 		tunnel->callback(tunnel, tunnel->callback_data);
@@ -1123,6 +1129,7 @@ static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
 		tb_domain_get(tunnel->tb);
 
 		tunnel->dprx_started = true;
+		tunnel->dprx_canceled = false;
 		tunnel->dprx_timeout = dprx_timeout_to_ktime(dprx_timeout);
 		queue_delayed_work(tunnel->tb->wq, &tunnel->dprx_work, 0);
 		return -EINPROGRESS;

-- 
2.55.0

[PATCH 5/5] thunderbolt: Cancel the DPRX read when the domain is stopped

[Sven Peter]

tb_stop only tears down DMA tunnels so a DP tunnel that is still
waiting for dprx_work to complete keeps that work queued while the
routers are removed and the control channel is stopped. The work only
stops once the DPRX timeout has passed and because it requeues itself
until then the flush_workqueue in tb_domain_remove won't wait for its
final run. The callback then runs against a domain that is already torn
down. A reference to that domain is kept so the completion waiting for
that domain to disappear in unbind will block until the timeout is
eventually reached.

Just cancel the work in tb_stop. This doesn't affect DP tunnels that are
already alive and keeps those displays working.

Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
Cc: stable@vger.kernel.org
Signed-off-by: Sven Peter <sven@kernel.org>
---
I also didn't run into this but noticed it when fixing the hop alloc thing
and think it makes sense to fix it anyway.
---
 drivers/thunderbolt/tb.c     | 5 ++++-
 drivers/thunderbolt/tunnel.c | 9 +++++++++
 drivers/thunderbolt/tunnel.h | 1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index e368a6b53f64..f7e68372da09 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -2958,10 +2958,13 @@ static void tb_stop(struct tb *tb)
 		/*
 		 * DMA tunnels require the driver to be functional so we
 		 * tear them down. Other protocol tunnels can be left
-		 * intact.
+		 * intact but a DPRX capabilities read that is still in
+		 * flight has to be canceled before the routers go away.
 		 */
 		if (tb_tunnel_is_dma(tunnel))
 			tb_tunnel_deactivate(tunnel);
+		else if (tb_tunnel_is_dp(tunnel))
+			tb_tunnel_cancel_dprx(tunnel);
 		tb_tunnel_put(tunnel);
 	}
 	tb_switch_remove(tb->root_switch);
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 52fa90786ff8..5b1ae5a0c12b 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -2487,6 +2487,15 @@ void tb_tunnel_deactivate(struct tb_tunnel *tunnel)
 	tb_tunnel_set_active(tunnel, false);
 }
 
+/**
+ * tb_tunnel_cancel_dprx() - Cancel the DPRX capabilities read work
+ * @tunnel: tunnel to cancel the DPRX capabilities read work for
+ */
+void tb_tunnel_cancel_dprx(struct tb_tunnel *tunnel)
+{
+	tb_dp_dprx_stop(tunnel);
+}
+
 /**
  * tb_tunnel_port_on_path() - Does the tunnel go through port
  * @tunnel: Tunnel to check
diff --git a/drivers/thunderbolt/tunnel.h b/drivers/thunderbolt/tunnel.h
index 4878763a82b3..9de5fac04269 100644
--- a/drivers/thunderbolt/tunnel.h
+++ b/drivers/thunderbolt/tunnel.h
@@ -138,6 +138,7 @@ struct tb_tunnel *tb_tunnel_alloc_usb3(struct tb *tb, struct tb_port *up,
 void tb_tunnel_put(struct tb_tunnel *tunnel);
 int tb_tunnel_activate(struct tb_tunnel *tunnel);
 void tb_tunnel_deactivate(struct tb_tunnel *tunnel);
+void tb_tunnel_cancel_dprx(struct tb_tunnel *tunnel);
 
 /**
  * tb_tunnel_is_active() - Is tunnel fully activated

-- 
2.55.0

Re: [PATCH 1/5] thunderbolt: Fix tunnel reference leak when the DPRX work is not started

[Mika Westerberg]

Hi,

On Mon, Aug 17, 2026 at 09:53:58PM +0200, Sven Peter wrote:
> tb_dp_dprx_start always takes a tunnel reference which is only dropped
> by dprx_work eventually. Tunnels that have no callback don't ever queue
> that work and tb_dp_dprx_stop then has nothing to cancel. It however only
> releases the reference if cancel_delayed_work returned true and the
> reference is leaked then.

Okay but we always actually pass that callback there so I guess you are
hitting this because you have modified the caller in tb.c not to pass the
callback, right? If that's the case then I suggest mention how you actually
reproduced this whole issue.

I'm thinking we should make the callback mandatory instead as we always
need it for DP tunnels anyway. It should work the same also in Apple
silicon (one you have the DP tunneling in place).

> Fix this by only taking the reference when dprx_work is actually queued.
> 
> Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sven Peter <sven@kernel.org>
> ---
> I didn't actually hit this on hardware but found it while fixing a domain
> leak in the same area and that fix depends on this one.
> ---
>  drivers/thunderbolt/tunnel.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
> index b7f32305f14a..50580ebdac4b 100644
> --- a/drivers/thunderbolt/tunnel.c
> +++ b/drivers/thunderbolt/tunnel.c
> @@ -1113,15 +1113,14 @@ static void tb_dp_dprx_work(struct work_struct *work)
>  
>  static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
>  {
> -	/*
> -	 * Bump up the reference to keep the tunnel around. It will be
> -	 * dropped in tb_dp_dprx_stop() once the tunnel is deactivated.
> -	 */
> -	tb_tunnel_get(tunnel);
> -
> -	tunnel->dprx_started = true;
> -
>  	if (tunnel->callback) {
> +		/*
> +		 * Bump up the reference to keep the tunnel around until the
> +		 * work has run or has been canceled.
> +		 */
> +		tb_tunnel_get(tunnel);
> +
> +		tunnel->dprx_started = true;
>  		tunnel->dprx_timeout = dprx_timeout_to_ktime(dprx_timeout);
>  		queue_delayed_work(tunnel->tb->wq, &tunnel->dprx_work, 0);
>  		return -EINPROGRESS;
> 
> -- 
> 2.55.0
> 

Re: [PATCH 1/5] thunderbolt: Fix tunnel reference leak when the DPRX work is not started

[Sven Peter]

Hi,

On 8/18/26 06:42, Mika Westerberg wrote:
> Hi,
>
> On Mon, Aug 17, 2026 at 09:53:58PM +0200, Sven Peter wrote:
>> tb_dp_dprx_start always takes a tunnel reference which is only dropped
>> by dprx_work eventually. Tunnels that have no callback don't ever queue
>> that work and tb_dp_dprx_stop then has nothing to cancel. It however only
>> releases the reference if cancel_delayed_work returned true and the
>> reference is leaked then.
> Okay but we always actually pass that callback there so I guess you are
> hitting this because you have modified the caller in tb.c not to pass the
> callback, right? If that's the case then I suggest mention how you actually
> reproduced this whole issue.
>
> I'm thinking we should make the callback mandatory instead as we always
> need it for DP tunnels anyway. It should work the same also in Apple
> silicon (one you have the DP tunneling in place).

As mentioned a few lines below,
> ---
> I didn't actually hit this on hardware but found it while fixing a domain
> leak in the same area and that fix depends on this one.
> ---

^-- there, I didn't actually hit this. It's just that there's also a 
tb_domain leak here (see patch 3) and when fixing that one the asymmetry 
here just jumps out. There's nothing special my code does to tb.c , the 
only reason DP tunnels don't work yet is because they need two separate 
MMIO blocks (what macOS calls "DP IN PHY" and "display crossbar") and 
possibly also the display co-processor to be up. Once that's done they 
should come up normally.

Tunnels discovered in tb_tunnel_discover_dp setup a DP tunnel with 
callback = NULL but also never start the dptx_work there and I'm not 
familiar enough with the code to know if it's possible to ever have 
those end up in the "normal" paths which queue the dprx_work then.
I'm happy to also just make the callback mandatory though and just bail 
if it's not set.


Sven

Re: [PATCH 1/5] thunderbolt: Fix tunnel reference leak when the DPRX work is not started

[Mika Westerberg]

Hi,

On Tue, Aug 18, 2026 at 07:44:35AM +0200, Sven Peter wrote:
> 
> Hi,
> 
> On 8/18/26 06:42, Mika Westerberg wrote:
> > Hi,
> > 
> > On Mon, Aug 17, 2026 at 09:53:58PM +0200, Sven Peter wrote:
> > > tb_dp_dprx_start always takes a tunnel reference which is only dropped
> > > by dprx_work eventually. Tunnels that have no callback don't ever queue
> > > that work and tb_dp_dprx_stop then has nothing to cancel. It however only
> > > releases the reference if cancel_delayed_work returned true and the
> > > reference is leaked then.
> > Okay but we always actually pass that callback there so I guess you are
> > hitting this because you have modified the caller in tb.c not to pass the
> > callback, right? If that's the case then I suggest mention how you actually
> > reproduced this whole issue.
> > 
> > I'm thinking we should make the callback mandatory instead as we always
> > need it for DP tunnels anyway. It should work the same also in Apple
> > silicon (one you have the DP tunneling in place).
> 
> As mentioned a few lines below,
> > ---
> > I didn't actually hit this on hardware but found it while fixing a domain
> > leak in the same area and that fix depends on this one.
> > ---

Hehe, sorry missed that one.

> ^-- there, I didn't actually hit this. It's just that there's also a
> tb_domain leak here (see patch 3) and when fixing that one the asymmetry
> here just jumps out. There's nothing special my code does to tb.c , the only
> reason DP tunnels don't work yet is because they need two separate MMIO
> blocks (what macOS calls "DP IN PHY" and "display crossbar") and possibly
> also the display co-processor to be up. Once that's done they should come up
> normally.
>
> Tunnels discovered in tb_tunnel_discover_dp setup a DP tunnel with callback
> = NULL but also never start the dptx_work there and I'm not familiar enough
> with the code to know if it's possible to ever have those end up in the
> "normal" paths which queue the dprx_work then.
> I'm happy to also just make the callback mandatory though and just bail if
> it's not set.

For the discovery (happens when the boot firmware/kexec sets up the
tunnels) DPRX negotiation is already done so we never need to do that for
those. I think it simplifies this if we just make it mandatory.

BTW, is this bringup stuff available somewhere already? I have M1 Mac here
so perhaps I can at least help testing things (assuming setting these up is
not super complex).

Re: [PATCH 2/5] thunderbolt: Hold a switch reference for each path hop

[Mika Westerberg]

Hi

In $subject I prefer "router" over "switch" as that's what USB4 calls it.

On Mon, Aug 17, 2026 at 09:53:59PM +0200, Sven Peter wrote:
> tb_stop drops the reference to all DP tunnels but does not deactivate
> them, thus nothing cancels a dprx_work still in flight (which holds its
> own tunnel reference) and the tunnel can outlive tb_switch_remove. The
> HopID releases in tb_path_free then operate on freed IDAs and trigger
> warnings like
> 
>   ida_free called for id=8 which is not allocated.
> 
> Take or release a reference for both ports of each hop whenever the
> HopIDs are allocated or released to ensure they have the same lifetime.

You reproduce this by unloading the driver while DPRX work is still
running, correct? Can you mention this in the commit log, I mean how this
can be triggered.

> The KUnit tests allocate their switches without ever registering them so
> initialize the embedded struct device there as well to make these
> references work.
> 
> Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sven Peter <sven@kernel.org>
> ---
>  drivers/thunderbolt/path.c | 21 +++++++++++++++++++++
>  drivers/thunderbolt/test.c | 12 ++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c
> index b2c322e76b8a..02c5e7a2101e 100644
> --- a/drivers/thunderbolt/path.c
> +++ b/drivers/thunderbolt/path.c
> @@ -196,6 +196,12 @@ struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
>  		path->hops[i].out_port = out_port;
>  		path->hops[i].next_hop_index = next_hop;
>  
> +		/* Keep the ports alive, see tb_path_free() */
> +		if (alloc_hopid) {
> +			tb_switch_get(path->hops[i].in_port->sw);
> +			tb_switch_get(path->hops[i].out_port->sw);
> +		}
> +
>  		tb_dump_hop(&path->hops[i], &hop);
>  
>  		h = next_hop;
> @@ -323,6 +329,10 @@ struct tb_path *tb_path_alloc(struct tb *tb, struct tb_port *src, int src_hopid,
>  		path->hops[i].out_port = out_port;
>  		path->hops[i].next_hop_index = out_hopid;
>  
> +		/* Keep the ports alive, see tb_path_free() */
> +		tb_switch_get(path->hops[i].in_port->sw);
> +		tb_switch_get(path->hops[i].out_port->sw);
> +
>  		in_hopid = out_hopid;
>  	}
>  
> @@ -356,6 +366,17 @@ void tb_path_free(struct tb_path *path)
>  			if (hop->out_port)
>  				tb_port_release_out_hopid(hop->out_port,
>  							  hop->next_hop_index);
> +			/*
> +			 * Only drop the switch references after both HopIDs
> +			 * have been released: the path may be freed after the
> +			 * switch was already removed (e.g. asynchronous DP
> +			 * tunnel teardown) and these references are what
> +			 * keeps the ports and their HopID IDAs alive.
> +			 */
> +			if (hop->in_port)
> +				tb_switch_put(hop->in_port->sw);
> +			if (hop->out_port)
> +				tb_switch_put(hop->out_port->sw);
>  		}
>  	}
>  
> diff --git a/drivers/thunderbolt/test.c b/drivers/thunderbolt/test.c
> index 05652ee82fbf..034c56845380 100644
> --- a/drivers/thunderbolt/test.c
> +++ b/drivers/thunderbolt/test.c
> @@ -33,6 +33,11 @@ static void kunit_ida_init(struct kunit *test, struct ida *ida)
>  	kunit_alloc_resource(test, __ida_init, __ida_destroy, GFP_KERNEL, ida);
>  }
>  
> +static void tb_test_switch_release(struct device *dev)
> +{
> +	/* The memory is owned by KUnit, nothing to do here */
> +}
> +
>  static struct tb_switch *alloc_switch(struct kunit *test, u64 route,
>  				      u8 upstream_port, u8 max_port_number)
>  {
> @@ -44,6 +49,13 @@ static struct tb_switch *alloc_switch(struct kunit *test, u64 route,
>  	if (!sw)
>  		return NULL;
>  
> +	/*
> +	 * The paths take a reference to their switches and those devices
> +	 * have to be initialized for that to work.
> +	 */
> +	sw->dev.release = tb_test_switch_release;
> +	device_initialize(&sw->dev);
> +
>  	sw->config.upstream_port_number = upstream_port;
>  	sw->config.depth = tb_route_length(route);
>  	sw->config.route_hi = upper_32_bits(route);
> 
> -- 
> 2.55.0
> 

Re: [PATCH 4/5] thunderbolt: Don't access a DP tunnel after its DPRX read was canceled

[Mika Westerberg]

Hi,

On Mon, Aug 17, 2026 at 09:54:01PM +0200, Sven Peter wrote:
> tb_dp_dprx_work checks dprx_canceled before it takes tb->lock so it
> misses a tb_dp_dprx_stop that could not cancel the already running
> work. It then polls the DPRX capabilities and runs the callback for a
> tunnel that is being torn down and touches routers that may already be
> gone after an unplug.

Also here it would be good to describe how you reproduce this.

> 
> Check the flag with tb->lock held instead and check it again in
> tb_dp_tunnel_active because the callback runs after the lock has been
> dropped again.
> 
> Also clear the flag in tb_dp_dprx_start so that it only ever describes
> the work that is currently in flight.
> 
> Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sven Peter <sven@kernel.org>
> ---
>  drivers/thunderbolt/tb.c     | 12 ++++++++++++
>  drivers/thunderbolt/tunnel.c | 11 +++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index fb9da53fe391..e368a6b53f64 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -1910,6 +1910,18 @@ static void tb_dp_tunnel_active(struct tb_tunnel *tunnel, void *data)
>  	struct tb *tb = data;
>  
>  	mutex_lock(&tb->lock);
> +
> +	/*
> +	 * If the DPRX read was canceled the tunnel is already being torn
> +	 * down by whoever canceled it. Do not touch the adapters here
> +	 * because the routers may be gone by now.
> +	 */
> +	if (tunnel->dprx_canceled) {
> +		tb_tunnel_dbg(tunnel, "DPRX read canceled, not activating\n");
> +		mutex_unlock(&tb->lock);
> +		return;
> +	}
> +
>  	if (tb_tunnel_is_active(tunnel)) {
>  		int consumed_up, consumed_down, ret;
>  
> diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
> index 82d9c0b556dd..52fa90786ff8 100644
> --- a/drivers/thunderbolt/tunnel.c
> +++ b/drivers/thunderbolt/tunnel.c
> @@ -1090,8 +1090,14 @@ static void tb_dp_dprx_work(struct work_struct *work)
>  	struct tb_tunnel *tunnel = container_of(work, typeof(*tunnel), dprx_work.work);
>  	struct tb *tb = tunnel->tb;
>  
> +	/*
> +	 * The DPRX read can be canceled while this work is waiting for
> +	 * tb->lock. Check the flag only once it is held: while the lock is
> +	 * held the tunnel cannot be torn down under us and the adapters are
> +	 * safe to access.
> +	 */
> +	mutex_lock(&tb->lock);
>  	if (!tunnel->dprx_canceled) {
> -		mutex_lock(&tb->lock);
>  		if (tb_dp_is_usb4(tunnel->src_port->sw) &&
>  		    tb_dp_wait_dprx(tunnel, TB_DPRX_WAIT_TIMEOUT)) {
>  			if (ktime_before(ktime_get(), tunnel->dprx_timeout)) {
> @@ -1103,8 +1109,8 @@ static void tb_dp_dprx_work(struct work_struct *work)
>  		} else {
>  			tb_tunnel_set_active(tunnel, true);
>  		}
> -		mutex_unlock(&tb->lock);
>  	}
> +	mutex_unlock(&tb->lock);
>  
>  	if (tunnel->callback)
>  		tunnel->callback(tunnel, tunnel->callback_data);
> @@ -1123,6 +1129,7 @@ static int tb_dp_dprx_start(struct tb_tunnel *tunnel)
>  		tb_domain_get(tunnel->tb);
>  
>  		tunnel->dprx_started = true;
> +		tunnel->dprx_canceled = false;
>  		tunnel->dprx_timeout = dprx_timeout_to_ktime(dprx_timeout);
>  		queue_delayed_work(tunnel->tb->wq, &tunnel->dprx_work, 0);
>  		return -EINPROGRESS;
> 
> -- 
> 2.55.0
> 

Re: [PATCH 1/5] thunderbolt: Fix tunnel reference leak when the DPRX work is not started

[Sven Peter]

Hi,

On 8/18/26 08:00, Mika Westerberg wrote:
> Hi,
>
> On Tue, Aug 18, 2026 at 07:44:35AM +0200, Sven Peter wrote:
>> Hi,
>>
>> On 8/18/26 06:42, Mika Westerberg wrote:
>>> Hi,
>>>
>>> On Mon, Aug 17, 2026 at 09:53:58PM +0200, Sven Peter wrote:
>>>> tb_dp_dprx_start always takes a tunnel reference which is only dropped
>>>> by dprx_work eventually. Tunnels that have no callback don't ever queue
>>>> that work and tb_dp_dprx_stop then has nothing to cancel. It however only
>>>> releases the reference if cancel_delayed_work returned true and the
>>>> reference is leaked then.
>>> Okay but we always actually pass that callback there so I guess you are
>>> hitting this because you have modified the caller in tb.c not to pass the
>>> callback, right? If that's the case then I suggest mention how you actually
>>> reproduced this whole issue.
>>>
>>> I'm thinking we should make the callback mandatory instead as we always
>>> need it for DP tunnels anyway. It should work the same also in Apple
>>> silicon (one you have the DP tunneling in place).
>> As mentioned a few lines below,
>>> ---
>>> I didn't actually hit this on hardware but found it while fixing a domain
>>> leak in the same area and that fix depends on this one.
>>> ---
> Hehe, sorry missed that one.

No worries, maybe I should've put it into the commit message and/or 
cover letter as well!
>
>> ^-- there, I didn't actually hit this. It's just that there's also a
>> tb_domain leak here (see patch 3) and when fixing that one the asymmetry
>> here just jumps out. There's nothing special my code does to tb.c , the only
>> reason DP tunnels don't work yet is because they need two separate MMIO
>> blocks (what macOS calls "DP IN PHY" and "display crossbar") and possibly
>> also the display co-processor to be up. Once that's done they should come up
>> normally.
>>
>> Tunnels discovered in tb_tunnel_discover_dp setup a DP tunnel with callback
>> = NULL but also never start the dptx_work there and I'm not familiar enough
>> with the code to know if it's possible to ever have those end up in the
>> "normal" paths which queue the dprx_work then.
>> I'm happy to also just make the callback mandatory though and just bail if
>> it's not set.
> For the discovery (happens when the boot firmware/kexec sets up the
> tunnels) DPRX negotiation is already done so we never need to do that for
> those. I think it simplifies this if we just make it mandatory.

Okay, great, I'll look into making it mandatory for v2 then.

>
> BTW, is this bringup stuff available somewhere already? I have M1 Mac here
> so perhaps I can at least help testing things (assuming setting these up is
> not super complex).

Sure, the current WIP code is in 
https://github.com/AsahiLinux/linux/commits/tbt-reset-wip/.
It's not quite in a good shape yet and I have a few improvements in my 
local tree that I can push this evening after $work. Right now only 
XDomain works since pcie, dp and usb3 tunnels need more bringup work and 
not all cables work because I'm not forwarding the information like 
Apple's NHI expects them just yet.


Best,

Sven

Re: [PATCH 5/5] thunderbolt: Cancel the DPRX read when the domain is stopped

[Mika Westerberg]

Hi,

On Mon, Aug 17, 2026 at 09:54:02PM +0200, Sven Peter wrote:
> tb_stop only tears down DMA tunnels so a DP tunnel that is still
> waiting for dprx_work to complete keeps that work queued while the
> routers are removed and the control channel is stopped. The work only
> stops once the DPRX timeout has passed and because it requeues itself
> until then the flush_workqueue in tb_domain_remove won't wait for its
> final run. The callback then runs against a domain that is already torn
> down. A reference to that domain is kept so the completion waiting for
> that domain to disappear in unbind will block until the timeout is
> eventually reached.
> 
> Just cancel the work in tb_stop. This doesn't affect DP tunnels that are
> already alive and keeps those displays working.
> 
> Fixes: d6d458d42e1e ("thunderbolt: Handle DisplayPort tunnel activation asynchronously")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sven Peter <sven@kernel.org>
> ---
> I also didn't run into this but noticed it when fixing the hop alloc thing
> and think it makes sense to fix it anyway.
> ---
>  drivers/thunderbolt/tb.c     | 5 ++++-
>  drivers/thunderbolt/tunnel.c | 9 +++++++++
>  drivers/thunderbolt/tunnel.h | 1 +
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index e368a6b53f64..f7e68372da09 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -2958,10 +2958,13 @@ static void tb_stop(struct tb *tb)
>  		/*
>  		 * DMA tunnels require the driver to be functional so we
>  		 * tear them down. Other protocol tunnels can be left
> -		 * intact.
> +		 * intact but a DPRX capabilities read that is still in
> +		 * flight has to be canceled before the routers go away.
>  		 */
>  		if (tb_tunnel_is_dma(tunnel))
>  			tb_tunnel_deactivate(tunnel);
> +		else if (tb_tunnel_is_dp(tunnel))
> +			tb_tunnel_cancel_dprx(tunnel);

I prefer not to expose "non-generic" functions for the CM if possible. I
wonder if this would work:

		else if (tb_tunnel_is_dp(tunnel) && !tb_tunnel_is_active(tunnel))
			tb_tunnel_deactivate(tunnel);


>  		tb_tunnel_put(tunnel);
>  	}
>  	tb_switch_remove(tb->root_switch);
> diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
> index 52fa90786ff8..5b1ae5a0c12b 100644
> --- a/drivers/thunderbolt/tunnel.c
> +++ b/drivers/thunderbolt/tunnel.c
> @@ -2487,6 +2487,15 @@ void tb_tunnel_deactivate(struct tb_tunnel *tunnel)
>  	tb_tunnel_set_active(tunnel, false);
>  }
>  
> +/**
> + * tb_tunnel_cancel_dprx() - Cancel the DPRX capabilities read work
> + * @tunnel: tunnel to cancel the DPRX capabilities read work for
> + */
> +void tb_tunnel_cancel_dprx(struct tb_tunnel *tunnel)
> +{
> +	tb_dp_dprx_stop(tunnel);
> +}
> +
>  /**
>   * tb_tunnel_port_on_path() - Does the tunnel go through port
>   * @tunnel: Tunnel to check
> diff --git a/drivers/thunderbolt/tunnel.h b/drivers/thunderbolt/tunnel.h
> index 4878763a82b3..9de5fac04269 100644
> --- a/drivers/thunderbolt/tunnel.h
> +++ b/drivers/thunderbolt/tunnel.h
> @@ -138,6 +138,7 @@ struct tb_tunnel *tb_tunnel_alloc_usb3(struct tb *tb, struct tb_port *up,
>  void tb_tunnel_put(struct tb_tunnel *tunnel);
>  int tb_tunnel_activate(struct tb_tunnel *tunnel);
>  void tb_tunnel_deactivate(struct tb_tunnel *tunnel);
> +void tb_tunnel_cancel_dprx(struct tb_tunnel *tunnel);
>  
>  /**
>   * tb_tunnel_is_active() - Is tunnel fully activated
> 
> -- 
> 2.55.0
> 

Re: [PATCH 1/5] thunderbolt: Fix tunnel reference leak when the DPRX work is not started

[Mika Westerberg]

Hi,

On Tue, Aug 18, 2026 at 08:11:09AM +0200, Sven Peter wrote:
> Hi,
> 
> On 8/18/26 08:00, Mika Westerberg wrote:
> > Hi,
> > 
> > On Tue, Aug 18, 2026 at 07:44:35AM +0200, Sven Peter wrote:
> > > Hi,
> > > 
> > > On 8/18/26 06:42, Mika Westerberg wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Aug 17, 2026 at 09:53:58PM +0200, Sven Peter wrote:
> > > > > tb_dp_dprx_start always takes a tunnel reference which is only dropped
> > > > > by dprx_work eventually. Tunnels that have no callback don't ever queue
> > > > > that work and tb_dp_dprx_stop then has nothing to cancel. It however only
> > > > > releases the reference if cancel_delayed_work returned true and the
> > > > > reference is leaked then.
> > > > Okay but we always actually pass that callback there so I guess you are
> > > > hitting this because you have modified the caller in tb.c not to pass the
> > > > callback, right? If that's the case then I suggest mention how you actually
> > > > reproduced this whole issue.
> > > > 
> > > > I'm thinking we should make the callback mandatory instead as we always
> > > > need it for DP tunnels anyway. It should work the same also in Apple
> > > > silicon (one you have the DP tunneling in place).
> > > As mentioned a few lines below,
> > > > ---
> > > > I didn't actually hit this on hardware but found it while fixing a domain
> > > > leak in the same area and that fix depends on this one.
> > > > ---
> > Hehe, sorry missed that one.
> 
> No worries, maybe I should've put it into the commit message and/or cover
> letter as well!
> > 
> > > ^-- there, I didn't actually hit this. It's just that there's also a
> > > tb_domain leak here (see patch 3) and when fixing that one the asymmetry
> > > here just jumps out. There's nothing special my code does to tb.c , the only
> > > reason DP tunnels don't work yet is because they need two separate MMIO
> > > blocks (what macOS calls "DP IN PHY" and "display crossbar") and possibly
> > > also the display co-processor to be up. Once that's done they should come up
> > > normally.
> > > 
> > > Tunnels discovered in tb_tunnel_discover_dp setup a DP tunnel with callback
> > > = NULL but also never start the dptx_work there and I'm not familiar enough
> > > with the code to know if it's possible to ever have those end up in the
> > > "normal" paths which queue the dprx_work then.
> > > I'm happy to also just make the callback mandatory though and just bail if
> > > it's not set.
> > For the discovery (happens when the boot firmware/kexec sets up the
> > tunnels) DPRX negotiation is already done so we never need to do that for
> > those. I think it simplifies this if we just make it mandatory.
> 
> Okay, great, I'll look into making it mandatory for v2 then.
> 
> > 
> > BTW, is this bringup stuff available somewhere already? I have M1 Mac here
> > so perhaps I can at least help testing things (assuming setting these up is
> > not super complex).
> 
> Sure, the current WIP code is in
> https://github.com/AsahiLinux/linux/commits/tbt-reset-wip/.

Cool! Thanks for sharing.

> It's not quite in a good shape yet and I have a few improvements in my local
> tree that I can push this evening after $work. Right now only XDomain works
> since pcie, dp and usb3 tunnels need more bringup work and not all cables
> work because I'm not forwarding the information like Apple's NHI expects
> them just yet.

Understood.