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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] thunderbolt: active ctl channel and hotplug work during s2idle
  in tb_suspend_noirq()
--

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

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

This commit adds a thunderbolt.s2idle_keep_pcie module parameter to keep
the domain, control channel, and routers running during suspend-to-idle
when a PCIe tunnel is active.

> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 24611f05b3cda..168a286cd516b 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -535,7 +535,7 @@ int tb_domain_suspend_noirq(struct tb *tb)
>  	mutex_lock(&tb->lock);
>  	if (tb->cm_ops->suspend_noirq)
>  		ret = tb->cm_ops->suspend_noirq(tb);
> -	if (!ret)
> +	if (!ret && !tb->kept_running)
>  		tb_ctl_stop(tb->ctl);
>  	mutex_unlock(&tb->lock);
>  

[ ... ]

> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index f54ab2189cf42..c453153c2ef12 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -1145,6 +1156,9 @@ static int nhi_resume_noirq(struct device *dev)
>  	struct tb_nhi *nhi = tb->nhi;
>  	int ret;
>  
> +	if (tb->kept_running)
> +		return tb_domain_resume_noirq(tb);
> +
>  	/*
>  	 * Check that the device is still there. It may be that the user
>  	 * unplugged last device which causes the host controller to go
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 418ad0a61a5de..641502ec2efa4 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c

[ ... ]

> @@ -3106,10 +3108,45 @@ static int tb_start(struct tb *tb, bool reset)
> +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");
> +		tb->kept_running = true;
> +		return 0;
> +	}
> +
>  	tb_dbg(tb, "suspending...\n");
>  	tb_disconnect_and_release_dp(tb);
>  	tb_switch_exit_redrive(tb->root_switch);
> @@ -3177,6 +3214,12 @@ 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, tunnels intact\n");
> +		return 0;
> +	}
> +
>  	tb_dbg(tb, "resuming...\n");

[Severity: High]
Can leaving tb->ctl running and tcm->hotplug_active set in
tb_suspend_noirq() cause hotplug and DisplayPort bandwidth work to run
while devices are suspended in the noirq phase?

tb_ctl_alloc() allocates ctl->tx and ctl->rx with Ring_Flag_No_Suspend,
so the control channel interrupt stays enabled across suspend_device_irqs()
without arming wakeup, and tb_domain_alloc() creates tb->wq without
Wq_Freezable.  If a connected monitor enters standby after the GPU
suspends and sends a DisplayPort notification, or a cable is plugged or
unplugged during suspend-to-idle, tb_handle_hotplug() or
tb_handle_dp_bandwidth_request() runs while runtime power management is
disabled on all devices.

In that state, pm_runtime_get_sync() fails with -Eacces, a plug event
calls tb_switch_add() -> device_add() -> device_pm_add() under a prepared
parent (warning and placing unsuspended devices on dpm_list while their
parents are on dpm_noirq_list), and an unplug event tears down tunnels and
unregisters switches while downstream PCI devices are suspended.

In addition, returning early in nhi_resume_noirq() and tb_resume_noirq()
skips the nhi->ops->is_present() check and tb_free_unplugged_children() if
the enclosure was unplugged during sleep.  Should this path set
tcm->hotplug_active = false and stop tb->ctl like tb_freeze_noirq() and
tb_domain_freeze_noirq() do when keeping tunnels up across freeze, and
still check presence and clean up unplugged devices on resume?

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