Re: [PATCH 3/4] thunderbolt: improve usb3 adapter link up timeout value

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] thunderbolt: stale 1-second timeout in tb_wait_for_port() kernel-doc
- [Low] thunderbolt: commit message misidentifies lane adapter timeout as USB3
--

commit bc67946f50d0699b97b8acb535ee59b1d04143ab
Author: Chunfeng Yun <chunfeng.yun@mediatek.com>

thunderbolt: improve usb3 adapter link up timeout value

This commit increases the retry count in tb_wait_for_port() from 10 to 25
(raising the timeout from 1 second to 2.5 seconds) so that ports with slow
link training do not time out and disconnect during resume.

>     thunderbolt: improve usb3 adapter link up timeout value
>     
>     when plug into a usb3 device into usb4 hub's DFP to trigger usb4
>     driver wakeup, encounter usb3 link up timeout issue, then cause
>     usb3 hub disconnect:
>     
>     usb4_hi NVDA8100:00: 0:1: failed to reach state TB_PORT_UP. Ignoring port...
>     usb4_hi NVDA8100:00: 0:1: lost during suspend, disconnecting
>     usb4_hi NVDA8100:00: 0:4 <-> 1:20 (USB3): deactivating
>     usb4_hi NVDA8100:00: deactivating USB3 Down path from 0:4 to 1:20
>     usb4_hi NVDA8100:00: deactivating USB3 Up path from 1:20 to 0:4
>     ...
>     thunderbolt 0-1: device disconnected
>     usb 2-1: USB disconnect, device number 10
>     
>     Completes link training at ~1.48s and transitions to TB_PORT_UP after
>     change the timeout to 2.5s:
>     
>     In the USB4 spec, Phase 5 (Link Equalization) alone is allowed up to
>     1000 ms. When add tPhase3 (100 ms), tPhase4 (100 ms), and retimer
>     transaction delays (tCmdResponse = 50 ms), the physical link is
>     allowed by spec to take 1200 ms to 1500 ms+ to complete initialization.
>     
>     here are two retimers in the link, if more retimers add it, need more
>     time
>     
>     Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

[Severity: Low]
This isn't a bug in the code, but is the commit subject and description
accurate in referring to a USB3 adapter link-up timeout?

Looking at tb_wait_for_port() in drivers/thunderbolt/switch.c, it requires
port->cap_phy and polls tb_port_state() for TB_PORT_UP (CL0). In
tb_switch_resume(), tb_wait_for_port() is only called on physical lane
adapters (checked via tb_port_is_null(port), which corresponds to port 0:1
in the log):

drivers/thunderbolt/switch.c:tb_switch_resume() {
	...
	tb_switch_for_each_port(sw, port) {
		if (!tb_port_is_null(port))
			continue;

		if (!tb_port_resume(port))
			continue;

		if (tb_wait_for_port(port, true) <= 0) {
	...
}

USB3 protocol adapters (ports 0:4 and 1:20 in the log) do not have cap_phy
and were only deactivated as a downstream consequence after lane adapter 0:1
timed out and the downstream router was marked unplugged. In addition,
changing retries in tb_wait_for_port() affects all lane adapter readiness
waits across tb_scan_port(), tb_switch_lane_bonding_enable(), and
tb_xdomain_lane_bonding_enable().

Could the commit title and message be updated to clarify that this increases
the physical lane adapter link-up timeout rather than a USB3 adapter timeout?

> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index d22d8db8f8906..e7ee9639c54cd 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -500,8 +500,9 @@ int tb_port_state(struct tb_port *port)
>   */
>  int tb_wait_for_port(struct tb_port *port, bool wait_if_unplugged)
>  {
> -	int retries = 10;
> +	int retries = 25;

[Severity: Low]
This isn't a bug, but with retries increased to 25 (2.5 seconds at 100 ms per
retry), should the kernel-doc comment immediately above tb_wait_for_port() in
drivers/thunderbolt/switch.c also be updated where it still states a 1-second
timeout?

drivers/thunderbolt/switch.c:
/**
 * tb_wait_for_port() - wait for a port to become ready
 * @port: Port to wait
 * @wait_if_unplugged: Wait also when port is unplugged
 *
 * Wait up to 1 second for a port to reach state TB_PORT_UP. If
 ...
 * Return:
 * * %0 - If the port is not connected or failed to reach
 *   state %TB_PORT_UP within one second.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030082539.1208733-1-chunfeng.yun@mediatek.com?part=3