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

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

On Tue, May 12, 2026 at 03:58:34PM +0200, Konrad Dybcio wrote:
> 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

Got it. Right. Let's then keep it as is for now (e.g don't move it from
tb.c).