Re: [PATCH 2/2] net: thunderbolt: Fix lockdep issue during tunnel tear down

Jakub Kicinski
2026-04-29T01:12:28Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: thunderbolt: Fix lockdep issue during tunnel tear down

This commit attempts to fix a circular locking dependency warning in
tbnet_tear_down() by dropping the connection lock earlier and returning if the
connection isn't fully established.

> diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> index 631af237998d..d8fcf18fc55c 100644
> --- a/drivers/net/thunderbolt/main.c
> +++ b/drivers/net/thunderbolt/main.c
[ ... ]
> @@ -372,48 +372,47 @@ static void tbnet_free_buffers(struct tbnet_ring *ring)
>
> static void tbnet_tear_down(struct tbnet *net, bool send_logout)
> {
> + int ret, retries = TBNET_LOGOUT_RETRIES;
> +
> netif_carrier_off(net->dev);
> netif_stop_queue(net->dev);
>
> stop_login(net);
>
> mutex_lock(&net->connection_lock);
> + if (!net->login_sent || !net->login_received) {
> + mutex_unlock(&net->connection_lock);
> + return;
> + }

If the connection is in a partially established state (e.g. login_sent is
true, but login_received is false), this early return skips clearing the
connection states.

Could this stall the connection if a new TBIP_LOGIN arrives later?
tbnet_handle_packet() would see net->login_sent as true and might fail to
queue the login_work to reply.

> + net->login_sent = false;
> + net->login_received = false;
> + mutex_unlock(&net->connection_lock);

By releasing net->connection_lock here, the rest of the teardown process runs
without mutual exclusion. Since the network interface is still running, is it
possible for tbnet_handle_packet() to receive a new TBIP_LOGIN request right
after this lock is dropped?

If tbnet_handle_packet() acquires the lock, updates net->remote_transmit_path
and brings up a new connection concurrently:

CPU1
tbnet_tear_down()
mutex_unlock(&net->connection_lock);

CPU2
tbnet_handle_packet()
mutex_lock(&net->connection_lock);
net->remote_transmit_path = pkg->transmit_path;
mutex_unlock(&net->connection_lock);

CPU1
tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);

Would CPU1 then erroneously release the newly established connection's hopid
and free active buffers?

>
> - if (net->login_sent && net->login_received) {
> - int ret, retries = TBNET_LOGOUT_RETRIES;
[ ... ]
> - tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
> - net->remote_transmit_path = 0;
> - }
> + tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
> + net->remote_transmit_path = 0;
>
> - net->login_retries = 0;

With the unconditional clearing of net->login_retries removed, does the
login_retries counter keep accumulating if tbnet_tear_down() returns early
above?

If it exceeds TBNET_LOGIN_RETRIES, could it permanently prevent new
connections until the module is reloaded?

> - net->login_sent = false;
> - net->login_received = false;
>
> netdev_dbg(net->dev, "network traffic stopped\n");
> -
> - mutex_unlock(&net->connection_lock);
> }