On Thu, 25 Sep 2025, Bjorn Helgaas wrote: > On Wed, Sep 24, 2025 at 04:42:28PM +0300, Ilpo Järvinen wrote: > > PNP resources are checked for conflicts with the other resource in the > > system by quirk_system_pci_resources() that walks through all PCI > > resources. quirk_system_pci_resources() correctly filters out resource > > with IORESOURCE_UNSET. > > > > Resources that do not reside within their bridge window, however, are > > not properly initialized with IORESOURCE_UNSET resulting in bogus > > conflicts detected in quirk_system_pci_resources(): > > > > pci 0000:00:02.0: VF BAR 2 [mem 0x00000000-0x1fffffff 64bit pref] > > pci 0000:00:02.0: VF BAR 2 [mem 0x00000000-0xdfffffff 64bit pref]: contains BAR 2 for 7 VFs > > ... > > pci 0000:03:00.0: VF BAR 2 [mem 0x00000000-0x1ffffffff 64bit pref] > > pci 0000:03:00.0: VF BAR 2 [mem 0x00000000-0x3dffffffff 64bit pref]: contains BAR 2 for 31 VFs > > ... > > pnp 00:04: disabling [mem 0xfc000000-0xfc00ffff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xc0000000-0xcfffffff] because it overlaps 0000:00:02.0 BAR 9 [mem 0x00000000-0xdfffffff 64bit pref] > > pnp 00:05: disabling [mem 0xfedc0000-0xfedc7fff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xfeda0000-0xfeda0fff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xfeda1000-0xfeda1fff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xc0000000-0xcfffffff disabled] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xfed20000-0xfed7ffff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xfed90000-0xfed93fff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xfed45000-0xfed8ffff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > pnp 00:05: disabling [mem 0xfee00000-0xfeefffff] because it overlaps 0000:03:00.0 BAR 9 [mem 0x00000000-0x3dffffffff 64bit pref] > > > > Mark resources that are not contained within their bridge window with > > IORESOURCE_UNSET in __pci_read_base() which resolves the false > > positives for the overlap check in quirk_system_pci_resources(). > > > > Fixes: f7834c092c42 ("PNP: Don't check for overlaps with unassigned PCI BARs") > > Signed-off-by: Ilpo Järvinen > > --- > > > > This change uses resource_contains() which will reject partial overlaps. > > I don't know for sure if partial overlaps should be allowed or not (but > > they feel as something FW didn't set things up properly so I chose to > > mark them UNSET as well). > > > > drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 7f9da8c41620..097389f25853 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -205,6 +205,26 @@ static void __pci_size_rom(struct pci_dev *dev, unsigned int pos, u32 *sizes) > > __pci_size_bars(dev, 1, pos, sizes, true); > > } > > > > +static struct resource *pbus_select_window_for_res_addr( > > + const struct pci_bus *bus, > > + const struct resource *res) > > +{ > > + unsigned long type = res->flags & IORESOURCE_TYPE_BITS; > > + struct resource *r; > > + > > + pci_bus_for_each_resource(bus, r) { > > + if (!r || r == &ioport_resource || r == &iomem_resource) I started to wonder if those two parent "anchor" resource can ever appear in resources returned by pci_bus_for_each_resource()? I've just copied those check from find_bus_resource_of_type() but it could be snake oil there as well. At least I don't see them ever in my quick tests, they only appeared as parents of the resources this loop iterates over. But again I'm limited to x86 systems so not sure if my testing yields universally true answers. > > + continue; > > + > > + if ((r->flags & IORESOURCE_TYPE_BITS) != type) > > + continue; > > + > > + if (resource_contains(r, res)) > > + return r; > > + } > > + return NULL; > > +} > > + > > /** > > * __pci_read_base - Read a PCI BAR > > * @dev: the PCI device > > @@ -329,6 +349,18 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, > > res_name, (unsigned long long)region.start); > > } > > > > + if (!(res->flags & IORESOURCE_UNSET)) { > > + struct resource *b_res; > > + > > + b_res = pbus_select_window_for_res_addr(dev->bus, res); > > + if (!b_res || > > + b_res->flags & (IORESOURCE_UNSET | IORESOURCE_DISABLED)) { > > + pci_dbg(dev, "%s %pR: no initial claim (no window)\n", > > + res_name, res); > > Should this be pci_info()? Or is there somewhere else that we > complain about a child resource that's not contained in a bridge > window? AFAIK, there's no other print. The kernel didn't even recognize this case until now so how could there have been one?! They'd generally show up as failures later in resource assignment if the resource doesn't fit to the bridge window [1], which should also set IORESOURCE_UNSET, but good luck for inferring things from that. It's tedious, I know. :-) If the bridge window is large enough, the base address would just change where the resource fits (I think). It can be pci_info() if you think that's better. I just picked the level which is the least noisy. We can go with pci_info() now and if the logging turns out excessive when we start to see dmesgs with it, we can of course adjust it later so it's not permanent either way. In any case, there's not much user can do for these as it's the setup FW gave us. > I recently got an internal report of child BARs being reassigned, I > think because they weren't inside a bridge window, and the dmesg log > (from an older kernel) showed the BAR reassignments, but didn't say > anything about the *reason* for the reassignment. Resource reassignment is only done after the resource was initially assigned so I'm not sure if that inferring chain is sound. Admittedly, you didn't exactly specify how you picked up that it was "reassigned" so it could be just terminology that doesn't match what setup-bus/res.c considers as resource reassignment. That is, if BAR's address was simply changed from the initial, that's not "reassignment" in the sense used by the kernel. I see these for ROM resource and uninitialized (0-based) resources but that isn't to say there couldn't be a case where the non-ROM resource was an invalid non-zero base address. [footnote 1] Some code uses IORESOURCE_UNSET where as others use ->parent to determine of the resource is not yet assigned. pdev_sort_resources() picks them based on ->parent so the resource fitting algorith tries to assign also resource that do not have IORESOURCE_UNSET. This entire ->parent and IORESOURCE_UNSET handling has lots of overlap and likely some remaining inconsistencies as well. I initially thought I could entirely drop IORESOURCE_UNSET and base everything on ->parent but I've now changed my mind because of this series. We need this flag between initial reading of BARs/windows and running the resource fitting/assignment algorithm. I could see some benefit from altering the meaning to something along the lines of IORESOURCE_INITIALLY_UNSET which is permanent flag. That information could then be used to determine we don't produce worse resource fits than what FW did. But it might be too hard to make such change to IORESOURCE_UNSET and there are not extra bits available in ->flags for realizing it parallel to existing flags. It may be useful to add some warnings if UNSET and ->parent are in disagreement to ensure consistent state for any resource. But those checks would need to cope the initial transient when windows are not yet claimed from the resource tree which makes it harder to find good spots for such checks. I generally dislike IORESOURCE_UNSET because it's semantics are not very clear and not properly enforced. -- i.