[PATCH 1/2] net: thunderbolt: Call tbnet_stop() upon shutdown

[PATCH 1/2] net: thunderbolt: Call tbnet_stop() upon shutdown

[Mika Westerberg]

Stopping the rings is not enough for the core driver. It expected that
the service drivers release them as well. For this reason call
tbnet_stop() if the interface has been brought up upon shutdown.

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

diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
index 7aae5d915a1e..631af237998d 100644
--- a/drivers/net/thunderbolt/main.c
+++ b/drivers/net/thunderbolt/main.c
@@ -1417,7 +1417,10 @@ static void tbnet_remove(struct tb_service *svc)
 
 static void tbnet_shutdown(struct tb_service *svc)
 {
-	tbnet_tear_down(tb_service_get_drvdata(svc), true);
+	struct tbnet *net = tb_service_get_drvdata(svc);
+
+	if (netif_running(net->dev))
+		tbnet_stop(net->dev);
 }
 
 static int tbnet_suspend(struct device *dev)
-- 
2.50.1

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

[Mika Westerberg]

When unplugging cable with lockdep enabled it triggers following:

 ======================================================
 WARNING: possible circular locking dependency detected
 6.18.0-rc4+ #1636 Tainted: G S
 ------------------------------------------------------
 kworker/u56:2/416 is trying to acquire lock:
 ffff88818642c8a8 ((work_completion)(&ring->work)){+.+.}-{0:0}, at: __flush_work+0x562/0xde0

 but task is already holding lock:
 ffff888184b30fa8 (&net->connection_lock){+.+.}-{4:4}, at: tbnet_tear_down+0x110/0x6f0 [thunderbolt_net]

We actually don't need to net->hold the connection_lock any more than
necessary to figure out if the connection is up or not, and can return
early if it is not.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/net/thunderbolt/main.c | 59 +++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 30 deletions(-)

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;
+	}
+	net->login_sent = false;
+	net->login_received = false;
+	mutex_unlock(&net->connection_lock);
 
-	if (net->login_sent && net->login_received) {
-		int ret, retries = TBNET_LOGOUT_RETRIES;
+	while (send_logout && retries-- > 0) {
+		netdev_dbg(net->dev, "sending logout request %u\n",
+			   retries);
+		ret = tbnet_logout_request(net);
+		if (ret != -ETIMEDOUT)
+			break;
+	}
 
-		while (send_logout && retries-- > 0) {
-			netdev_dbg(net->dev, "sending logout request %u\n",
-				   retries);
-			ret = tbnet_logout_request(net);
-			if (ret != -ETIMEDOUT)
-				break;
-		}
+	tb_ring_stop(net->rx_ring.ring);
+	tb_ring_stop(net->tx_ring.ring);
+	tbnet_free_buffers(&net->rx_ring);
+	tbnet_free_buffers(&net->tx_ring);
 
-		tb_ring_stop(net->rx_ring.ring);
-		tb_ring_stop(net->tx_ring.ring);
-		tbnet_free_buffers(&net->rx_ring);
-		tbnet_free_buffers(&net->tx_ring);
-
-		ret = tb_xdomain_disable_paths(net->xd,
-					       net->local_transmit_path,
-					       net->tx_ring.ring->hop,
-					       net->remote_transmit_path,
-					       net->rx_ring.ring->hop);
-		if (ret)
-			netdev_warn(net->dev, "failed to disable DMA paths\n");
-
-		tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
-		net->remote_transmit_path = 0;
-	}
+	ret = tb_xdomain_disable_paths(net->xd,
+				       net->local_transmit_path,
+				       net->tx_ring.ring->hop,
+				       net->remote_transmit_path,
+				       net->rx_ring.ring->hop);
+	if (ret)
+		netdev_warn(net->dev, "failed to disable DMA paths\n");
 
-	net->login_retries = 0;
-	net->login_sent = false;
-	net->login_received = false;
+	tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
+	net->remote_transmit_path = 0;
 
 	netdev_dbg(net->dev, "network traffic stopped\n");
-
-	mutex_unlock(&net->connection_lock);
 }
 
 static int tbnet_handle_packet(const void *buf, size_t size, void *data)
-- 
2.50.1

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

[Jakub Kicinski]

On Mon, 27 Apr 2026 10:16:23 +0200 Mika Westerberg wrote:
> When unplugging cable with lockdep enabled it triggers following:

Sounds like a fix, should it have a Fixes tag?

I'll also send out the AI review but on a quick look not sure if any 
of it is worth addressing.

Re: [PATCH 1/2] net: thunderbolt: Call tbnet_stop() upon shutdown

[Jakub Kicinski]

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: Call tbnet_stop() upon shutdown
    
This commit updates the shutdown callback for the Thunderbolt network service
driver to call tbnet_stop() if the interface is running. This ensures the
service drivers release the rings upon shutdown.

> diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> index 7aae5d915a1e..631af237998d 100644
> --- a/drivers/net/thunderbolt/main.c
> +++ b/drivers/net/thunderbolt/main.c
> @@ -1417,7 +1417,10 @@ static void tbnet_remove(struct tb_service *svc)
>  
>  static void tbnet_shutdown(struct tb_service *svc)
>  {
> -	tbnet_tear_down(tb_service_get_drvdata(svc), true);
> +	struct tbnet *net = tb_service_get_drvdata(svc);
> +
> +	if (netif_running(net->dev))
> +		tbnet_stop(net->dev);

Does calling tbnet_stop() directly bypass the network stack teardown sequence?

tbnet_stop() appears to be the driver's ndo_stop callback. By bypassing
dev_close(), netif_tx_disable() is not called and the device's transmit queues
remain active.

tbnet_stop() destroys the DMA rings and explicitly sets net->tx_ring.ring
to NULL:

drivers/net/thunderbolt/main.c:tbnet_stop() {
    ...
    tb_ring_free(net->tx_ring.ring);
    net->tx_ring.ring = NULL;
    ...
}

If a concurrent transmission occurs, will tbnet_start_xmit() access the null
pointer and cause a crash?

Additionally, since this does not hold the rtnl_lock(), could this race
with concurrent administrative state changes (like 'ip link set dev down')
and lead to a double-execution of tbnet_stop()?

>  }
>  
>  static int tbnet_suspend(struct device *dev)
-- 
pw-bot: cr

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

[Jakub Kicinski]

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);
>  }

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

[Mika Westerberg]

On Tue, Apr 28, 2026 at 06:12:26PM -0700, Jakub Kicinski wrote:
> 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.

I don't think that matters because tbnet_tear_down() is actually tearing
down the connection so after this we do not expect the connection to be
established until start_login() is called again (this happens when the
interface is brought up again or on resume).

> > +	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?

Oh, this one definitely can happen.

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

Indeed, that's an accidental removal.

Okay let me try to figure out better fix for this lockdep issue.

Re: [PATCH 1/2] net: thunderbolt: Call tbnet_stop() upon shutdown

[Mika Westerberg]

On Tue, Apr 28, 2026 at 06:12:24PM -0700, Jakub Kicinski wrote:
> 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: Call tbnet_stop() upon shutdown
>     
> This commit updates the shutdown callback for the Thunderbolt network service
> driver to call tbnet_stop() if the interface is running. This ensures the
> service drivers release the rings upon shutdown.
> 
> > diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> > index 7aae5d915a1e..631af237998d 100644
> > --- a/drivers/net/thunderbolt/main.c
> > +++ b/drivers/net/thunderbolt/main.c
> > @@ -1417,7 +1417,10 @@ static void tbnet_remove(struct tb_service *svc)
> >  
> >  static void tbnet_shutdown(struct tb_service *svc)
> >  {
> > -	tbnet_tear_down(tb_service_get_drvdata(svc), true);
> > +	struct tbnet *net = tb_service_get_drvdata(svc);
> > +
> > +	if (netif_running(net->dev))
> > +		tbnet_stop(net->dev);
> 
> Does calling tbnet_stop() directly bypass the network stack teardown sequence?

This is called when the machine is rebooted or shut down to quiesce the
device. I don't think the network stack has anything similar?

> tbnet_stop() appears to be the driver's ndo_stop callback. By bypassing
> dev_close(), netif_tx_disable() is not called and the device's transmit queues
> remain active.

tbnet_stop() calls tbnet_tear_down() that calls netif_stop_queue() which
should stop the queue.

> tbnet_stop() destroys the DMA rings and explicitly sets net->tx_ring.ring
> to NULL:
> 
> drivers/net/thunderbolt/main.c:tbnet_stop() {
>     ...
>     tb_ring_free(net->tx_ring.ring);
>     net->tx_ring.ring = NULL;
>     ...
> }
> 
> If a concurrent transmission occurs, will tbnet_start_xmit() access the null
> pointer and cause a crash?

It should not happen see above.

> Additionally, since this does not hold the rtnl_lock(), could this race
> with concurrent administrative state changes (like 'ip link set dev down')
> and lead to a double-execution of tbnet_stop()?

As far as I can tell no because when these hooks are called userspace is
not running anymore (we are rebooting or shutting down).

Re: [PATCH 1/2] net: thunderbolt: Call tbnet_stop() upon shutdown

[Andrew Lunn]

On Wed, Apr 29, 2026 at 07:30:46AM +0200, Mika Westerberg wrote:
> On Tue, Apr 28, 2026 at 06:12:24PM -0700, Jakub Kicinski wrote:
> > 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: Call tbnet_stop() upon shutdown
> >     
> > This commit updates the shutdown callback for the Thunderbolt network service
> > driver to call tbnet_stop() if the interface is running. This ensures the
> > service drivers release the rings upon shutdown.
> > 
> > > diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> > > index 7aae5d915a1e..631af237998d 100644
> > > --- a/drivers/net/thunderbolt/main.c
> > > +++ b/drivers/net/thunderbolt/main.c
> > > @@ -1417,7 +1417,10 @@ static void tbnet_remove(struct tb_service *svc)
> > >  
> > >  static void tbnet_shutdown(struct tb_service *svc)
> > >  {
> > > -	tbnet_tear_down(tb_service_get_drvdata(svc), true);
> > > +	struct tbnet *net = tb_service_get_drvdata(svc);
> > > +
> > > +	if (netif_running(net->dev))
> > > +		tbnet_stop(net->dev);
> > 
> > Does calling tbnet_stop() directly bypass the network stack teardown sequence?
> 
> This is called when the machine is rebooted or shut down to quiesce the
> device. I don't think the network stack has anything similar?

You mean that network interfaces are left running while the machine
reboots, potentially DMAing receive packets over the bootloader or
freshly loaded kernel? Does that seem reasonable? Don't you think the
core will somehow call the .close function? I cannot point to the code
doing that without a bit if searching, but i think it probably exists.

	Andrew

Re: [PATCH 1/2] net: thunderbolt: Call tbnet_stop() upon shutdown

[Mika Westerberg]

On Wed, Apr 29, 2026 at 03:31:43PM +0200, Andrew Lunn wrote:
> On Wed, Apr 29, 2026 at 07:30:46AM +0200, Mika Westerberg wrote:
> > On Tue, Apr 28, 2026 at 06:12:24PM -0700, Jakub Kicinski wrote:
> > > 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: Call tbnet_stop() upon shutdown
> > >     
> > > This commit updates the shutdown callback for the Thunderbolt network service
> > > driver to call tbnet_stop() if the interface is running. This ensures the
> > > service drivers release the rings upon shutdown.
> > > 
> > > > diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> > > > index 7aae5d915a1e..631af237998d 100644
> > > > --- a/drivers/net/thunderbolt/main.c
> > > > +++ b/drivers/net/thunderbolt/main.c
> > > > @@ -1417,7 +1417,10 @@ static void tbnet_remove(struct tb_service *svc)
> > > >  
> > > >  static void tbnet_shutdown(struct tb_service *svc)
> > > >  {
> > > > -	tbnet_tear_down(tb_service_get_drvdata(svc), true);
> > > > +	struct tbnet *net = tb_service_get_drvdata(svc);
> > > > +
> > > > +	if (netif_running(net->dev))
> > > > +		tbnet_stop(net->dev);
> > > 
> > > Does calling tbnet_stop() directly bypass the network stack teardown sequence?
> > 
> > This is called when the machine is rebooted or shut down to quiesce the
> > device. I don't think the network stack has anything similar?
> 
> You mean that network interfaces are left running while the machine
> reboots, potentially DMAing receive packets over the bootloader or
> freshly loaded kernel? Does that seem reasonable? 

Hey! 20+ years ago I spent quite amount of time to investigate an issue
exactly like that where ramdisk randomly corrupted. Not fun.

> Don't you think the core will somehow call the .close function? I cannot
> point to the code doing that without a bit if searching, but i think it
> probably exists.

If I do 'reboot -f' .ndo_stop (I think that's what you mean by .close) is
not called.

If I do just 'reboot' it is called. I suspect based on my added WARN_ON()
that it is done by userspace shutdown scripts.