Re: [PATCH v2 0/2] thunderbolt: Fix S4 resume incongruities

[Mario Limonciello <superm1@kernel.org>]

On 1/29/26 17:13, Katiyar, Pooja wrote:
> Hello,
> 
> On Mon, Jan 19, 2026 at 02:13:50PM -0800, Kenneth Crudup wrote:
>>
>> On 1/19/26 11:59, Mario Limonciello (AMD) (kernel.org) wrote:
>>
>>> On 1/17/2026 10:57 AM, Katiyar, Pooja wrote:
>>>
>>>>> I have confirmation the hack patch does help the issue for us too.
>>>>>
>>>>> If your patch doesn't work another logical solution could be to destroy
>>>>> all the tunnels as part of the PM freeze callback (not just the DP
>>>>> resources).  Maybe even unify the suspend and freeze codepaths for more
>>>>> opportunities for code reuse?
>>>>>
>>>>
>>>> Thanks for confirming the hack patch helps!
>>>>
>>>> We are actually working on a solution that releases the DP resources and
>>>> suspends the switch as part of the freeze sequence. This way the hibernation
>>>> image that is stored doesn't contain any active tunnels, and during resume
>>>> we get a DP hotplug notification for a new tunnel, similar to S5. So far
>>>> this patch is working fine but is under review.
>>>>
>>>
>>> Thanks.  If you want early testing from us too before you're ready to
>>> post publicly feel free to ping it offline to me too.
>>
>> I'd like to get a CC: on that, too.
>>
>> I've been testing that hack patch and will test further later tonight.
>>
>> The issue I'm trying to chase down (and not sure if any of this will
>> help with this, I wonder if it's really BIOS/EC related) is often times
>> that after a suspend (or hibernate, but I use "suspend then hibernate",
>> which I think does both and chooses which to use upon resume) and then
>> connect to a different dock (or setup) from the one I'd suspended with,
>> sometimes I have to unplug/replug my TB cable, otherwise I either get no
>> recognition of my new display setup (and sometimes TB devices) or it'll
>> try and use the same monitor resolution of the previously-connected
>> monitor (as if the TB subsystem doesn't recognize things have changed).
>>
> 
> Below is the patch series that addresses mentioned issue. There are two
> patches in this series. The series takes care of releasing the DP resources
> as part of freeze call before the hibernation image is created. You can test
> it for your issues and let us know if it helps.
> 
> Please note that these changes are still under internal review and are
> subject to change.
> 
> From: Pooja Katiyar <pooja.katiyar@intel.com>
> Date: Thu, 22 Jan 2026 11:57:07 -0800
> Subject: [RFC PATCH 1/2] thunderbolt: Add helper functions for suspend/resume
>   operations
> 
> Extract common resume logic from tb_resume_noirq() into
> tb_recover_tunnels() helper function to consolidate switch resume,
> tunnel discovery/teardown, tunnel recreation, and topology
> reinitialization logic.
> 
> Introduce __nhi_suspend_ops() and __nhi_resume_noirq() helper functions
> to consolidate common suspend/resume logic used by multiple PM callbacks.
> 
> This is a preparatory cleanup for hibernation improvements.
> 
> Co-developed-by: Rene Sapiens <rene.sapiens@linux.intel.com>
> Signed-off-by: Rene Sapiens <rene.sapiens@linux.intel.com>
> Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
> ---
>   drivers/thunderbolt/nhi.c | 46 ++++++++++++++++++++++++++-------------
>   drivers/thunderbolt/tb.c  | 34 ++++++++++++++++++++---------
>   2 files changed, 55 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 6d0c9d37c55d..5b84223937fb 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -971,7 +971,14 @@ static irqreturn_t nhi_msi(int irq, void *data)
>   	return IRQ_HANDLED;
>   }
>   
> -static int __nhi_suspend_noirq(struct device *dev, bool wakeup)
> +static int __nhi_suspend_ops(struct tb_nhi *nhi, bool wakeup)
> +{
> +	if (nhi->ops && nhi->ops->suspend_noirq)
> +		return nhi->ops->suspend_noirq(nhi, wakeup);
> +	return 0;
> +}
> +
> +static int nhi_suspend_noirq(struct device *dev)
>   {
>   	struct pci_dev *pdev = to_pci_dev(dev);
>   	struct tb *tb = pci_get_drvdata(pdev);
> @@ -982,18 +989,7 @@ static int __nhi_suspend_noirq(struct device *dev, bool wakeup)
>   	if (ret)
>   		return ret;
>   
> -	if (nhi->ops && nhi->ops->suspend_noirq) {
> -		ret = nhi->ops->suspend_noirq(tb->nhi, wakeup);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int nhi_suspend_noirq(struct device *dev)
> -{
> -	return __nhi_suspend_noirq(dev, device_may_wakeup(dev));
> +	return __nhi_suspend_ops(nhi, device_may_wakeup(dev));
>   }
>   
>   static int nhi_freeze_noirq(struct device *dev)
> @@ -1029,10 +1025,17 @@ static bool nhi_wake_supported(struct pci_dev *pdev)
>   static int nhi_poweroff_noirq(struct device *dev)
>   {
>   	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct tb *tb = pci_get_drvdata(pdev);
> +	struct tb_nhi *nhi = tb->nhi;
>   	bool wakeup;
> +	int ret;
> +
> +	ret = tb_domain_suspend_noirq(tb);
> +	if (ret)
> +		return ret;
>   
>   	wakeup = device_may_wakeup(dev) && nhi_wake_supported(pdev);
> -	return __nhi_suspend_noirq(dev, wakeup);
> +	return __nhi_suspend_ops(nhi, wakeup);
>   }
>   
>   static void nhi_enable_int_throttling(struct tb_nhi *nhi)
> @@ -1051,7 +1054,7 @@ static void nhi_enable_int_throttling(struct tb_nhi *nhi)
>   	}
>   }
>   
> -static int nhi_resume_noirq(struct device *dev)
> +static int __nhi_resume_noirq(struct device *dev)
>   {
>   	struct pci_dev *pdev = to_pci_dev(dev);
>   	struct tb *tb = pci_get_drvdata(pdev);
> @@ -1074,6 +1077,19 @@ static int nhi_resume_noirq(struct device *dev)
>   		nhi_enable_int_throttling(tb->nhi);
>   	}
>   
> +	return 0;
> +}
> +
> +static int nhi_resume_noirq(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct tb *tb = pci_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = __nhi_resume_noirq(dev);
> +	if (ret)
> +		return ret;
> +
>   	return tb_domain_resume_noirq(tb);
>   }
>   
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 4f5f1dfc0fbf..69015def6cd1 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -3110,22 +3110,21 @@ static void tb_restore_children(struct tb_switch *sw)
>   	}
>   }
>   
> -static int tb_resume_noirq(struct tb *tb)
> +/**
> + * tb_recover_tunnels() - Common resume logic for suspend and hibernate
> + * @tb: Domain to resume
> + *
> + * Common recovery logic shared between suspend resume and hibernate restore.
> + * Handles switch resume, tunnel discovery/teardown, tunnel recreation, and
> + * topology reinitialization.
> + */
> +static void tb_recover_tunnels(struct tb *tb)
>   {
>   	struct tb_cm *tcm = tb_priv(tb);
>   	struct tb_tunnel *tunnel, *n;
>   	unsigned int usb3_delay = 0;
>   	LIST_HEAD(tunnels);
>   
> -	tb_dbg(tb, "resuming...\n");
> -
> -	/*
> -	 * For non-USB4 hosts (Apple systems) remove any PCIe devices
> -	 * the firmware might have setup.
> -	 */
> -	if (!tb_switch_is_usb4(tb->root_switch))
> -		tb_switch_reset(tb->root_switch);
> -
>   	tb_switch_resume(tb->root_switch, false);
>   	tb_free_invalid_tunnels(tb);
>   	tb_free_unplugged_children(tb->root_switch);
> @@ -3166,6 +3165,21 @@ static int tb_resume_noirq(struct tb *tb)
>   	tb_switch_enter_redrive(tb->root_switch);
>   	 /* Allow tb_handle_hotplug to progress events */
>   	tcm->hotplug_active = true;
> +}
> +
> +static int tb_resume_noirq(struct tb *tb)
> +{
> +	tb_dbg(tb, "resuming...\n");
> +
> +	/*
> +	 * For non-USB4 hosts (Apple systems) remove any PCIe devices
> +	 * the firmware might have setup.
> +	 */
> +	if (!tb_switch_is_usb4(tb->root_switch))
> +		tb_switch_reset(tb->root_switch);
> +
> +	tb_recover_tunnels(tb);
> +
>   	tb_dbg(tb, "resume finished\n");
>   
>   	return 0;

Sorry for the insanely big delay, but we were getting totally unexpected 
results with your patch series.  It was helping; but then even with it 
taken off it was also helping :P.

It turned out the issue that prompted my series got fixed in another 
way.  It took some time to figure out why.  It was fixed by this change:

15b1d7b77e9836ff4184093163174a1ef28bbdd7

That is basically because of the order of events the kernel does we were 
having a link training failure.  My series (and your patches) did change 
the order of events to adjust it.  But instead we are catching this 
sequence in the GPU driver and handling it more cleanly.