Re: [PATCH v2 10/22] thunderbolt: Unlock host router ports during startup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Empty downstream ports of the host router are not unlocked during system or runtime resume.

Pre-existing issues:
- [High] A concurrent execution of `tb_remove_work` during domain teardown causes a NULL pointer dereference because the work isn't fully synchronized before resources are cleared.
--

commit 26f03f28fcb6a632e2cb7c69ea5f90adb5f5b97b
Author: Sven Peter <sven@kernel.org>

thunderbolt: Unlock host router ports during startup

The downstream ports of the host router may still be locked after a
reset. Unlock them when the software connection manager starts before
scanning for any connected devices.

> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 47753a5c0f2eb..bf1c192ec10a5 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -3049,6 +3050,14 @@ static int tb_start(struct tb *tb, bool reset)
>  		return dev_err_probe(tb->nhi->dev, ret, "failed to add host router\n");
>  	}
>  
> +	/* Make all host router downstream ports accessible to the CM. */
> +	tb_switch_for_each_port(tb->root_switch, port) {
> +		if (!tb_port_is_null(port))
> +			continue;
> +		if (tb_port_unlock(port))
> +			tb_port_warn(port, "failed to unlock port\n");
> +	}
> +

[Severity: High]
Does this logic also need to be applied during system or runtime resume?

If the system resumes from sleep and the hardware resets to its default
locked state, it appears empty ports might not be unlocked.

In drivers/thunderbolt/switch.c:tb_switch_resume(), tb_port_resume() is
used to check if a port had a connected device:

	tb_switch_for_each_port(sw, port) {
		if (!tb_port_is_null(port))
			continue;

		if (!tb_port_resume(port))
			continue;

		...
		} else {
			/*
			 * Always unlock the port so the downstream
			 * switch/domain is accessible.
			 */
			if (tb_port_unlock(port))
				tb_port_warn(port, "failed to unlock port\n");

Because tb_port_resume() returns false for empty ports, the loop will
continue early and skip the tb_port_unlock() logic.

Will this prevent the software connection manager from discovering
downstream routers plugged in after wake?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential race
condition with tb_remove_work() that could lead to a NULL pointer
dereference during driver unbind or hot-unplug.

In drivers/thunderbolt/tb.c:tb_stop(), the driver attempts to cancel the
cleanup workqueue using a non-blocking cancel:

	cancel_delayed_work(&tcm->remove_work);

If tb_remove_work() is already executing and waiting for tb->lock, it will
continue execution once tb_stop() releases the lock. Meanwhile, tb_stop()
proceeds to set tb->root_switch = NULL.

When tb_remove_work() acquires the lock, it observes tb->root_switch is
NULL:

drivers/thunderbolt/tb.c:tb_remove_work() {
	...
	mutex_lock(&tb->lock);
	if (tb->root_switch)
		tb_free_unplugged_children(tb->root_switch);
	mutex_unlock(&tb->lock);

	tb_free_unplugged_xdomains(tb->root_switch);
}

After dropping the lock, it unconditionally calls
tb_free_unplugged_xdomains(tb->root_switch), passing the NULL pointer.

Because tb_free_unplugged_xdomains() lacks a NULL check, can this
immediately dereference the pointer in the tb_switch_for_each_port() macro
and cause a panic?

>  	/*
>  	 * To support highest CLx state, we set host router's TMU to
>  	 * Normal mode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-b4-apple-soc-tbt-v2-0-1f80085f93fb@kernel.org?part=10