thunderbolt: Reset NHI during S4 restore_noirq() callback

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) <[email protected]>

---
Cc: [email protected]

---
 drivers/thunderbolt/nhi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

drivers/thunderbolt/nhi.c +0 -0

index 5f63f9b9cf405..042d184637a93 100644

drivers/thunderbolt/nhi.c +1 -0

+
 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);

drivers/thunderbolt/nhi.c +4 -1

+	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,

Signature · 1 line

2.43.0

Hi,

Quoted 28 lines · 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) <[email protected]>

---
Cc: [email protected]

---
  drivers/thunderbolt/nhi.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

drivers/thunderbolt/nhi.c +0 -0

index 5f63f9b9cf405..042d184637a93 100644
@@ -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)");
 

drivers/thunderbolt/nhi.c static bool host_reset = true; · line 54

Quoted 1 line

drivers/thunderbolt/nhi.c +1 -0

+static void nhi_reset(struct tb_nhi *nhi);

Please move the whole function above nhi_restore_noirq() then.

Quoted 7 lines

drivers/thunderbolt/nhi.c +1 -0

+
  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);

drivers/thunderbolt/nhi.c static int nhi_resume_noirq(struct device *dev) · line 1089

Quoted 10 lines

drivers/thunderbolt/nhi.c +8 -0

  }
 
+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)?

Quoted 18 lines

drivers/thunderbolt/nhi.c +4 -1

+	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,

Signature · 1 line

2.43.0

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

Quoted 1 line · Mario Limonciello

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!

Quoted 2 lines · Mario Limonciello

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.

Quoted 1 line · Mario Limonciello

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

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

Quoted 1 line · Mario Limonciello

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