Re: [PATCH v2 3/3] PCI: Skip bridge window reads when window is not supported

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

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

On Wed, 18 Mar 2026, Ilpo Järvinen wrote:

> On Wed, 18 Mar 2026, Ahmed Naseef wrote:
> 
> > On Tue, Mar 17, 2026 at 04:29:08PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Mar 16, 2026 at 03:51:57PM +0000, Caleb James DeLisle wrote:
> > > > pci_read_bridge_io() and pci_read_bridge_mmio_pref() read bridge window
> > > > registers unconditionally. If the registers are hardwired to zero
> > > > (not implemented), both base and limit will be 0. Since (0 <= 0) is
> > > > true, a bogus window [mem 0x00000000-0x000fffff] or [io 0x0000-0x0fff]
> > > > gets created.
> > > > 
> > > > pci_read_bridge_windows() already detects unsupported windows by
> > > > testing register writability and sets io_window/pref_window flags
> > > > accordingly. Check these flags at the start of pci_read_bridge_io()
> > > > and pci_read_bridge_mmio_pref() to skip reading registers when the
> > > > window is not supported.
> > > 
> > > The fundamental problem here is that assigned space to a bridge window
> > > that isn't implemented.  I wish we understood the connection between
> > > this "read window" path and the assignment path.
> > > 
> > > Maybe this patch fixes it because we enter pci_read_bridge_mmio_pref()
> > > with res->flags being NULL, and we set IORESOURCE_MEM |
> > > IORESOURCE_PREFETCH again, which makes it look like we can assign
> > > space for it?
> > 
> > Yes, that's exactly right.
> > 
> > > 
> > > If that's the case, I think it would improve the commit log to mention
> > > the actual mechanism by which we avoid assigning space.
> > > 
> > 
> > How about this:
> > 
> >   pci_read_bridge_io() and pci_read_bridge_mmio_pref() read
> >   bridge window registers unconditionally. If the registers
> >   are hardwired to zero (not implemented), both base and limit
> >   will be 0. Since (0 <= 0) is true, these functions set
> >   IORESOURCE_IO or IORESOURCE_MEM | IORESOURCE_PREFETCH on
> >   the bridge resource. This causes the allocator to assign
> >   space for the window even though the hardware can't
> >   implement it.
> > 
> >   pci_read_bridge_windows() already detects unsupported windows
> >   by testing register writability and sets io_window/pref_window
> >   flags accordingly. Check these flags at the start of
> >   pci_read_bridge_io() and pci_read_bridge_mmio_pref() to skip
> >   reading registers when the window is not supported, so the
> >   resource flags remain clear and the allocator does not assign
> >   space for non-existent windows.
> 
> At least to me the proposed text reads much better than the original.
> The original text required reading between the lines to connect the dots, 
> whereas this new one clearly explains what causes what.

Hi again,

Reading the code I think the entire 0 <= 0 part is a red herring 
when it comes to the current code, the flags are always set by the 
functions!

The code would only add IORESOURCE_UNSET | IORESOURCE_DISABLED if the 
base <= limit check fails but that's still wrong because it says to the 
resource allocation code that it can enable that bridge window if it needs 
to.

Prior to the commit 8278c6914306 ("PCI: Preserve bridge window resource 
type flags") the base <= limit check did play some role (maybe the 
original commit message was based on some older tree than the most current 
one).

-- 
 i.