From 3baeae36039afc233d4a42d6ff4aa7019892619f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Fri, 29 Aug 2025 16:10:57 +0300 Subject: PCI: Use pci_release_resource() instead of release_resource() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A few places in setup-bus.c call release_resource() directly and end up duplicating functionality from pci_release_resource() such as parent check, logging, and clearing the resource. Worse yet, the way the resource is cleared is inconsistent between different sites. Convert release_resource() calls into pci_release_resource() to remove code duplication. This will also make the resource start, end, and flags behavior consistent, i.e., start address is cleared, and only IORESOURCE_UNSET is asserted for the resource. While at it, eliminate the unnecessary initialization of idx variable in pci_bridge_release_resources(). Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas Link: https://patch.msgid.link/20250829131113.36754-9-ilpo.jarvinen@linux.intel.com --- drivers/pci/setup-res.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'drivers/pci/setup-res.c') diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c index d2b3ed51e880..0468c058b598 100644 --- a/drivers/pci/setup-res.c +++ b/drivers/pci/setup-res.c @@ -406,20 +406,25 @@ int pci_reassign_resource(struct pci_dev *dev, int resno, return 0; } -void pci_release_resource(struct pci_dev *dev, int resno) +int pci_release_resource(struct pci_dev *dev, int resno) { struct resource *res = pci_resource_n(dev, resno); const char *res_name = pci_resource_name(dev, resno); + int ret; if (!res->parent) - return; + return 0; pci_info(dev, "%s %pR: releasing\n", res_name, res); - release_resource(res); + ret = release_resource(res); + if (ret) + return ret; res->end = resource_size(res) - 1; res->start = 0; res->flags |= IORESOURCE_UNSET; + + return 0; } EXPORT_SYMBOL(pci_release_resource); -- cgit v1.2.3 From 1cdffa51ecc4ee02c46160bafa2e5d25d09e97ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Fri, 29 Aug 2025 16:10:58 +0300 Subject: PCI: Enable bridge even if bridge window fails to assign MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A normal PCI bridge has multiple bridge windows and not all of them are always required by devices underneath the bridge. If a Root Port or bridge does not have a device underneath, no bridge windows get assigned. Yet, pci_enable_resources() is set to fail indiscriminantly on any resource assignment failure if the resource is not known to be optional. In practice, the code in pci_enable_resources() is currently largely dormant. The kernel sets resource flags to zero for any unused bridge window and resets flags to zero in case of an resource assignment failure, which short-circuits pci_enable_resources() because of this check: if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM))) continue; However, an upcoming change to resource flags will alter how bridge window resource flags behave activating these long dormants checks in pci_enable_resources(). While complex logic could be built to selectively enable a bridge only under some conditions, a few versions of such logic were tried during development of this change and none of them worked satisfactorily. Thus, I just gave up and decided to enable any bridge regardless of the bridge windows as there seems to be no clear benefit from not enabling it, but a major downside as pcieport will not be probed for the bridge if it's not enabled. Therefore, change pci_enable_resources() to not check if bridge window resources remain unassigned. Resource assignment failures are pretty noisy already so there is no need to log that for bridge windows in pci_enable_resources(). Ignoring bridge window failures hopefully prevents an obvious source of regressions when the upcoming change that no longer clears resource flags for bridge windows is enacted. I've hit this problem even during my own testing on multiple occasions so I expect it to be a quite common problem. This can always be revisited later if somebody thinks the enable check for bridges is not strict enough, but expect a mind-boggling number of regressions from such a change. Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas Link: https://patch.msgid.link/20250829131113.36754-10-ilpo.jarvinen@linux.intel.com --- drivers/pci/setup-res.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) (limited to 'drivers/pci/setup-res.c') diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c index 0468c058b598..4e0e60256f04 100644 --- a/drivers/pci/setup-res.c +++ b/drivers/pci/setup-res.c @@ -527,22 +527,26 @@ int pci_enable_resources(struct pci_dev *dev, int mask) if (pci_resource_is_optional(dev, i)) continue; - if (r->flags & IORESOURCE_UNSET) { - pci_err(dev, "%s %pR: not assigned; can't enable device\n", - r_name, r); - return -EINVAL; + if (i < PCI_BRIDGE_RESOURCES) { + if (r->flags & IORESOURCE_UNSET) { + pci_err(dev, "%s %pR: not assigned; can't enable device\n", + r_name, r); + return -EINVAL; + } + + if (!r->parent) { + pci_err(dev, "%s %pR: not claimed; can't enable device\n", + r_name, r); + return -EINVAL; + } } - if (!r->parent) { - pci_err(dev, "%s %pR: not claimed; can't enable device\n", - r_name, r); - return -EINVAL; + if (r->parent) { + if (r->flags & IORESOURCE_IO) + cmd |= PCI_COMMAND_IO; + if (r->flags & IORESOURCE_MEM) + cmd |= PCI_COMMAND_MEMORY; } - - if (r->flags & IORESOURCE_IO) - cmd |= PCI_COMMAND_IO; - if (r->flags & IORESOURCE_MEM) - cmd |= PCI_COMMAND_MEMORY; } if (cmd != old_cmd) { -- cgit v1.2.3 From 8278c6914306f35f32d73bdf2a918950919a0051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Fri, 29 Aug 2025 16:10:59 +0300 Subject: PCI: Preserve bridge window resource type flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a bridge window is found unused or fails to assign, the flags of the associated resource are cleared. Clearing flags is problematic as it also removes the type information of the resource which is needed later. Thus, always preserve the bridge window type flags and use IORESOURCE_UNSET and IORESOURCE_DISABLED to indicate the status of the bridge window. Also, when initializing resources, make sure all valid bridge windows do get their type flags set. Change various places that relied on resource flags being cleared to check for IORESOURCE_UNSET and IORESOURCE_DISABLED to allow bridge window resource to retain their type flags. Add pdev_resource_assignable() and pdev_resource_should_fit() helpers to filter out disabled bridge windows during resource fitting; the latter combines more common checks into the helper. When reading the bridge windows from the registers, instead of leaving the resource flags cleared for bridge windows that are not enabled, always set up the flags and set IORESOURCE_UNSET | IORESOURCE_DISABLED as needed. When resource fitting or assignment fails for a bridge window resource, or the bridge window is not needed, mark the resource with IORESOURCE_UNSET or IORESOURCE_DISABLED, respectively. Use dummy zero resource in resource_show() for backwards compatibility as lspci will otherwise misrepresent disabled bridge windows. This change fixes an issue which highlights the importance of keeping the resource type flags intact: At the end of __assign_resources_sorted(), reset_resource() is called, previously clearing the flags. Later, pci_prepare_next_assign_round() attempted to release bridge resources using pci_bus_release_bridge_resources() that calls into pci_bridge_release_resources() that assumes type flags are still present. As type flags were cleared, IORESOURCE_MEM_64 was not set leading to resources under an incorrect bridge window to be released (idx = 1 instead of idx = 2). While the assignments performed later covered this problem so that the wrongly released resources got assigned in the end, it was still causing extra release+assign pairs. There are other reasons why the resource flags should be retained in upcoming changes too. Removing the flag reset for non-bridge window resource is left as future work, in part because it has a much higher regression potential due to pci_enable_resources() that will start to work also for those resources then and due to what endpoint drivers might assume about resources. Despite the Fixes tag, backporting this (at least any time soon) is highly discouraged. The issue fixed is borderline cosmetic as the later assignments normally cover the problem entirely. Also there might be non-obvious dependencies. Fixes: 5b28541552ef ("PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources") Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas Link: https://patch.msgid.link/20250829131113.36754-11-ilpo.jarvinen@linux.intel.com --- drivers/pci/setup-res.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/pci/setup-res.c') diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c index 4e0e60256f04..21f77e5c647c 100644 --- a/drivers/pci/setup-res.c +++ b/drivers/pci/setup-res.c @@ -359,6 +359,9 @@ int pci_assign_resource(struct pci_dev *dev, int resno) res->flags &= ~IORESOURCE_UNSET; res->flags &= ~IORESOURCE_STARTALIGN; + if (resno >= PCI_BRIDGE_RESOURCES && resno <= PCI_BRIDGE_RESOURCE_END) + res->flags &= ~IORESOURCE_DISABLED; + pci_info(dev, "%s %pR: assigned\n", res_name, res); if (resno < PCI_BRIDGE_RESOURCES) pci_update_resource(dev, resno); -- cgit v1.2.3 From 3ab10f83e277ba9640742cbba67b8df369591450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Fri, 29 Aug 2025 16:11:02 +0300 Subject: PCI: Fix finding bridge window in pci_reassign_bridge_resources() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pci_reassign_bridge_resources() walks upwards in the PCI bus hierarchy, locates the relevant bridge window on each level using flags check, and attempts to release the bridge window. The flags-based check is fragile due to various fallbacks in the bridge window selection logic. As such, the algorithm might not locate the correct bridge window. Refactor pci_reassign_bridge_resources() to determine the correct bridge window using pbus_select_window(), which contains logic to handle all fallback cases correctly. Change function prefix to pbus as it now inputs struct bus and resource for which to locate the bridge window. The main purpose is to make bridge window selection logic consistent across the entire PCI core (one step at a time). While this technically also fixes the commit 8bb705e3e79d ("PCI: Add pci_resize_resource() for resizing BARs") making the bridge window walk algorithm more robust, the normal setup having a 64-bit resizable BAR underneath bridge(s) with 64-bit prefetchable windows does not need to use any fallbacks. As such, the practical impact is low (requiring BAR resize use case and a non-typical bridge device). The way to detect if unrelated resource failed again is left to use the type based approximation which should not behave worse than before. Fixes: 8bb705e3e79d ("PCI: Add pci_resize_resource() for resizing BARs") Signed-off-by: Ilpo Järvinen Signed-off-by: Bjorn Helgaas Link: https://patch.msgid.link/20250829131113.36754-14-ilpo.jarvinen@linux.intel.com --- drivers/pci/setup-res.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/pci/setup-res.c') diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c index 21f77e5c647c..c3ba4ccecd43 100644 --- a/drivers/pci/setup-res.c +++ b/drivers/pci/setup-res.c @@ -496,7 +496,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size) /* Check if the new config works by trying to assign everything. */ if (dev->bus->self) { - ret = pci_reassign_bridge_resources(dev->bus->self, res->flags); + ret = pbus_reassign_bridge_resources(dev->bus, res); if (ret) goto error_resize; } -- cgit v1.2.3