Re: [PATCH net 1/2] net: thunderbolt: Release the Rx HopID that was handed out on mismatch

Hi,

On Sun, Aug 09, 2026 at 02:28:00AM +0000, Fan Ye via B4 Relay wrote:
> From: Fan Ye <[email protected]>
> 
> 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: [email protected]
> Signed-off-by: Fan Ye <[email protected]>

This is okay but should you also add assisted-by tag for the LLM you used
to generate the patch? I think that's still required. Ditto for the other
patches as well.

Acked-by: Mika Westerberg <[email protected]>

> ---
> 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.
> ---
>  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
>