[PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Mario Limonciello (AMD)]

When a machine is restored from S4 if the firmware CM has created
tunnels there can be an incongruity of expectation from the kernel
when compared to booting from S5.

For example this incongruity can lead to problems like the display
not working properly if the firmware CM didn't turn it on pre-OS
but the OS had it on before entering S4.

Add an extra restore_noirq() callback to explicitly reset the NHI
before running the rest of the resume sequence.  This will allow
the standard discovery and probing routines to run and create the
tunnels as expected.

Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
Cc: Sanath.S@amd.com
---
 drivers/thunderbolt/nhi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 5f63f9b9cf405..042d184637a93 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -51,6 +51,8 @@ static bool host_reset = true;
 module_param(host_reset, bool, 0444);
 MODULE_PARM_DESC(host_reset, "reset USB4 host router (default: true)");
 
+static void nhi_reset(struct tb_nhi *nhi);
+
 static int ring_interrupt_index(const struct tb_ring *ring)
 {
 	int bit = ring->hop;
@@ -1077,6 +1079,17 @@ static int nhi_resume_noirq(struct device *dev)
 	return tb_domain_resume_noirq(tb);
 }
 
+static int nhi_restore_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;
+
+	nhi_reset(nhi);
+
+	return nhi_resume_noirq(dev);
+}
+
 static int nhi_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -1449,7 +1462,7 @@ static const struct dev_pm_ops nhi_pm_ops = {
 					    * pci-tunnels stay alive.
 					    */
 	.thaw_noirq = nhi_thaw_noirq,
-	.restore_noirq = nhi_resume_noirq,
+	.restore_noirq = nhi_restore_noirq,
 	.suspend = nhi_suspend,
 	.poweroff_noirq = nhi_poweroff_noirq,
 	.poweroff = nhi_suspend,
-- 
2.43.0

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Mika Westerberg]

Hi,

On Thu, Oct 23, 2025 at 12:03:26AM -0500, Mario Limonciello (AMD) wrote:
> When a machine is restored from S4 if the firmware CM has created
> tunnels there can be an incongruity of expectation from the kernel
> when compared to booting from S5.
> 
> For example this incongruity can lead to problems like the display
> not working properly if the firmware CM didn't turn it on pre-OS
> but the OS had it on before entering S4.
> 
> Add an extra restore_noirq() callback to explicitly reset the NHI
> before running the rest of the resume sequence.  This will allow
> the standard discovery and probing routines to run and create the
> tunnels as expected.
> 
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> Cc: Sanath.S@amd.com
> ---
>  drivers/thunderbolt/nhi.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 5f63f9b9cf405..042d184637a93 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -51,6 +51,8 @@ static bool host_reset = true;
>  module_param(host_reset, bool, 0444);
>  MODULE_PARM_DESC(host_reset, "reset USB4 host router (default: true)");
>  
> +static void nhi_reset(struct tb_nhi *nhi);

Please move the whole function above nhi_restore_noirq() then.

> +
>  static int ring_interrupt_index(const struct tb_ring *ring)
>  {
>  	int bit = ring->hop;
> @@ -1077,6 +1079,17 @@ static int nhi_resume_noirq(struct device *dev)
>  	return tb_domain_resume_noirq(tb);
>  }
>  
> +static int nhi_restore_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;
> +
> +	nhi_reset(nhi);
> +

Now that we do this, should ge get rid of the S4 things we do in
tb_resume_noirq()? Especially this block:

	/*
	 * If we get here from suspend to disk the boot firmware or the
	 * restore kernel might have created tunnels of its own. Since
	 * we cannot be sure they are usable for us we find and tear
	 * them down.
	 */
	tb_switch_discover_tunnels(tb->root_switch, &tunnels, false);
	list_for_each_entry_safe_reverse(tunnel, n, &tunnels, list) {
		if (tb_tunnel_is_usb3(tunnel))
			usb3_delay = 500;
		tb_tunnel_deactivate(tunnel);
		tb_tunnel_put(tunnel);
	}

Also have you checked that this works when you have tunnels such as P2P
(networking)?

> +	return nhi_resume_noirq(dev);
> +}
> +
>  static int nhi_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -1449,7 +1462,7 @@ static const struct dev_pm_ops nhi_pm_ops = {
>  					    * pci-tunnels stay alive.
>  					    */
>  	.thaw_noirq = nhi_thaw_noirq,
> -	.restore_noirq = nhi_resume_noirq,
> +	.restore_noirq = nhi_restore_noirq,
>  	.suspend = nhi_suspend,
>  	.poweroff_noirq = nhi_poweroff_noirq,
>  	.poweroff = nhi_suspend,
> -- 
> 2.43.0

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Mario Limonciello]

On 10/23/25 12:56 AM, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Oct 23, 2025 at 12:03:26AM -0500, Mario Limonciello (AMD) wrote:
>> When a machine is restored from S4 if the firmware CM has created
>> tunnels there can be an incongruity of expectation from the kernel
>> when compared to booting from S5.
>>
>> For example this incongruity can lead to problems like the display
>> not working properly if the firmware CM didn't turn it on pre-OS
>> but the OS had it on before entering S4.
>>
>> Add an extra restore_noirq() callback to explicitly reset the NHI
>> before running the rest of the resume sequence.  This will allow
>> the standard discovery and probing routines to run and create the
>> tunnels as expected.
>>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> Cc: Sanath.S@amd.com
>> ---
>>   drivers/thunderbolt/nhi.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
>> index 5f63f9b9cf405..042d184637a93 100644
>> --- a/drivers/thunderbolt/nhi.c
>> +++ b/drivers/thunderbolt/nhi.c
>> @@ -51,6 +51,8 @@ static bool host_reset = true;
>>   module_param(host_reset, bool, 0444);
>>   MODULE_PARM_DESC(host_reset, "reset USB4 host router (default: true)");
>>   
>> +static void nhi_reset(struct tb_nhi *nhi);
> 
> Please move the whole function above nhi_restore_noirq() then.

OK - wasn't sure that would be OK to do.

> 
>> +
>>   static int ring_interrupt_index(const struct tb_ring *ring)
>>   {
>>   	int bit = ring->hop;
>> @@ -1077,6 +1079,17 @@ static int nhi_resume_noirq(struct device *dev)
>>   	return tb_domain_resume_noirq(tb);
>>   }
>>   
>> +static int nhi_restore_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;
>> +
>> +	nhi_reset(nhi);
>> +
> 
> Now that we do this, should ge get rid of the S4 things we do in
> tb_resume_noirq()? Especially this block:
> 
> 	/*
> 	 * If we get here from suspend to disk the boot firmware or the
> 	 * restore kernel might have created tunnels of its own. Since
> 	 * we cannot be sure they are usable for us we find and tear
> 	 * them down.
> 	 */
> 	tb_switch_discover_tunnels(tb->root_switch, &tunnels, false);
> 	list_for_each_entry_safe_reverse(tunnel, n, &tunnels, list) {
> 		if (tb_tunnel_is_usb3(tunnel))
> 			usb3_delay = 500;
> 		tb_tunnel_deactivate(tunnel);
> 		tb_tunnel_put(tunnel);
> 	}
> 

Yeah; I agree this block should go now.

> Also have you checked that this works when you have tunnels such as P2P
> (networking)?

I didn't check this, I'll spin a v2 and get that checked before posting it.

> 
>> +	return nhi_resume_noirq(dev);
>> +}
>> +
>>   static int nhi_suspend(struct device *dev)
>>   {
>>   	struct pci_dev *pdev = to_pci_dev(dev);
>> @@ -1449,7 +1462,7 @@ static const struct dev_pm_ops nhi_pm_ops = {
>>   					    * pci-tunnels stay alive.
>>   					    */
>>   	.thaw_noirq = nhi_thaw_noirq,
>> -	.restore_noirq = nhi_resume_noirq,
>> +	.restore_noirq = nhi_restore_noirq,
>>   	.suspend = nhi_suspend,
>>   	.poweroff_noirq = nhi_poweroff_noirq,
>>   	.poweroff = nhi_suspend,
>> -- 
>> 2.43.0

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Kenneth Crudup]

On 10/23/25 07:13, Mario Limonciello wrote:

> I didn't check this, I'll spin a v2 and get that checked before posting it.

Any luck/update with a V2? (Is V1 close enough to try testing?)

I have a similar problem with my XPS 9320; if I don't wait an 
appropriate amount of time after coming out of suspend before plugging 
in a TB dock that wasn't the one I'd suspended with, my laptop still 
thinks it's plugged into the original TB dock (and things like DP 
monitor resolutions, etc. aren't adapted to the new environment) and I 
would like to help you test this out.

Thanks, Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Mario Limonciello]

On 11/4/25 1:11 PM, Kenneth Crudup wrote:
> 
> On 10/23/25 07:13, Mario Limonciello wrote:
> 
>> I didn't check this, I'll spin a v2 and get that checked before 
>> posting it.
> 
> Any luck/update with a V2? (Is V1 close enough to try testing?)
> 
> I have a similar problem with my XPS 9320; if I don't wait an 
> appropriate amount of time after coming out of suspend before plugging 
> in a TB dock that wasn't the one I'd suspended with, my laptop still 
> thinks it's plugged into the original TB dock (and things like DP 
> monitor resolutions, etc. aren't adapted to the new environment) and I 
> would like to help you test this out.
> 
> Thanks, Kenny
> 

V2 caused nonsensical testing failures that I haven't figured out yet so 
I didn't put it out on the list.

Try V1 and let me know how it goes!

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Kenneth Crudup]

On 11/4/25 11:12, Mario Limonciello wrote:

> V2 caused nonsensical testing failures that I haven't figured out yet so 
> I didn't put it out on the list.
> Try V1 and let me know how it goes!

Will do ... the transition from work to home (with totally different 
setups) is where I usually see this issue come up, so I'll know more 
tonight, but at least just a quick suspend-resume cycle RN hasn't blown 
anything up, so that's promising :)

-Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Mario Limonciello]

On 11/4/25 2:22 PM, Kenneth Crudup wrote:
> 
> On 11/4/25 11:12, Mario Limonciello wrote:
> 
>> V2 caused nonsensical testing failures that I haven't figured out yet 
>> so I didn't put it out on the list.
>> Try V1 and let me know how it goes!
> 
> Will do ... the transition from work to home (with totally different 
> setups) is where I usually see this issue come up, so I'll know more 
> tonight, but at least just a quick suspend-resume cycle RN hasn't blown 
> anything up, so that's promising :)
> 
> -Kenny
> 

Are you doing S4 at all?  Or only s2idle/s3?  The patch was specifically 
for resume from S4.

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Kenneth Crudup]

On 11/4/25 12:31, Mario Limonciello wrote:

> Are you doing S4 at all?  Or only s2idle/s3?  The patch was specifically 
> for resume from S4.

Ah, good point and I'll have to look out for that, but I do use "hybrid 
sleep", which (AIUI) starts a hibernation cycle but stays (in my case) 
in s0ix suspend until after 4h have passed.

-K

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Mario Limonciello]

On 11/4/25 3:19 PM, Kenneth Crudup wrote:
> 
> On 11/4/25 12:31, Mario Limonciello wrote:
> 
>> Are you doing S4 at all?  Or only s2idle/s3?  The patch was 
>> specifically for resume from S4.
> 
> Ah, good point and I'll have to look out for that, but I do use "hybrid 
> sleep", which (AIUI) starts a hibernation cycle but stays (in my case) 
> in s0ix suspend until after 4h have passed.
> 
> -K
> 

The behavior you describe is suspend-then-hibernate not hybrid sleep. 
Could you confirm which one you're using?

This patch could change suspend-then-hibernate (if you resumed from 
hibernate not suspend) but hybrid sleep would only help if the system 
lost power during suspend (like a dead battery).

For the purposes of testing this patch works it's probably easiest to do 
pure S4.

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Kenneth Crudup]

On 11/4/25 13:21, Mario Limonciello wrote:

> For the purposes of testing this patch works it's probably easiest to do pure S4.

Well, you were right ... the patch didn't (re-/properly) enumerate the 
devices when I'd switched TB docks before coming out of suspend ... :|

That being said, I do "suspend-then-hibernate" AND "hybrid sleep" (which 
I'm about to disable the former as I think that solves an unrelated 
issue I have with "double suspend").

So, could this patch be applied for the coming out of (s0ix) sleep case 
as well? (I swear that used to work properly until a few kernel revs ago 
....)

-Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: [PATCH] thunderbolt: Reset NHI during S4 restore_noirq() callback

[Mario Limonciello (AMD) (kernel.org)]

On 11/5/2025 1:06 PM, Kenneth Crudup wrote:
> 
> On 11/4/25 13:21, Mario Limonciello wrote:
> 
>> For the purposes of testing this patch works it's probably easiest to 
>> do pure S4.
> 
> Well, you were right ... the patch didn't (re-/properly) enumerate the 
> devices when I'd switched TB docks before coming out of suspend ... :|
> 
> That being said, I do "suspend-then-hibernate" AND "hybrid sleep" (which 
> I'm about to disable the former as I think that solves an unrelated 
> issue I have with "double suspend").

Yeah I would suggest either/or.

> 
> So, could this patch be applied for the coming out of (s0ix) sleep case 
> as well? (I swear that used to work properly until a few kernel revs 
> ago ....)
> 
> -Kenny
> 

Well the problems for suspend/resume and hibernate/resume are actually a 
bit different.

The hibernate/resume case is specifically because the system may have a 
Pre-OS CM that is running which had configured all the devices 
differently than the SW CM in Linux does.

For bootup we go through a reset to ensure Linux gets a chance to set up 
everything as intended.

This is /mostly/ for differences of opinions between FW and Linux for 
PCIe resource enumeration, but it also has implications for other tunnel 
types like DP.

So for hibernate/resume this patch aligns the policies for bootup and 
resume from hibernate.

For suspend, some tunnels are left active but not all of them.

DP tunnels are explicitly released through 
tb_disconnect_and_release_dp().  They come back up on resume plug events.

So if you're having problems unique to suspend/resume and DP tunnels 
there is either a problem with the handling of that hotplug event or a 
problem in the graphics driver most likely.


But I'm still wondering about pure S4.  Does it help there?