Re: [PATCH v2] thunderbolt: Optionally keep PCIe tunnels up through suspend-to-idle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] thunderbolt: missing tb_free_unplugged_xdomains() in
  tb_resume_noirq()
- [Medium] thunderbolt: incomplete XDomain replacement and child rescan
  on s2idle resume
- [Medium] thunderbolt: incomplete topology rescan in tb_complete() via
  tb_scan_port()
- [Medium] thunderbolt: missing router enumeration and UID checks in
  tb_check_kept_running()
- [Medium] thunderbolt: unquiesced delayed works access stopped ctl in
  tb_suspend_noirq()
- [Medium] thunderbolt: stale DP tunnels after monitor unplug during
  kept-running s2idle
--

commit a1b4dee4a46d2ea714a73ee4360b3adbd9dd4c3f
Author: Andrei Rusu de Castro <arc@empyreal.works>

thunderbolt: Optionally keep PCIe tunnels up through suspend-to-idle

This commit adds an s2idle_keep_pcie module parameter to keep USB4 routers
and tunnels up across suspend-to-idle when a PCIe tunnel is active, while
stopping hotplug handling and the control channel during sleep and checking
for topology changes on resume.

> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 418ad0a61a5de..a7528b93a7037 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
[ ... ]
> @@ -3106,10 +3112,94 @@ static int tb_start(struct tb *tb, bool reset)
[ ... ]
> +static void tb_check_kept_running(struct tb_switch *sw)
> +{
> +	struct tb_port *port;
> +
> +	tb_switch_for_each_port(sw, port) {
> +		struct tb_switch *child;
> +		u64 uid;
> +		int ret;
> +
> +		if (!tb_port_is_null(port))
> +			continue;
> +
> +		if (port->xdomain) {
> +			if (tb_wait_for_port(port, false) <= 0)
> +				port->xdomain->is_unplugged = true;
> +			continue;
> +		}

[Severity: Medium]
What happens if an XDomain is unplugged during suspend-to-idle and
replaced by a router on the same port?

Because the link is up, tb_wait_for_port() returns 1 and
port->xdomain->is_unplugged stays false.  Unlike tb_switch_resume(),
tb_check_kept_running() does not unlock the port and probe
tb_cfg_get_upstream_port(sw->tb->ctl, port->xdomain->route) to detect that
a router is now at that route.

When tb_complete() then runs, tb_domain_unregister_unplugged_xdomains()
does not unregister port->xdomain, and tb_scan_port() calls
tb_xdomain_remove() (which leaves a registered xd->dev on tb_bus_type) and
clears port->xdomain, leaking xd->dev on the bus and causing
tb_switch_add() for the new router to fail with -EEXIST due to the
colliding device name.

Also, in tb_complete(), tb_scan_port() returns early whenever port->remote
is already set without recursing into port->remote->sw, so replacements or
newly plugged devices on surviving child switches are skipped.

> +		if (!tb_port_has_remote(port))
> +			continue;
> +
> +		child = port->remote->sw;
> +		ret = tb_cfg_get_upstream_port(sw->tb->ctl, tb_route(child));
> +		if (ret >= 0 && child->uid) {
> +			if (tb_switch_is_usb4(child))
> +				ret = usb4_switch_read_uid(child, &uid);
> +			else
> +				ret = tb_drom_read_uid_only(child, &uid);
> +			if (!ret && uid != child->uid)
> +				ret = -ENODEV;
> +		}

[Severity: Medium]
Can a child router that power-cycled or reset in place during
suspend-to-idle pass this check while unenumerated?

ROUTER_CS_0 (read by tb_cfg_get_upstream_port()) and the UID registers or
DROM remain readable after a router reset even though ROUTER_CS_3_V
(checked by tb_switch_enumerated()) was cleared and all hardware HopID
tunnel entries were wiped.  Because tb_resume_noirq() skips
tb_switch_configure() and tb_tunnel_activate() when tb->kept_running is
set, keeping a router whose ROUTER_CS_3_V bit is cleared leaves its
tunnels dead in hardware.

In addition, if child->uid is 0 (for example in safe mode), the
child->uid check skips UID verification altogether (whereas
tb_switch_resume() rejects !sw->uid with -ENODEV), and the upstream port
returned by tb_cfg_get_upstream_port() is not compared against
child->config.upstream_port_number.

> +		if (ret < 0) {
> +			tb_port_warn(port, "lost during suspend, disconnecting\n");
> +			tb_sw_set_unplugged(child);
> +			continue;
> +		}
> +
> +		tb_check_kept_running(child);
> +	}
> +}
> +
>  static int tb_suspend_noirq(struct tb *tb)
>  {
>  	struct tb_cm *tcm = tb_priv(tb);
>  
> +	if (tb_keep_running_s2idle(tb)) {
> +		tb_dbg(tb, "PCIe tunnels active, keeping the domain running\n");
> +		/* As for freeze, no hotplug handling until resume */
> +		tcm->hotplug_active = false;
> +		tb->kept_running = true;
> +		return 0;
> +	}

[Severity: Medium]
Can delayed work items access tb->ctl while it is stopped during
suspend-to-idle?

Skipping tb_disconnect_and_release_dp() leaves DP tunnels and bandwidth
groups active while tb_domain_suspend_noirq() stops tb->ctl.  Because
group->release_work (on system_percpu_wq) and tunnel->dprx_work or
tcm->remove_work (on tb->wq, which is allocated without WQ_FREEZABLE) are
neither canceled nor guarded by tcm->hotplug_active, they can run during
suspend-to-idle and issue config reads/writes on the stopped control
channel, triggering WARN() in tb_cfg_read() and tb_cfg_write() on
-ENOTCONN and leaving software state out of sync with hardware.

[Severity: Medium]
What happens if a DisplayPort monitor on a surviving router is unplugged
during suspend-to-idle while tb->kept_running is set?

Because tb_disconnect_and_release_dp() is skipped and hotplug events are
ignored while asleep, the DP tunnel stays active.  On resume,
tb_check_kept_running() skips non-null ports, and tb_scan_port() only
checks DP OUT ports where !tb_dp_port_is_enabled(port), so lost HPD on an
enabled DP OUT port is never handled and the stale DP tunnel and DP IN
resource remain allocated.

[ ... ]
> @@ -3177,6 +3267,17 @@ static int tb_resume_noirq(struct tb *tb)
>  	unsigned int usb3_delay = 0;
>  	LIST_HEAD(tunnels);
>  
> +	if (tb->kept_running) {
> +		tb->kept_running = false;
> +		tb_dbg(tb, "domain kept running, checking for changes\n");
> +		tb_check_kept_running(tb->root_switch);
> +		tb_free_invalid_tunnels(tb);
> +		tb_free_unplugged_children(tb->root_switch);
> +		tcm->rescan_on_complete = true;
> +		tcm->hotplug_active = true;
> +		return 0;
> +	}

[Severity: High]
Does this miss a call to tb_free_unplugged_xdomains(tb->root_switch)?

If an XDomain attached to tb->root_switch or a surviving child switch is
unplugged during suspend-to-idle, tb_check_kept_running() sets
port->xdomain->is_unplugged = true, but tb_free_unplugged_children() only
processes ports with tb_port_has_remote(port).  Without
tb_free_unplugged_xdomains(), tb_xdomain_remove() is never called and
port->xdomain is not cleared.

During dpm_resume, tb_xdomain_resume() then restarts xd->state_work, and
tb_complete() -> tb_domain_unregister_unplugged_xdomains() unregisters and
frees the struct tb_xdomain while xd->state_work is still queued and
port->xdomain still points to the freed object, causing a use-after-free
(or leaking xd if xd->dev was not yet registered on tb_bus_type).

[ ... ]
> @@ -3250,12 +3351,17 @@ static int tb_thaw_noirq(struct tb *tb)
>  
>  static void tb_complete(struct tb *tb)
>  {
> +	struct tb_cm *tcm = tb_priv(tb);
> +	bool rescan = tcm->rescan_on_complete;
> +
> +	tcm->rescan_on_complete = false;
> +
>  	/*
>  	 * Unregister unplugged XDomains and if there is a case where
>  	 * another domain is swapped in place of unplugged XDomain we
>  	 * need to run another rescan.
>  	 */
> -	if (tb_domain_unregister_unplugged_xdomains(tb)) {
> +	if (tb_domain_unregister_unplugged_xdomains(tb) || rescan) {
>  		scoped_guard(mutex, &tb->lock)
>  			tb_scan_switch(tb->root_switch);
>  	}

[Severity: Medium]
Does tb_scan_switch(tb->root_switch) rescan downstream ports of child
routers that stayed up during suspend-to-idle?

In tb_scan_port(), any port that already has port->remote set returns
early without recursing into port->remote->sw:

drivers/thunderbolt/tb.c:tb_scan_port() {
    ...
	if (port->remote) {
		tb_port_dbg(port, "port already has a remote\n");
		goto out_rpm_put;
	}
    ...
}

Because s2idle_keep_pcie only triggers when a downstream router with a
PCIe tunnel is present on tb->root_switch, port->remote is non-NULL for
every surviving child router, so devices or monitors plugged into or
replaced behind a kept-running dock or enclosure during sleep are never
scanned on resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-tb-s2idle-keep-pcie-v2-b3c197ac@empyreal.works?part=1