From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 018E3606A31A for ; Sun, 25 Oct 2026 04:35:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=izPMe62p Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6677710E6A2; Sun, 25 Oct 2026 04:34:56 +0000 (UTC) Received: from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3DE7310E6A2 for ; Sun, 25 Oct 2026 04:34:50 +0000 (UTC) Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id 0FCDE41797; Sun, 25 Oct 2026 04:34:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C16691F031E1; Sun, 25 Oct 2026 04:34:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1792902889; bh=DMQlGLYZoKdFM2vxiqXaAYc27Y9+cmHiFyh5SnGpFGc=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=izPMe62pYjcVUxGydwPkbZMxOwxywm+6lsw0q3OqQpuqq5uJx2U8WxhzWKFufys1l iiGMFo60KqPYTXGd6b91dUHW+3GfjcIj386kQIBOOG6+A3GCNNC7RrKyVcGADt6sQ3 JTvTwgVONIWSQNEqQqpAP0WjlxHxPZz27gOlp5Pa5V8yRRFKSVjBBbaaUm/tp/jt8O UOOV5NAQTeNv9POfSwD8ACu0qCuB24r8RR3aox+UlGIqhUrxYfP1zuy0YO/02SJeRb mO/Fsgx8TYe/ZZuUiw+kehDeE34IsepZj1vITJRjmEcrJ64LxMLNAOHPh2RPJwbUCf Tel+c0BL6JtXQ== Message-ID: From: sashiko-bot@kernel.org Subject: Re: [PATCH 02/13] PCI: Disable pcie port and access on D3 on specific amd devices To: "Atharva Tiwari" Cc: =?utf-8?b?b3BlbiBsaXN0OlBDSSBTVUJTWVNURU0=?= , =?utf-8?b?b3BlbiBsaXN0OkRSTSBEUklWRVJT?= In-Reply-To: <20261025040046.1602-3-atharvatiwarilinuxdev@gmail.com> References: <20261025040046.1602-1-atharvatiwarilinuxdev@gmail.com> <20261025040046.1602-3-atharvatiwarilinuxdev@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Sun, 25 Oct 2026 04:34:49 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: sashiko-reviews@lists.linux.dev Sender: "dri-devel" X-Spam-Status: No, score=-11.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=4.0.1 X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on aws-us-west-2-korg-listserv-alma10-1 X-Archived-By: Thank you for your contribution! Sashiko AI review found 3 potential issue(= s) to consider: - [Critical] PCI: uninitialized *val in pci_read_config_*() when ignoring D= 3cold access - [Critical] PCI: pci_user_read_config_*() and cap walks bypass pci_dev_ign= ore_access() - [High] PCI: missing 0x7360 fixup in quirk_disable_pcie_port() for MacBook= Pro16,4 -- commit f3d5ededdfe7b0bad0fd200b82a9eb2e3e834ed0 Author: Atharva Tiwari PCI: Disable pcie port and access on D3 on specific amd devices This commit adds an early PCI fixup for Apple MacBookPro16,1 and MacBookPro16,4 systems to disable the PCIe port driver and ignore config space accesses on upstream bridges of the discrete GPU when those bridges are in D3cold. > diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c > index 795d81c7a4de4..66829941e8fe2 100644 > --- a/arch/x86/pci/fixup.c > +++ b/arch/x86/pci/fixup.c > @@ -1094,6 +1094,35 @@ static void asus_disable_nvme_d3cold(struct pci_de= v *pdev) > } > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x9a09, asus_disable_nvme_d= 3cold); > =20 > +static const struct dmi_system_id quirk_disable_pcie_port_dmi_table[] = =3D { > + { > + .matches =3D { > + DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."), > + DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"), > + }, > + }, > + { > + .matches =3D { > + 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 =3D pdev; > + > + if (dmi_check_system(quirk_disable_pcie_port_dmi_table)) { > + while ((upstream =3D pci_upstream_bridge(upstream))) { > + upstream->no_pcie_port =3D 1; > + upstream->ignore_access_d3 =3D 1; > + } > + } > +} > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7340, quirk_disable_pcie_po= rt); [Severity: High] While quirk_disable_pcie_port_dmi_table in arch/x86/pci/fixup.c matches both MacBookPro16,1 and MacBookPro16,4, DECLARE_PCI_FIXUP_EARLY is only register= ed for PCI device ID 0x7340 (Navi 14, used in MacBookPro16,1). MacBookPro16,4 uses the AMD Radeon Pro 5600M (Navi 12, PCI device ID 0x7360= ). Without a matching fixup registration for 0x7360, will quirk_disable_pcie_port() ever run on MacBookPro16,4, or will its upstream bridges remain unprotected when apple-gmux powers them off? [ ... ] > diff --git a/drivers/pci/access.c b/drivers/pci/access.c > index b123da16b63ba..5e95130671767 100644 > --- a/drivers/pci/access.c > +++ b/drivers/pci/access.c > @@ -559,6 +559,9 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword); > =20 > int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val) > { > + if (pci_dev_ignore_access(dev)) > + return PCIBIOS_SUCCESSFUL; > + > if (pci_dev_is_disconnected(dev)) { > PCI_SET_ERROR_RESPONSE(val); > return PCIBIOS_DEVICE_NOT_FOUND; > @@ -569,6 +572,9 @@ EXPORT_SYMBOL(pci_read_config_byte); > =20 > int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val) > { > + if (pci_dev_ignore_access(dev)) > + return PCIBIOS_SUCCESSFUL; > + > if (pci_dev_is_disconnected(dev)) { > PCI_SET_ERROR_RESPONSE(val); > return PCIBIOS_DEVICE_NOT_FOUND; > @@ -580,6 +586,9 @@ EXPORT_SYMBOL(pci_read_config_word); > int pci_read_config_dword(const struct pci_dev *dev, int where, > u32 *val) > { > + if (pci_dev_ignore_access(dev)) > + return PCIBIOS_SUCCESSFUL; > + > if (pci_dev_is_disconnected(dev)) { > PCI_SET_ERROR_RESPONSE(val); > return PCIBIOS_DEVICE_NOT_FOUND; [Severity: Critical] When pci_dev_ignore_access(dev) is true for a bridge in PCI_D3cold, pci_read_config_byte(), pci_read_config_word(), and pci_read_config_dword() in drivers/pci/access.c return PCIBIOS_SUCCESSFUL (0) without initializing *val. Many callers pass an uninitialized stack variable and rely on either a non-zero return value or PCI_SET_ERROR_RESPONSE(val) (0xff / 0xffff / 0xffffffff) when the device cannot be read. For example, in drivers/pci/pci-sysfs.c, secondary_bus_number_show() and subordinate_bus_number_show() read world-readable sysfs attributes into uninitialized stack variables: drivers/pci/pci-sysfs.c:secondary_bus_number_show() { struct pci_dev *pci_dev =3D to_pci_dev(dev); u8 sec_bus; int err; pci_config_pm_runtime_get(pci_dev); err =3D pci_read_config_byte(pci_dev, PCI_SECONDARY_BUS, &sec_bus); pci_config_pm_runtime_put(pci_dev); if (err) return -EINVAL; return sysfs_emit(buf, "%u\n", sec_bus); } In addition, pci_update_current_state() and pci_power_up() in drivers/pci/pci.c read PCI_PM_CTRL into an uninitialized stack variable pmcsr and check PCI_POSSIBLE_ERROR(pmcsr): drivers/pci/pci.c:pci_update_current_state() { ... } else if (dev->pm_cap) { u16 pmcsr; pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr); if (PCI_POSSIBLE_ERROR(pmcsr)) { dev->current_state =3D PCI_D3cold; return; } dev->current_state =3D pmcsr & PCI_PM_CTRL_STATE_MASK; } } If pmcsr happens not to be 0xffff on the stack, dev->current_state is overwritten with D0..D3hot (or set to PCI_D0 in pci_power_up()), which disarms pci_dev_ignore_access(dev) while the bridge is still powered off. Similarly, during suspend on the driverless bridge, pci_pm_suspend_noirq() -> pci_save_state() -> pci_save_pcie_state() -> pcie_capability_read_word() will see PCIBIOS_SUCCESSFUL with *val =3D=3D 0 = and overwrite the bridge's saved PCIe capability state with zeros. Should pci_read_config_byte(), pci_read_config_word(), and pci_read_config_dword() call PCI_SET_ERROR_RESPONSE(val) and return an error such as PCIBIOS_DEVICE_NOT_FOUND (matching pci_dev_is_disconnected(dev)), or otherwise ensure *val is never left uninitialized? > @@ -590,6 +599,9 @@ EXPORT_SYMBOL(pci_read_config_dword); > =20 > int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val) > { > + if (pci_dev_ignore_access(dev)) > + return PCIBIOS_SUCCESSFUL; > + > if (pci_dev_is_disconnected(dev)) > return PCIBIOS_DEVICE_NOT_FOUND; > return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val); > @@ -598,6 +610,9 @@ EXPORT_SYMBOL(pci_write_config_byte); > =20 > int pci_write_config_word(const struct pci_dev *dev, int where, u16 val) > { > + if (pci_dev_ignore_access(dev)) > + return PCIBIOS_SUCCESSFUL; > + > if (pci_dev_is_disconnected(dev)) > return PCIBIOS_DEVICE_NOT_FOUND; > return pci_bus_write_config_word(dev->bus, dev->devfn, where, val); > @@ -607,6 +622,9 @@ EXPORT_SYMBOL(pci_write_config_word); > int pci_write_config_dword(const struct pci_dev *dev, int where, > u32 val) > { > + if (pci_dev_ignore_access(dev)) > + return PCIBIOS_SUCCESSFUL; > + > if (pci_dev_is_disconnected(dev)) > return PCIBIOS_DEVICE_NOT_FOUND; > return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val); [Severity: Critical] Does restricting only pci_read_config_*() and pci_write_config_*() in drivers/pci/access.c leave other config space access paths unprotected when the bridge is in PCI_D3cold? Userspace config reads and writes via sysfs (pci_read_config() and pci_write_config() in drivers/pci/pci-sysfs.c) and procfs (drivers/pci/proc= .c) call pci_user_read_config_*() and pci_user_write_config_*(), which are generated by PCI_USER_READ_CONFIG and PCI_USER_WRITE_CONFIG in drivers/pci/access.c: drivers/pci/access.c:PCI_USER_READ_CONFIG() { ... raw_spin_lock_irq(&pci_lock); if (unlikely(dev->block_cfg_access)) pci_wait_cfg(dev); ret =3D dev->bus->ops->read(dev->bus, dev->devfn, pos, sizeof(type), &data); raw_spin_unlock_irq(&pci_lock); ... } Because dev->no_pcie_port =3D 1 prevents pcie_portdrv_probe() from binding = to the upstream bridges, runtime PM is not enabled on them and pci_config_pm_runtime_get() in pci_read_config() is a no-op. Any unprivileg= ed process (such as lspci or udev) reading /sys/bus/pci/devices/.../config whi= le the bridge is powered off in PCI_D3cold will invoke dev->bus->ops->read() directly on the dead bridge. In addition, capability walks in drivers/pci/pci.c (pci_find_capability() -> __pci_bus_find_cap_start(), pci_find_next_capability(), and pci_find_next_ext_capability()) call pci_bus_read_config_*() directly rather than pci_read_config_*(). Because the bridge has no bound driver (pm =3D=3D= NULL), during system suspend pci_pm_suspend_noirq() in drivers/pci/pci-driver.c runs: pci_pm_suspend_noirq() -> pci_save_state() -> pci_save_pcix_state() -> pci_find_capability(dev, PCI_CAP_ID_PCIX) -> __pci_bus_find_cap_start() -> pci_bus_read_config_word(bus, devfn, PCI_STATUS, &status) Could these paths still access the powered-off bridge and trigger a hardware crash unless PCI_USER_READ_CONFIG, PCI_USER_WRITE_CONFIG, and the capability lookup paths (or a lower layer) also check pci_dev_ignore_access(dev)? --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261025040046.1602= -1-atharvatiwarilinuxdev@gmail.com?part=3D2