Re: [PATCH] thunderbolt: Enable runtime PM for the Ice Lake router with the software CM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] thunderbolt: root switch skipped in tb_lc_set_wake() and lane
  init on ICL
- [High] thunderbolt: missing DL_FLAG_RPM_ACTIVE in add_link() suspends
  active PCIe
--

commit 7b8fe879e2da1e2a2041c441ee239114a6aec29b
Author: Andre Eikmeyer <dev@deq.rocks>

thunderbolt: Enable runtime PM for the Ice Lake router with the software CM

This commit adds tb_switch_is_ice_lake() and enables software connection
manager runtime PM on the Ice Lake host router.

> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 418ad0a61a5de..139ef49b49e5c 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -3044,8 +3044,9 @@ static int tb_start(struct tb *tb, bool reset)
>  	 * implement the necessary router operations.
>  	 */
>  	tb->root_switch->no_nvm_upgrade = !tb_switch_is_usb4(tb->root_switch);
> -	/* All USB4 routers support runtime PM */
> -	tb->root_switch->rpm = tb_switch_is_usb4(tb->root_switch);
> +	/* All USB4 routers support runtime PM. Ice Lake also supports RTD3 */
> +	tb->root_switch->rpm = tb_switch_is_usb4(tb->root_switch) ||
> +			       tb_switch_is_ice_lake(tb->root_switch);

[Severity: High]
Does enabling runtime PM on the Ice Lake root switch put its link
controllers to sleep without configuring wakeups or restarting lane
initialization on resume?

Ice Lake is a generation 3 (non-USB4) router, so tb_switch_suspend() uses
the link controller path: it disables plug events, calls
tb_switch_set_wake() -> tb_lc_set_wake(), and then calls tb_lc_set_sleep()
to set TB_LC_SX_CTRL_SLP on each link controller.  However, tb_lc_set_wake()
bails out on the root switch:

drivers/thunderbolt/lc.c:tb_lc_set_wake() {
	if (sw->generation < 2)
		return 0;

	if (!tb_route(sw))
		return 0;
    ...
}

When a connected device is runtime-suspended, icl_nhi_suspend() keeps force
power on and leaves the root switch in link controller sleep with no wake
bits (WOC, WOD, WOU4, WOP, WODPC, WODPD) programmed.  On runtime resume,
tb_runtime_resume() -> tb_switch_resume() -> tb_port_resume() ->
tb_lc_start_lane_initialization() also returns early on !tb_route(sw)
instead of asserting TB_LC_SX_CTRL_SLI on disconnected downstream lane
adapters.

[Severity: High]
Can enabling runtime PM on Ice Lake allow nhi->dev and the root switch to
runtime-suspend while a tunneled PCIe root port is still active?

On Apple Ice Lake systems, nhi_select_cm() creates the runtime PM device
links to the tunneled PCIe root ports through icl_nhi_add_links() ->
add_link() rather than tb_acpi_add_links():

drivers/thunderbolt/pci.c:add_link() {
	link = device_link_add(&pdev->dev, nhi->dev,
			       DL_FLAG_AUTOREMOVE_SUPPLIER |
			       DL_FLAG_PM_RUNTIME);
    ...
}

Unlike tb_acpi_add_link(), which was updated in f3380cac0c0b ("thunderbolt:
Runtime PM activate both ends of the device link") to wrap device_link_add()
with pm_runtime_get_sync(&pdev->dev), DL_FLAG_RPM_ACTIVE, and
pm_runtime_put(&pdev->dev), add_link() does not pass DL_FLAG_RPM_ACTIVE.
When a PCIe tunnel is already up at boot and the consumer PCIe root port is
already runtime-active when add_link() runs, nhi->dev's runtime PM usage
count is not incremented, allowing the Thunderbolt domain and root switch to
autosuspend underneath the active PCIe tunnel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111130851.62101-1-dev@deq.rocks?part=1