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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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