Message-ID: <2493cba7-151d-5ce6-5f33-e56966baac7a@linux.intel.com>
Date: Fri, 26 Sep 2025 15:21:17 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Bjorn Helgaas <helgaas@...nel.org>
cc: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>, 
    Krzysztof Wilczyński <kw@...ux.com>, 
    "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>, 
    LKML <linux-kernel@...r.kernel.org>, 
    Lucas De Marchi <lucas.demarchi@...el.com>
Subject: Re: [PATCH 2/2] PCI: Resources outside their window must set
 IORESOURCE_UNSET

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 <ilpo.jarvinen@...ux.intel.com>
> > ---
> > 
> > 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.

Powered by blists - more mailing lists