Re: [PATCH v2 02/12] PCI: Disable pcie port on specific amd devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] PCI: no_pcie_port leaves D3cold bridges exposed in suspend and
  sysfs
- [Medium] x86/PCI: quirk_disable_pcie_port() disables pcieport on
  Thunderbolt eGPUs
--

commit ceffb6c4c39f628d202fdc8e2a5f341d822f54ff
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

PCI: Disable pcie port on specific amd devices

This commit adds a no_pcie_port flag to struct pci_dev and sets it via an
early PCI fixup on the upstream bridges of certain AMD GPUs on Apple
MacBookPro models so pcie_portdrv does not bind to them.

> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 795d81c7a4de4..b1697cc4aed56 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -1094,6 +1094,48 @@ static void asus_disable_nvme_d3cold(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x9a09, asus_disable_nvme_d3cold);
>  
> +static const struct dmi_system_id quirk_disable_pcie_port_dmi_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,3"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
> +		},
> +	},
> +	{}
> +};
> +
> +static void quirk_disable_pcie_port(struct pci_dev *pdev)
> +{
> +	struct pci_dev *upstream = pdev;
> +
> +	if (dmi_check_system(quirk_disable_pcie_port_dmi_table)) {
> +		while ((upstream = pci_upstream_bridge(upstream)))
> +			upstream->no_pcie_port = 1;
> +	}
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x67ef, quirk_disable_pcie_port);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x69af, quirk_disable_pcie_port);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7340, quirk_disable_pcie_port);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7360, quirk_disable_pcie_port);

[Severity: Medium]
Can quirk_disable_pcie_port() also disable pcieport on a Thunderbolt
hierarchy when an external GPU with one of these retail AMD PCI device IDs
(such as Radeon RX 460/560 0x67ef or Radeon RX 5500 XT 0x7340) is attached?

Walking every upstream bridge without checking
!pci_is_thunderbolt_attached(pdev) sets no_pcie_port = 1 on the Thunderbolt
Root Port and switch bridges, breaking pciehp, PME, AER, and bridge runtime
D3 across that hierarchy.

> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index a9cbfc1d2bc75..d2f19e5666022 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -688,7 +688,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  	int type = pci_pcie_type(dev);
>  	int status;
>  
> -	if (!pci_is_pcie(dev) ||
> +	if (!pci_is_pcie(dev) || dev->no_pcie_port ||
>  	    ((type != PCI_EXP_TYPE_ROOT_PORT) &&
>  	     (type != PCI_EXP_TYPE_UPSTREAM) &&
>  	     (type != PCI_EXP_TYPE_DOWNSTREAM) &&

[Severity: High]
Does returning -ENODEV here leave the upstream bridges exposed to config
space accesses while powered off by apple-gmux via PWRD(1)?

Without pcie_portdrv bound, each upstream bridge has dev->driver == NULL
(so pm == NULL) and remains in RPM_ACTIVE with runtime PM forbidden.  When
apple-gmux powers off the discrete GPU in gmux_set_discrete_state(), it
saves state, sets pdev->current_state = PCI_D3cold on the bridges, and cuts
power via PWRD(1).

During system suspend while the bridges are off, pci_pm_prepare() sees
!pm_runtime_suspended(dev) and skips direct_complete, and then
pci_pm_suspend_noirq() accesses the dead bridge directly:

drivers/pci/pci-driver.c:pci_pm_suspend_noirq() {
    ...
	if (!pm) {
		pci_save_state(pci_dev);
		goto set_unknown;
	}
    ...
}

pci_pm_resume_noirq() similarly accesses config space and PCIe capabilities
on the dead bridges before gmux_resume() restores power.

At runtime, any userspace read of /sys/bus/pci/devices/.../config (such as
lspci) calls pci_read_config() -> pci_config_pm_runtime_get(), where
pm_runtime_resume(dev) is a no-op because the unbound bridge is still in
RPM_ACTIVE, so pci_user_read_config_*() reads directly from the powered-off
bridge and triggers a CPU CATERR.

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