* [PATCH net v2 1/2] net: thunderbolt: Release the Rx HopID that was handed out on mismatch
2026-08-10 9:39 [PATCH net v2 0/2] net: thunderbolt: two fixes for the failed bring-up path Fan Ye via B4 Relay
@ 2026-08-10 9:39 ` Fan Ye via B4 Relay
2026-08-10 18:56 ` Andy Shevchenko
2026-08-10 9:39 ` [PATCH net v2 2/2] net: thunderbolt: Mark the connection down when bringing it up fails Fan Ye via B4 Relay
1 sibling, 1 reply; 5+ messages in thread
From: Fan Ye via B4 Relay @ 2026-08-10 9:39 UTC (permalink / raw)
To: Jakub Kicinski, Eric Dumazet, David S. Miller, Paolo Abeni,
Andrew Lunn, Mika Westerberg, Yehezkel Bernat
Cc: netdev, linux-kernel, Andy Shevchenko, stable, Fan Ye,
Mika Westerberg
From: Fan Ye <fy15309206903@gmail.com>
tbnet_connected_work() asks for a specific input HopID and treats getting
a different one as a failure:
ret = tb_xdomain_alloc_in_hopid(net->xd, net->remote_transmit_path);
if (ret != net->remote_transmit_path) {
netdev_err(net->dev, "failed to allocate Rx HopID\n");
return;
}
That call ends in ida_alloc_range(&xd->in_hopids, hopid,
xd->local_max_hopid, GFP_KERNEL), which allocates the lowest free id at
or above the one asked for. When the
wanted HopID is already taken it does not fail - it succeeds with the next
one - so this path returns with an id allocated and no reference to it
left anywhere. It stays allocated for the rest of the XDomain connection.
Forcing the branch by occupying the wanted HopID first shows the returned
id is a live allocation, not an error code:
LEAKPROBE squat=8 requested=8 local_max_hopid=27
LEAKPROBE real alloc ret=9
thunderbolt-net 0-1.0 thunderbolt0: failed to allocate Rx HopID
Release the id when it is not the one we wanted, matching what the error
unwind at the end of the function already does for the expected id.
Fixes: 180b0689425c ("thunderbolt: Allow multiple DMA tunnels over a single XDomain connection")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Fan Ye <fy15309206903@gmail.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
These four came out of one investigation on a pair of ASMedia ASM4242
hosts wired to each other. Apply them in this order: the second one
touches lines the first one adds, so it needs that one underneath to
apply at all, and the last two want the first two under them for the
reason below.
1 net: thunderbolt: Release the Rx HopID that was handed out on mismatch
2 net: thunderbolt: Mark the connection down when bringing it up fails
3 thunderbolt: Report DMA path teardown failures to the caller
4 thunderbolt: Stop waiting on a path pending bit that never clears
This one is number 1 on that list.
1 and 2 fix two separate things that happen to be reached through the
same branch. Neither depends on the other for correctness - each leaves
the other's defect in place - but 2 edits the lines 1 adds, so it will
not apply on its own.
3 and 4 do want 1 and 2 underneath: the warning splat that 2 removes
fires throughout any prolonged run of link cycling, which is what 3 and 4
have to be measured across. 3 makes teardown failures visible to the
caller at all; 4 stops the teardown paying for one that cannot succeed.
Note what that pair does on this particular router - 4 leaves the first
failure to be reported and silences the rest, so 3's new signal fires
once per adapter here rather than on every teardown. 4 is the one I am
least sure of, for the reasons in its own notes.
Found on an ASMedia ASM4242 host-to-host link, where the branch is
reached on its own when the peer drops out while a connection is being
brought up. Cycling the interface down and up 200 times over 80 minutes
hits it 23 times across the two hosts, no fault injection involved.
I am not claiming a user-visible symptom for this one. Every one of those
23 occurrences recovered on its own, 19 to 21 seconds later, because the
XDomain connection ends and its ida is recreated along with it, which
also disposes of the leaked id. I could not reach the branch twice within
one connection, so I cannot show the HopID range being exhausted either.
What the patch fixes is the leak itself. The patch that follows has the
measurable effect, and does not depend on this one - they are separate
defects reached through the same branch, and with only that one applied
the id this path obtained is still never released.
I could use help with one thing. The only way I found to reach this
branch is to wait for the peer to drop out at the wrong moment, and the
XDomain connection ends with it, so the leaked id goes away with the ida.
If anyone has a setup where the branch can be hit twice within one
connection - more than one service on the same XDomain, say - that would
settle whether the range can actually be run down, which I could not
show either way.
v2:
- Add the Assisted-by tag Mika asked for. No code change from v1.
- Pick up Mika's Acked-by.
v1: https://lore.kernel.org/netdev/20260809-b4-tbnet-hopid-v1-0-9a8c7f5f0ba9@gmail.com/
---
drivers/net/thunderbolt/main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
index 98893732bc6e..e5199a87ea7a 100644
--- a/drivers/net/thunderbolt/main.c
+++ b/drivers/net/thunderbolt/main.c
@@ -647,6 +647,8 @@ static void tbnet_connected_work(struct work_struct *work)
ret = tb_xdomain_alloc_in_hopid(net->xd, net->remote_transmit_path);
if (ret != net->remote_transmit_path) {
netdev_err(net->dev, "failed to allocate Rx HopID\n");
+ if (ret >= 0)
+ tb_xdomain_release_in_hopid(net->xd, ret);
return;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net v2 2/2] net: thunderbolt: Mark the connection down when bringing it up fails
2026-08-10 9:39 [PATCH net v2 0/2] net: thunderbolt: two fixes for the failed bring-up path Fan Ye via B4 Relay
2026-08-10 9:39 ` [PATCH net v2 1/2] net: thunderbolt: Release the Rx HopID that was handed out on mismatch Fan Ye via B4 Relay
@ 2026-08-10 9:39 ` Fan Ye via B4 Relay
2026-08-10 18:57 ` Andy Shevchenko
1 sibling, 1 reply; 5+ messages in thread
From: Fan Ye via B4 Relay @ 2026-08-10 9:39 UTC (permalink / raw)
To: Jakub Kicinski, Eric Dumazet, David S. Miller, Paolo Abeni,
Andrew Lunn, Mika Westerberg, Yehezkel Bernat
Cc: netdev, linux-kernel, Andy Shevchenko, stable, Fan Ye,
Mika Westerberg
From: Fan Ye <fy15309206903@gmail.com>
Every failure path in tbnet_connected_work() undoes its own work and
returns, but none of them clears login_sent/login_received. The
connection therefore still looks established, and the next
tbnet_tear_down() takes its main branch and runs the whole teardown a
second time over work that was already undone:
thunderbolt-net 0-1.0 thunderbolt0: failed to allocate Rx HopID
thunderbolt 0000:78:00.0: RX ring 1 already stopped
WARNING: CPU: 0 PID: 235 at drivers/thunderbolt/nhi.c:773 tb_ring_stop
tbnet_tear_down -> tbnet_stop -> __dev_close_many
thunderbolt 0000:78:00.0: TX ring 1 already stopped
WARNING: CPU: 0 PID: 235 at drivers/thunderbolt/nhi.c:773 tb_ring_stop
(line 773 is the dev_WARN in tb_ring_stop() as of v6.17, which is what
this was captured on; it is line 760 in current mainline)
It stops rings that were never started, which is what the two warnings
above are, and on a kernel booted with panic_on_warn those are fatal.
It also releases net->remote_transmit_path. On the HopID mismatch path
that one was never successfully allocated by this connection - the
allocator handed out a different id precisely because the wanted one was
already taken by somebody else - so this hands back an id the connection
does not own, and it does so silently. The id is then free to be handed
out again while its owner is still using it.
Mark the connection as no longer established on those paths. Only
login_sent is cleared, which is enough for tbnet_tear_down() to leave the
already unwound state alone; login_received records that the peer has
logged in with us and carries the transmit path it gave us, and nothing
on this side can make the peer send that again.
Skipping that block skips two things that are not just a repeat of the
unwind. One is the logout request it would have sent to the peer. The
other is net->remote_transmit_path = 0 at the end of it; that field is
only read under the same login_sent && login_received guard and the
peer's next login request overwrites it, so leaving it stale is
harmless, but it is a clear that no longer happens. The rest of the
block is either already undone by the unwind that just ran or was never
done in the first place - the rings are not started and no buffers are
allocated when the HopID mismatch is hit, and the paths are not enabled
on any path that reaches err_stop_rings. The parts outside the block -
carrier off, queue stopped, login stopped, and the state reset at the
end - keep running as before.
Clearing login_sent also changes what the peer's next login request
does: tbnet_handle_packet() re-queues our login work when it sees
!login_sent, where before it would only have queued connected_work. That
is the direction I want - it gives the connection a fresh login instead
of retrying the bring-up on stale state - but it is a behaviour change
beyond keeping tbnet_tear_down() out of the way.
Measured on a link between two ASMedia ASM4242 hosts by cycling the
interface down and up 200 times from one of them over 80 minutes, and
counting what the kernel logs on both.
The mismatch is reached on its own during that, no fault injection, and
both runs were started from a cold boot with no module reloads in
between. Only the thunderbolt-net module differs between the two:
without with
failed to allocate Rx HopID 11 / 12 9 / 13
ring already stopped + WARNING 22 / 24 0 / 0
(host A / host B)
The race still happens as often as before - it is not what this patch
addresses - but it no longer leaves a warning splat behind, and no longer
releases a HopID that belongs to someone else.
Fixes: e69b6c02b4c3 ("net: Add support for networking over Thunderbolt cable")
Cc: <stable@vger.kernel.org> # 5.13+
Assisted-by: Claude:claude-opus-5
Signed-off-by: Fan Ye <fy15309206903@gmail.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
These four came out of one investigation on a pair of ASMedia ASM4242
hosts wired to each other. Apply them in this order: the second one
touches lines the first one adds, so it needs that one underneath to
apply at all, and the last two want the first two under them for the
reason below.
1 net: thunderbolt: Release the Rx HopID that was handed out on mismatch
2 net: thunderbolt: Mark the connection down when bringing it up fails
3 thunderbolt: Report DMA path teardown failures to the caller
4 thunderbolt: Stop waiting on a path pending bit that never clears
This one is number 2 on that list.
1 and 2 fix two separate things that happen to be reached through the
same branch. Neither depends on the other for correctness - each leaves
the other's defect in place - but 2 edits the lines 1 adds, so it will
not apply on its own.
3 and 4 do want 1 and 2 underneath: the warning splat that 2 removes
fires throughout any prolonged run of link cycling, which is what 3 and 4
have to be measured across. 3 makes teardown failures visible to the
caller at all; 4 stops the teardown paying for one that cannot succeed.
Note what that pair does on this particular router - 4 leaves the first
failure to be reported and silences the rest, so 3's new signal fires
once per adapter here rather than on every teardown. 4 is the one I am
least sure of, for the reasons in its own notes.
Sits on top of "net: thunderbolt: Release the Rx HopID that was handed
out on mismatch". The two fix different things - that one releases the id
the allocator actually handed out, this one stops the teardown that
follows from running a second time - but this one edits the lines that
one adds, so it will not apply without it.
An earlier version of this also rearmed the login from the failure paths,
on the assumption that the connection would otherwise stay down. It does
not: every one of those failures was followed by the XDomain connection
being torn down and rebuilt, and the interface came back on its own. Of
the 60 occurrences across three runs, 59 had a tunnel set up again
afterwards that I could time - 42 of them 19 seconds later, 14 at 20, one
at 21 and two at 43 - with and without the patch alike. That is the
re-enumeration, not a login retry; a retry would have shown up as a
multiple of TBNET_LOGIN_DELAY. The rearm bought nothing and needed a
retry bound of its own, so it is gone.
What I am not claiming: that this makes the link survive longer. I have
run the 200 cycles four times, on four different combinations. Two of
them stopped coming up after 65 and 69 minutes and two ran to the end.
Both of the ones that finished had this patch, and neither of the ones
that died had this version of it - one had no part of it at all, the
other an earlier version. On the face of it that reads the way I would
like it to, which is exactly why I am not reading it that way: four runs,
four different combinations, one run each. What I can say is that the
race this patch is about is not what ends a run - it fired 22 times in
the run in the table above and the link came back from all 22.
One question I cannot answer from here. Skipping the block also skips the
logout request that would have gone to the peer. Everything else in it is
either already undone or was never done, but the logout is a real thing
the peer no longer hears about. I could not measure a difference - the
XDomain connection is torn down and rebuilt right afterwards in every
case I logged - but whether that is good enough, or whether the failure
paths should send one explicitly before returning, is a call I would
rather leave to you.
v2:
- Add the Assisted-by tag Mika asked for. No code change from v1.
- Pick up Mika's Acked-by.
v1: https://lore.kernel.org/netdev/20260809-b4-tbnet-hopid-v1-0-9a8c7f5f0ba9@gmail.com/
---
drivers/net/thunderbolt/main.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
index e5199a87ea7a..b6aec215a8e9 100644
--- a/drivers/net/thunderbolt/main.c
+++ b/drivers/net/thunderbolt/main.c
@@ -626,6 +626,20 @@ static int tbnet_alloc_tx_buffers(struct tbnet *net)
return 0;
}
+static void tbnet_connect_failed(struct tbnet *net)
+{
+ /*
+ * Mark the connection as no longer established so that
+ * tbnet_tear_down() does not run over the unwind we just did.
+ * Only login_sent is cleared: login_received records that the peer
+ * has logged in with us and carries the transmit path it gave us,
+ * and nothing on this side can make the peer send that again.
+ */
+ mutex_lock(&net->connection_lock);
+ net->login_sent = false;
+ mutex_unlock(&net->connection_lock);
+}
+
static void tbnet_connected_work(struct work_struct *work)
{
struct tbnet *net = container_of(work, typeof(*net), connected_work);
@@ -649,6 +663,7 @@ static void tbnet_connected_work(struct work_struct *work)
netdev_err(net->dev, "failed to allocate Rx HopID\n");
if (ret >= 0)
tb_xdomain_release_in_hopid(net->xd, ret);
+ tbnet_connect_failed(net);
return;
}
@@ -693,6 +708,7 @@ static void tbnet_connected_work(struct work_struct *work)
tb_ring_stop(net->rx_ring.ring);
tb_ring_stop(net->tx_ring.ring);
tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
+ tbnet_connect_failed(net);
}
static void tbnet_login_work(struct work_struct *work)
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread