Re: [PATCH v2 08/22] thunderbolt: Don't read the UID if we already know it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch fails to skip the config space UID read on Apple Silicon host routers due to an incorrect assumption about the execution order of `tb_switch_set_uuid()`.
--

commit 79dbfccf7bd2744a7a8dfb582a24daa82d8e7d2c
Author: Sven Peter <sven@kernel.org>
Subject: thunderbolt: Don't read the UID if we already know it

This commit attempts to skip reading the UID from the config space on Apple
Silicon host routers by checking if sw->uid is already populated. It also
claims to drop a duplicate read for other USB4 hosts.

> On Apple Silicon the host router's UID comes from the DROM provided by
> the device tree and is already known by the time tb_switch_set_uuid()
> runs. Skip reading it from the config space again in that case.
> 
> For other USB4 hosts this only drops a duplicate read because
> tb_drom_host_read() has already filled in the UID from ROUTER_CS_7
> before tb_switch_set_uuid() runs. Device routers keep reading it
> unconditionally.

Is this execution order correct?

Looking at tb_switch_add(), tb_switch_add_dma_port() unconditionally calls
tb_switch_set_uuid() before tb_drom_read() executes:

drivers/thunderbolt/switch.c:tb_switch_add() {
	...
	ret = tb_switch_add_dma_port(sw);
	...
		/* read drom */
		ret = tb_drom_read(sw);
	...
}

During tb_switch_add_dma_port(), tb_switch_set_uuid() is called directly.

Because of this order, doesn't tb_switch_set_uuid() run before the UID
is populated from either the device tree or ROUTER_CS_7?

> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 404c0693df50e..3f54a5ae56b07 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -2692,9 +2692,11 @@ static int tb_switch_set_uuid(struct tb_switch *sw)
>  		return 0;
>  
>  	if (tb_switch_is_usb4(sw)) {
> -		ret = usb4_switch_read_uid(sw, &sw->uid);
> -		if (ret)
> -			return ret;
> +		if (tb_route(sw) || !sw->uid) {
> +			ret = usb4_switch_read_uid(sw, &sw->uid);

[Severity: High]
Since sw->uid is still 0 during the initial call from tb_switch_add_dma_port(),
doesn't this condition evaluate to true and force the usb4_switch_read_uid()
read anyway?

If reading ROUTER_CS_7 returns garbage on Apple Silicon hardware, will the
host router be assigned an incorrect UUID since sw->uuid is set during this
first call and won't be updated later?

> +			if (ret)
> +				return ret;
> +		}
>  		uid = true;
>  	} else {
>  		/*

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