Re: [PATCH v2 2/4] thunderbolt: Separate out common NHI bits

[Konrad Dybcio]

On 5/12/26 3:54 PM, Mika Westerberg wrote:
> On Tue, May 12, 2026 at 03:43:12PM +0200, Konrad Dybcio wrote:
>> On 5/12/26 3:20 PM, Mika Westerberg wrote:
>>> On Tue, May 12, 2026 at 03:06:58PM +0200, Konrad Dybcio wrote:
>>>> On 5/4/26 8:54 AM, Mika Westerberg wrote:
>>>>> Hi,
>>
>> [...]
>>
>>>>>> +/*
>>>>>> + * During suspend the Thunderbolt controller is reset and all PCIe
>>>>>> + * tunnels are lost. The NHI driver will try to reestablish all tunnels
>>>>>> + * during resume. This adds device links between the tunneled PCIe
>>>>>> + * downstream ports and the NHI so that the device core will make sure
>>>>>> + * NHI is resumed first before the rest.
>>>>>> + */
>>>>>> +bool tb_apple_add_links(struct tb_nhi *nhi)
>>>>>
>>>>> Okay you moved it here good. I think we can call it in nhi_pci_probe()
>>>>> directly so no need to expose outside.
>>>>
>>>> Yeah that seems like a good idea. It's already there, behind N calls
>>>> in the software CM case.
>>>>
>>>> Do we have to check the CM type though, or do you think it'd be fine
>>>> to just call it unconditionally? (either because there are presumably
>>>> no Apple machines with ICM or because these devlinks would be harmless?)
>>>
>>> I think you can call it unconditionally. It only does something for TB1-2
>>> Apple systems.
>>>
>>> For Apple TB3 we used to start ICM firmware but this was changed as the
>>> driver learned SW CM. However, we never setup any device links so this
>>> would not change anything.
>>
>> OK. I'm keeping tb_acpi_add_link() as-is, since that's both bus- and
>> arch-independent.
>>
>> However, doing just something like:
>>
>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
>> index cb5d028de3bc..f5ddc8ddb8bb 100644
>> --- a/drivers/thunderbolt/tb.c
>> +++ b/drivers/thunderbolt/tb.c
>> @@ -3327,7 +3327,7 @@ struct tb *tb_probe(struct tb_nhi *nhi)
>>          * before the PCIe/USB stack is resumed so complain here if we
>>          * found them missing.
>>          */
>> -       if (!tb_apple_add_links(nhi) && !tb_acpi_add_links(nhi))
>> +       if (!tb_acpi_add_links(nhi))
>>                 tb_warn(tb, "device links to tunneled native ports are missing!\n");
>>
>>
>> diff --git a/drivers/thunderbolt/pci.c b/drivers/thunderbolt/pci.c
>> index ca50e3584cac..e0abd1d503c5 100644
>> --- a/drivers/thunderbolt/pci.c
>> +++ b/drivers/thunderbolt/pci.c
>> @@ -294,6 +294,8 @@ static int nhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>  
>>         pci_set_master(pdev);
>>  
>> +       tb_apple_add_links(nhi)
>> +
>>         return nhi_probe(&nhi_pci->nhi);
>>  }
>>
>>
>> Will cause the warning to show up. And adding something like
>> `nhi->device_links_done` is a little ugly.. Ideas?
> 
> Ah in Qualcomm case? We are going to add tb_of_add_links() as well, right (or
> something along thoese lines)? Then tb.c does:
> 
>        if (!tb_acpi_add_links(nhi) && !tb_of_add_links(nhi))
>                  tb_warn(tb, "device links to tunneled native ports are missing!\n");
> 
> In the meantime it is okay to have that warn because we really do want to
> have those links in place :)

No no, I meant that with the diff above, tb_apple_add_links() failing
would not lead to any warning messages, and it would always hit the
warning in tb.c

(because these two are now checked independently)

Konrad