Re: [PATCH 2/3] thunderbolt: Reset the host interface before reusing a DMA HopID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] thunderbolt: NULL pointer dereference in nhi_reset_work() during probe failure
- [High] thunderbolt: lost control channel interrupt due to race in nhi_reset_work()
- [Low] net: thunderbolt: mixed goto and guard() cleanup in tbnet_connected_work()
--

commit dc6bb5398632a1b975f04bc4d5441e027de1977f
Author: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Subject: thunderbolt: Reset the host interface before reusing a DMA HopID

This patch tracks DMA HopIDs and resets the host interface once all DMA rings
are idle to allow safe HopID reuse. It also defers ring allocation when HopIDs
are dirty and blocks control and DMA traffic during the reset.

Link: https://lore.kernel.org/linux-usb/20260831130638.GK124825@black.igk.intel.com/T/#m668c2efcfe2f298632537721d72b17445b39211a

> diff --git a/drivers/net/thunderbolt/main.c b/drivers/net/thunderbolt/main.c
> index 93ccccc5cf8bb..f2dbb6fb30c63 100644
> --- a/drivers/net/thunderbolt/main.c
> +++ b/drivers/net/thunderbolt/main.c

[ ... ]

> @@ -644,9 +657,9 @@ static void tbnet_connected_work(struct work_struct *work)
>  	if (netif_carrier_ok(net->dev))
>  		return;
>  
> -	mutex_lock(&net->connection_lock);
> -	connected = net->login_sent && net->login_received;
> -	mutex_unlock(&net->connection_lock);
> +	scoped_guard(mutex, &net->connection_lock)
> +		connected = !net->stopping && !net->disconnecting &&
> +			    net->login_sent && net->login_received;
>  
>  	if (!connected)
>  		return;

[ ... ]

> @@ -714,24 +732,34 @@ static void tbnet_connected_work(struct work_struct *work)
>  err_release_hopid:
>  	tb_xdomain_release_in_hopid(net->xd, net->remote_transmit_path);
>  	tbnet_connect_failed(net);
> +	if (ret == -EAGAIN) {
> +		guard(mutex)(&net->connection_lock);
> +		if (!net->stopping && !net->disconnecting)
> +			queue_delayed_work(system_long_wq, &net->login_work,
> +					   msecs_to_jiffies(TBNET_LOGIN_DELAY));
> +	}
>  }

[Severity: Low]
This isn't a bug, but does this function mix goto-based error handling with
scoped-based cleanup? The cleanup subsystem guidelines indicate that we should
either convert all resources that need a goto cleanup to scope-based cleanup,
or convert none of them. Since this function still uses goto labels like
err_release_hopid and err_free_tx_buffers, adding scoped_guard and guard could
create confusing ownership semantics.

> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 1960fa30e13af..a68ecabadcb57 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c

[ ... ]

> @@ -513,7 +515,8 @@ void tb_ring_poll_complete(struct tb_ring *ring)
>  
>  	spin_lock_irqsave(&ring->nhi->lock, flags);
>  	spin_lock(&ring->lock);
> -	if (ring->start_poll)
> +	if (ring->start_poll && ring->running && !ring->nhi->resetting &&
> +	    !ring->nhi->going_away)
>  		__ring_interrupt_mask(ring, false);
>  	spin_unlock(&ring->lock);
>  	spin_unlock_irqrestore(&ring->nhi->lock, flags);

[ ... ]

> +static void nhi_reset_work(struct work_struct *work)
> +{
> +	struct tb_nhi *nhi = container_of(to_delayed_work(work), struct tb_nhi,
> +					  reset_work);
> +	struct tb *tb = dev_get_drvdata(nhi->dev);
> +
> +	/* The connection manager must be blocked over the reset */
> +	guard(mutex)(&tb->lock);

[Severity: High]
Could this dereference a NULL tb pointer during probe failure? 

In nhi_probe(), tb_domain_add() is called prior to dev_set_drvdata(). 
If tb_domain_add() discovers routers and synchronously binds drivers that start
and stop DMA rings, tb_ring_stop() can queue nhi->reset_work. If 
tb_domain_add() subsequently fails, the probe error path tears down the domain
and nhi_reset_work() may execute from the workqueue concurrently.

At this point, dev_get_drvdata() would return NULL because dev_set_drvdata()
was never reached, leading to a kernel panic when tb->lock is accessed.

> +
> +	scoped_guard(spinlock_irq, &nhi->lock) {
> +		if (nhi->going_away || nhi->removing || nhi->suspended)
> +			return;
> +		if (bitmap_empty(nhi->dma_hops_used, nhi_dma_hops_bits(nhi)))
> +			return;
> +		if (nhi_dma_rings_running(nhi))
> +			return;
> +
> +		/* Keep the DMA rings from starting over the reset */
> +		nhi->resetting = true;
> +	}
> +
> +	if (!tb_ctl_stop_if_idle(tb->ctl)) {
> +		scoped_guard(spinlock_irq, &nhi->lock) {
> +			nhi->resetting = false;
> +			queue_delayed_work(system_long_wq, &nhi->reset_work,
> +					   msecs_to_jiffies(100));
> +		}
> +		return;
> +	}
> +	nhi_reset_interface(nhi);
> +	tb_ctl_start(tb->ctl);
> +
> +	scoped_guard(spinlock_irq, &nhi->lock) {
> +		bitmap_zero(nhi->dma_hops_used, nhi_dma_hops_bits(nhi));
> +		nhi->resetting = false;
> +	}
> +}

[Severity: High]
Does this sequence open a race condition that permanently loses the control
channel interrupt? 

When tb_ctl_start() is called, nhi->resetting is still true. The hardware can
immediately receive a packet and trigger an MSI-X interrupt, which masks the
interrupt and schedules the polling callback.

When the callback processes the packet and calls tb_ring_poll_complete(),
the check for !ring->nhi->resetting will fail because nhi->resetting is only
cleared after tb_ctl_start() returns. This skips the unmasking of the
interrupt and leaves the control channel permanently hung. 

Should nhi->resetting = false be executed before calling tb_ctl_start() to
close this window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790854235.git.Basavaraj.Natikar@amd.com?part=2