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

[Mario Limonciello <superm1@kernel.org>]

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