Re: [PATCH v3 2/2] thunderbolt: Add device links for Apple T2 NHI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Unconditional early return bypasses power management logic for older Thunderbolt PCIe add-in cards on T2 Macs.
- [Medium] Suboptimal power management coupling due to N-to-M device linking on systems with multiple Thunderbolt controllers.
- [Low] Build failure due to implicit function declaration of `acpi_device_bid` when CONFIG_ACPI is disabled.

Pre-existing issues:
- [High] Missing `pci_bus_sem` lock during `for_each_pci_bridge` traversal can cause list corruption or use-after-free.
--

--- Patch [2]: [PATCH v3 2/2] thunderbolt: Add device links for Apple T2 NHI ---
commit 177c208d72f3f6296c38f42a04ec5607baa927af
Author: Andre Eikmeyer <andre@negmaster.com>

thunderbolt: Add device links for Apple T2 NHI
    
This commit adds support for Thunderbolt NHI on T2 Macs by finding ACPI root
ports named TRP0, TRP1, etc. It creates device links back to the NHI so that
PCIe tunnels can be re-established after sleep.

> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index c69c323e6952a..0cdffcf577cdc 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c

[ ... ]

> @@ -3305,11 +3306,59 @@ static const struct tb_cm_ops tb_cm_ops = {
>  static bool tb_apple_add_links(struct tb_nhi *nhi)
>  {
>  	struct pci_dev *upstream, *pdev;
> -	bool ret;
> +	bool ret = false;
>  
>  	if (!x86_apple_machine)
>  		return false;
>  
> +	/* On T2 Macs. the root ports are stored in ACPI as TRP0,
> +	 * TRP1, etc. Find them and create device links
> +	 * so that PCIe tunnels can be re-established after
> +	 * sleep.
> +	 */
> +	if (has_apple_t2_chip && IS_ENABLED(CONFIG_ACPI)) {
> +		struct acpi_device *adev;
> +		unsigned int slot, func;
> +		const struct device_link *link;
> +		const char *bid;
> +
> +		for (slot = 0; slot < 32; slot++) {
> +			for (func = 0; func < 8; func++) {
> +				pdev = pci_get_slot(nhi->pdev->bus, PCI_DEVFN(slot, func));

[Severity: Medium]
Since all NHIs and TRP ports reside on the same root complex bus on T2
Macs, does this loop link every TRP port on the bus to the current NHI
indiscriminately?

On systems with multiple NHIs, could this cause all Thunderbolt controllers
to be kept awake if any single Thunderbolt port is active, preventing
independent power management?

> +				if (!pdev)
> +					continue;
> +
> +				if (!pci_is_pcie(pdev) || pci_pcie_type(pdev) !=
> +						PCI_EXP_TYPE_ROOT_PORT)
> +					goto put_pdev;
> +
> +				adev = ACPI_COMPANION(&pdev->dev);
> +				if (!adev)
> +					goto put_pdev;
> +
> +				bid = acpi_device_bid(adev);

[Severity: Low]
Because IS_ENABLED(CONFIG_ACPI) is evaluated at runtime, the compiler
will still parse this block when CONFIG_ACPI is disabled.

Since acpi_device_bid() is not stubbed in the ACPI headers when CONFIG_ACPI
is disabled, does this cause a compile-time implicit function declaration
error?

[ ... ]

> +put_pdev:
> +	pci_dev_put(pdev);
> +			}
> +		}
> +		return ret;
> +	}
> +
>  	switch (nhi->pdev->device) {

[Severity: High]
Does this unconditional return bypass the legacy fallback logic below for
older Thunderbolt controllers installed in T2 Macs (like a 2019 Mac Pro)?

If the function returns here, power management device links might not be
created for non-T2 add-in cards like Falcon Ridge, preventing them from
waking up from sleep.

[ ... ]

>  	/*
>  	 * For each hotplug downstream port, create add device link
>  	 * back to NHI so that PCIe tunnels can be re-established after
>  	 * sleep.
>  	 */
>  	ret = false;
>  	for_each_pci_bridge(pdev, upstream->subordinate) {

[Severity: High]
This is a pre-existing issue, but is it safe to iterate over the PCI bus
devices list using for_each_pci_bridge() locklessly?

If a PCI hotplug event occurs on the upstream bridge's subordinate bus
during probe, could this lead to a use-after-free or list corruption?

Should this block be protected by down_read(&pci_bus_sem) or use
pci_walk_bus() instead?

>  		const struct device_link *link;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721063412.11588-1-atharvatiwarilinuxdev@gmail.com?part=2