* Re: [PATCH v2 2/4] thunderbolt: Separate out common NHI bits
[not found] ` <20260504065402.GB6785@black.igk.intel.com>
@ 2026-05-12 13:06 ` Konrad Dybcio
2026-05-12 13:20 ` Mika Westerberg
0 siblings, 1 reply; 6+ messages in thread
From: Konrad Dybcio @ 2026-05-12 13:06 UTC (permalink / raw)
To: Mika Westerberg, Konrad Dybcio
Cc: Andreas Noever, Mika Westerberg, Yehezkel Bernat, linux-kernel,
linux-usb, usb4-upstream, Raghavendra Thoorpu
On 5/4/26 8:54 AM, Mika Westerberg wrote:
> Hi,
[...]
>> +void nhi_pci_shutdown(struct tb_nhi *nhi)
>
> Why these are not static?
[...]
>> +static const struct tb_nhi_ops pci_nhi_default_ops = {
>> + .pre_nvm_auth = nhi_pci_start_dma_port,
>> + .post_nvm_auth = nhi_pci_complete_dma_port,
>> + .request_ring_irq = nhi_pci_ring_request_msix,
>> + .release_ring_irq = nhi_pci_ring_release_msix,
>> + .shutdown = nhi_pci_shutdown,
>> + .is_present = nhi_pci_is_present,
>> + .init_interrupts = nhi_pci_init_msi,
>
> You populate them here so there is no need to expose outside of pci.c.
nhi_ops.c needs them too, as they were previously called
unconditionally for all NHI flavors
[...]
>> +/*
>> + * 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?)
(ack for all the remaining comments)
Konrad
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] thunderbolt: Separate out common NHI bits
2026-05-12 13:06 ` [PATCH v2 2/4] thunderbolt: Separate out common NHI bits Konrad Dybcio
@ 2026-05-12 13:20 ` Mika Westerberg
2026-05-12 13:43 ` Konrad Dybcio
0 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2026-05-12 13:20 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Konrad Dybcio, Andreas Noever, Mika Westerberg, Yehezkel Bernat,
linux-kernel, linux-usb, usb4-upstream, Raghavendra Thoorpu
On Tue, May 12, 2026 at 03:06:58PM +0200, Konrad Dybcio wrote:
> On 5/4/26 8:54 AM, Mika Westerberg wrote:
> > Hi,
>
> [...]
>
> >> +void nhi_pci_shutdown(struct tb_nhi *nhi)
> >
> > Why these are not static?
>
> [...]
>
> >> +static const struct tb_nhi_ops pci_nhi_default_ops = {
> >> + .pre_nvm_auth = nhi_pci_start_dma_port,
> >> + .post_nvm_auth = nhi_pci_complete_dma_port,
> >> + .request_ring_irq = nhi_pci_ring_request_msix,
> >> + .release_ring_irq = nhi_pci_ring_release_msix,
> >> + .shutdown = nhi_pci_shutdown,
> >> + .is_present = nhi_pci_is_present,
> >> + .init_interrupts = nhi_pci_init_msi,
> >
> > You populate them here so there is no need to expose outside of pci.c.
>
> nhi_ops.c needs them too, as they were previously called
> unconditionally for all NHI flavors
OK.
> [...]
>
>
> >> +/*
> >> + * 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] thunderbolt: Separate out common NHI bits
2026-05-12 13:20 ` Mika Westerberg
@ 2026-05-12 13:43 ` Konrad Dybcio
2026-05-12 13:54 ` Mika Westerberg
0 siblings, 1 reply; 6+ messages in thread
From: Konrad Dybcio @ 2026-05-12 13:43 UTC (permalink / raw)
To: Mika Westerberg
Cc: Konrad Dybcio, Andreas Noever, Mika Westerberg, Yehezkel Bernat,
linux-kernel, linux-usb, usb4-upstream, Raghavendra Thoorpu
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?
Konrad
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] thunderbolt: Separate out common NHI bits
2026-05-12 13:43 ` Konrad Dybcio
@ 2026-05-12 13:54 ` Mika Westerberg
2026-05-12 13:58 ` Konrad Dybcio
0 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2026-05-12 13:54 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Konrad Dybcio, Andreas Noever, Mika Westerberg, Yehezkel Bernat,
linux-kernel, linux-usb, usb4-upstream, Raghavendra Thoorpu
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 :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] thunderbolt: Separate out common NHI bits
2026-05-12 13:54 ` Mika Westerberg
@ 2026-05-12 13:58 ` Konrad Dybcio
2026-05-12 14:02 ` Mika Westerberg
0 siblings, 1 reply; 6+ messages in thread
From: Konrad Dybcio @ 2026-05-12 13:58 UTC (permalink / raw)
To: Mika Westerberg
Cc: Konrad Dybcio, Andreas Noever, Mika Westerberg, Yehezkel Bernat,
linux-kernel, linux-usb, usb4-upstream, Raghavendra Thoorpu
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] thunderbolt: Separate out common NHI bits
2026-05-12 13:58 ` Konrad Dybcio
@ 2026-05-12 14:02 ` Mika Westerberg
0 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2026-05-12 14:02 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Konrad Dybcio, Andreas Noever, Mika Westerberg, Yehezkel Bernat,
linux-kernel, linux-usb, usb4-upstream, Raghavendra Thoorpu
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).
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-05-12 14:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260428-topic-usb4_nonpcie_prepwork-v2-0-452fb9d63f77@oss.qualcomm.com>
[not found] ` <20260428-topic-usb4_nonpcie_prepwork-v2-2-452fb9d63f77@oss.qualcomm.com>
[not found] ` <20260504065402.GB6785@black.igk.intel.com>
2026-05-12 13:06 ` [PATCH v2 2/4] thunderbolt: Separate out common NHI bits Konrad Dybcio
2026-05-12 13:20 ` Mika Westerberg
2026-05-12 13:43 ` Konrad Dybcio
2026-05-12 13:54 ` Mika Westerberg
2026-05-12 13:58 ` Konrad Dybcio
2026-05-12 14:02 ` Mika Westerberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox