Message-ID: <89a20c14-dd0f-22ae-d998-da511a94664a@linux.intel.com>
Date: Mon, 20 Oct 2025 19:20:10 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
cc: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>, 
    Kai-Heng Feng <kaihengf@...dia.com>, Rob Herring <robh@...nel.org>, 
    LKML <linux-kernel@...r.kernel.org>, 
    Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
    Krzysztof Wilczyński <kw@...ux.com>, 
    Linux-Renesas <linux-renesas-soc@...r.kernel.org>, 
    Linux-Renesas <linux-renesas-soc@...r.kernel.or>
Subject: Re: [PATCH 0/3] PCI & resource: Make coalescing host bridge windows
 safer

On Mon, 20 Oct 2025, Geert Uytterhoeven wrote:
> On Fri, 10 Oct 2025 at 16:42, Ilpo Järvinen
> <ilpo.jarvinen@...ux.intel.com> wrote:
> > Here's a series for Geert to test if this fixes the improper coalescing
> > of resources as was experienced with the pci_add_resource() change (I
> > know the breaking change was pulled before 6.18 main PR but I'd want to
> > retry it later once the known issues have been addressed). The expected
> > result is there'll be two adjacent host bridge resources in the
> > resource tree as the different name should disallow coalescing them
> > together, and therefore BAR0 has a window into which it belongs to.
> >
> > Generic info for the series:
> >
> > PCI host bridge windows were coalesced in place into one of the structs
> > on the resources list. The host bridge window coalescing code does not
> > know who holds references and still needs the struct resource it's
> > coalescing from/to so it is safer to perform coalescing into entirely
> > a new struct resource instead and leave the old resource addresses as
> > they were.
> >
> > The checks when coalescing is allowed are also made stricter so that
> > only resources that have identical the metadata can be coalesced.
> >
> > As a bonus, there's also a bit of framework to easily create kunit
> > tests for resource tree functions (beyond just resource_coalesce()).
> >
> > Ilpo Järvinen (3):
> >   PCI: Refactor host bridge window coalescing loop to use prev
> >   PCI: Do not coalesce host bridge resource structs in place
> >   resource, kunit: add test case for resource_coalesce()
> 
> Thanks for your series!
> 
> I have applied this on top of commit 06b77d5647a4d6a7 ("PCI:
> Mark resources IORESOURCE_UNSET when outside bridge windows"), and
> gave it a a try on Koelsch (R-Car M2-W).

So the pci_bus_add_resource() patch to rcar_pci_probe() was not included?
No coalescing would be attempted without that change.

With the pci_bus_add_resource() patch, the resource name is different, I 
think, so even then it should abort coalescing the ranges with this 
series.

> Impact on dmesg (for the first PCI/USB) instance:
> 
>      pci-rcar-gen2 ee090000.pci: host bridge /soc/pci@...90000 ranges:
>      pci-rcar-gen2 ee090000.pci:      MEM 0x00ee080000..0x00ee08ffff
> -> 0x00ee080000
>      pci-rcar-gen2 ee090000.pci: PCI: revision 11
>      pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
>      pci_bus 0000:00: root bus resource [bus 00]
>      pci_bus 0000:00: root bus resource [mem 0xee080000-0xee08ffff]
>      pci 0000:00:00.0: [1033:0000] type 00 class 0x060000 conventional
> PCI endpoint
>     -pci 0000:00:00.0: BAR 0 [mem 0xee090800-0xee090bff]: no initial
> claim (no window)
>      pci 0000:00:00.0: BAR 0 [mem size 0x00000400]
>     -pci 0000:00:00.0: BAR 1 [mem 0x40000000-0x7fffffff pref]: no
> initial claim (no window)
>      pci 0000:00:00.0: BAR 1 [mem size 0x40000000 pref]
>      pci 0000:00:01.0: [1033:0035] type 00 class 0x0c0310 conventional
> PCI endpoint
>     -pci 0000:00:01.0: BAR 0 [mem 0x00000000-0x00000fff]: no initial
> claim (no window)
>      pci 0000:00:01.0: BAR 0 [mem size 0x00001000]
>      pci 0000:00:01.0: supports D1 D2
>      pci 0000:00:01.0: PME# supported from D0 D1 D2 D3hot
>      pci 0000:00:02.0: [1033:00e0] type 00 class 0x0c0320 conventional
> PCI endpoint
>     -pci 0000:00:02.0: BAR 0 [mem 0x00000000-0x000000ff]: no initial
> claim (no window)
>      pci 0000:00:02.0: BAR 0 [mem size 0x00000100]
>      pci 0000:00:02.0: supports D1 D2
>      pci 0000:00:02.0: PME# supported from D0 D1 D2 D3hot
>      PCI: bus0: Fast back to back transfers disabled
>      pci 0000:00:01.0: BAR 0 [mem 0xee080000-0xee080fff]: assigned
>      pci 0000:00:02.0: BAR 0 [mem 0xee081000-0xee0810ff]: assigned
>      pci_bus 0000:00: resource 4 [mem 0xee080000-0xee08ffff]
>      pci 0000:00:01.0: enabling device (0140 -> 0142)
>      pci 0000:00:02.0: enabling device (0140 -> 0142)
> 
> I.e. the "no initial claim (no window)" messages introduced by commit
> 06b77d5647a4d6a7 are no longer seen.

Is that perhaps again because of pci_dbg() vs pci_info()?

> The BARs still show "mem size <n>" instead of the "mem <start>-<end>"
> before commit 06b77d5647a4d6a7, though.

To me this looks like UNSET was still set for these resources. Missing the 
pci_bus_add_resource() patch would explain that and if the pci_dbg() 
wasn't printed, it would explain both symptoms.

Once these questions are resolved, I'll ask Rob what is the preferred 
solution here, a) driver doing pci_bus_add_resource() or b) including it 
into the DT ranges (if we could fix the ranges).

We likely need to go with a) to preserve backwards compatibility but I 
also want to understand how those ranges are supposed to be used if we 
wouldn't have historical baggage.


(I appreciate following through even if the original series is now 
reverted!)

> This series has not impact on /proc/iomem, or on the output of
> "lspci -v" (commit 06b77d5647a4d6a7 also had no impact here).
> I.e. the part of /proc/iomem related to the first PCI/USB instance
> still looks like:
> 
>     ee080000-ee08ffff : pci@...90000
>       ee080000-ee080fff : 0000:00:01.0
>         ee080000-ee080fff : ohci_hcd
>       ee081000-ee0810ff : 0000:00:02.0
>         ee081000-ee0810ff : ehci_hcd
>     ee090000-ee090bff : ee090000.pci pci@...90000
>
> I hope this matches your expectation.s
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

-- 
 i.

Powered by blists - more mailing lists