Re: [PATCH 2/2] PCI: Resources outside their window must set IORESOURCE_UNSET

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

On Fri, 26 Sep 2025, Bjorn Helgaas wrote:
> On Fri, Sep 26, 2025 at 03:21:17PM +0300, Ilpo Järvinen wrote:
> > 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():
> 
> > > > @@ -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.
> 
> Here's the case I saw (a v6.1 kernel, so old log messages):
> 
>   pci 0000:00:00.0: bridge window [mem 0x80000000-0x97fffffff 64bit pref] to [bus 01-05] add_size 80000000 add_align 80000000
>   pci 0000:00:00.0: BAR 15: assigned [mem 0x380000000-0xcffffffff 64bit pref]
>   pci 0000:01:01.0: BAR 0: [mem 0xb00000000-0xbffffffff 64bit pref]
>   ...
>   pci 0000:01:01.7: BAR 0: [mem 0x400000000-0x4ffffffff 64bit pref]
>   pci 0000:01:01.0: BAR 0: assigned [mem 0x400000000-0x4ffffffff 64bit pref]
> 
> Obviously these initial BAR 0 values don't fit in the
> [0x80000000-0x97fffffff] bridge window, so I think we moved and
> expanded it and then assigned the BARs to be inside.
> 
> I was thinking might get the "can't claim; no compatible bridge
> window" message in pci_claim_resource() as a clue, but we didn't.

Is pci_bus_claim_resources() called in this case? That requires 
preserve_config. In my tests pci_bus_allocate_dev_resources() is never 
called, only bridge window resources are claimed.

> This *seems* like a firmware defect: why would firmware bother to
> program these BARs at all unless it also made a bridge window that
> could reach them.

-- 
 i.