| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a4ce405c-4abd-400f-afd8-16c53170250e@nvidia.com>
Date: Wed, 8 Oct 2025 16:40:10 +0800
From: Kai-Heng Feng <kaihengf@...dia.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Rob Herring <robh@...nel.org>, 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>,
Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH 2/2] PCI: Resources outside their window must set
IORESOURCE_UNSET
On 2025/10/8 1:30 AM, Ilpo Järvinen wrote:
> External email: Use caution opening links or attachments
>
>
> + Kai-Heng
>
> On Mon, 6 Oct 2025, Geert Uytterhoeven wrote:
>> On Mon, 6 Oct 2025 at 14:37, Ilpo Järvinen
>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>> On Mon, 6 Oct 2025, Geert Uytterhoeven wrote:
>>>> On Fri, 3 Oct 2025 at 16:58, Ilpo Järvinen
>>>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>>>> On Fri, 3 Oct 2025, Geert Uytterhoeven wrote:
>>>>>> On Thu, 2 Oct 2025 at 18:59, Ilpo Järvinen
>>>>>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>>>>>> On Thu, 2 Oct 2025, Geert Uytterhoeven wrote:
>>>>>>>> On Thu, 2 Oct 2025 at 16:54, Ilpo Järvinen
>>>>>>>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>>>>>>>> On Wed, 1 Oct 2025, Geert Uytterhoeven wrote:
>>>>>>>>>> On Wed, 1 Oct 2025 at 15:06, Ilpo Järvinen
>>>>>>>>>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>>>>>>>>>> On Wed, 1 Oct 2025, Geert Uytterhoeven wrote:
>>>>>>>>>>>> On Tue, 30 Sept 2025 at 18:32, Ilpo Järvinen
>>>>>>>>>>>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>>>>>>>>>>>> On Tue, 30 Sep 2025, Geert Uytterhoeven wrote:
>>>>>>>>>>>>>> On Fri, 26 Sept 2025 at 04:40, Ilpo Järvinen
>>>>>>>>>>>>>> <ilpo.jarvinen@...ux.intel.com> 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>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks for your patch, which is now commit 06b77d5647a4d6a7 ("PCI:
>>>>>>>>>>>>>> Mark resources IORESOURCE_UNSET when outside bridge windows") in
>>>>>>>>>>>>>> linux-next/master next-20250929 pci/next
>>>>>>>>>>>>
>>>>>>>>>>>>>> This replaces the actual resources by their sizes in the boot log on
>>>>>>>>>>>>>> e.g. on R-Car M2-W:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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]
>>>>>>>>>>>>>> -pci 0000:00:00.0: BAR 1 [mem 0x40000000-0x7fffffff pref]
>>>>>>>>>>>>>
>>>>>>>>>>>>> What is going to be the parent of these resources? They don't seem to fall
>>>>>>>>>>>>> under the root bus resource above in which case the output change is
>>>>>>>>>>>>> intentional so they don't appear as if address range would be "okay".
>>>>>>>>>>>>
>>>>>>>>>>>> >From /proc/iomem:
>>>>>>>>>>>>
>>>>>>>>>>>> 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
>>>>>>>>>>>
>>>>>>>>>>> Okay, so this seem to appear in the resource tree but not among the root
>>>>>>>>>>> bus resources.
>>>>>>>>>>>
>>>>>>>>>>>> ee0c0000-ee0cffff : pci@...d0000
>>>>>>>>>>>> ee0c0000-ee0c0fff : 0001:01:01.0
>>>>>>>>>>>> ee0c0000-ee0c0fff : ohci_hcd
>>>>>>>>>>>> ee0c1000-ee0c10ff : 0001:01:02.0
>>>>>>>>>>>> ee0c1000-ee0c10ff : ehci_hcd
>>>>>>>>>>>>
>>>>>>>>>>>>> When IORESOURCE_UNSET is set in a resource, %pR does not print the start
>>>>>>>>>>>>> and end addresses.
>>>>>>>>>>>>
>>>>>>>>>>>> Yeah, that's how I found this commit, without bisecting ;-)
>>>>>>>>>>>>
>>>>>>>>>>>>>> +pci 0000:00:00.0: BAR 0 [mem size 0x00000400]
>>>>>>>>>>>>>> +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]
>>>>>>>>>>>>>> +pci 0000:00:01.0: BAR 0 [mem size 0x00001000]
>>>>>>>>>>>>>
>>>>>>>>>>>>> For this resource, it's very much intentional. It's a zero-based BAR which
>>>>>>>>>>>>> was left without IORESOURCE_UNSET prior to my patch leading to others part
>>>>>>>>>>>>> of the kernel to think that resource range valid and in use (in your
>>>>>>>>>>>>> case it's so small it wouldn't collide with anything but it wasn't
>>>>>>>>>>>>> properly set up resource, nonetheless).
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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]
>>>>>>>>>>>>>> +pci 0000:00:02.0: BAR 0 [mem size 0x00000100]
>>>>>>>>>>>>>
>>>>>>>>>>>>> And this as well is very much intentional.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Is that intentional?
>>>>>>>>>>>>>
>>>>>>>>>>>>> There's also that pci_dbg() in the patch which would show the original
>>>>>>>>>>>>> addresses (print the resource before setting IORESOURCE_UNSET) but to see
>>>>>>>>>>>>> it one would need to enable it with dyndbg=... Bjorn was thinking of
>>>>>>>>>>>>> making that pci_info() though so it would appear always.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Was this the entire PCI related diff? I don't see those 0000:00:00.0 BARs
>>>>>>>>>>>>> getting assigned anywhere.
>>>>>>>>>>>>
>>>>>>>>>>>> The above log is almost complete (lacked enabling the device afterwards).
>>>>>>>>>>>>
>>>>>>>>>>>> AFAIU, the BARs come from the reg and ranges properties in the
>>>>>>>>>>>> PCI controller nodes;
>>>>>>>>>>>> https://elixir.bootlin.com/linux/v6.17/source/arch/arm/boot/dts/renesas/r8a7791.dtsi#L1562
>>>>>>>>>>>
>>>>>>>>>>> Thanks, although I had already found this line by grep. I was just
>>>>>>>>>>> expecting the address appear among root bus resources too.
>>>>>>>>>>>
>>>>>>>>>>> Curiously enough, pci_register_host_bridge() seems to try to add some
>>>>>>>>>>> resource from that list into bus and it's what prints those "root bus
>>>>>>>>>>> resource" lines and ee090000 is not among the printed lines despite
>>>>>>>>>>> appearing in /proc/iomem. As is, the resource tree and PCI bus view on the
>>>>>>>>>>> resources seems to be in disagreement and I'm not sure what to make of it.
>>>>>>>>>>>
>>>>>>>>>>> But before considering going into that direction or figuring out why this
>>>>>>>>>>> ee090000 resource does not appear among root bus resources, could you
>>>>>>>>>>> check if the attached patch changes behavior for the resource which are
>>>>>>>>>>> non-zero based?
>>>>>>>>>>
>>>>>>>>>> This patch has no impact on dmesg, lspci output, or /proc/iomem
>>>>>>>>>> for pci-rcar-gen2.
>>>>>>>>>
>>>>>>>>> It would have been too easy... :-(
>>>>>>>>>
>>>>>>>>> I'm sorry I don't really know these platform well and I'm currently trying
>>>>>>>>> to understand what adds that ee090000 resource into the resource tree
>>>>>>>>> and so far I've not been very successful.
>>>>>>>>>
>>>>>>>>> Perhaps it would be easiest to print a stacktrace when the resource is
>>>>>>>>> added but there are many possible functions. I think all of them
>>>>>>>>> converge in __request_resource() so I suggest adding:
>>>>>>>>>
>>>>>>>>> WARN_ON(new->start == 0xee090000);
>>>>>>>>>
>>>>>>>>> before __request_resource() does anything.
>>>>>>>>
>>>>>>>> Call trace:
>>>>>>>> unwind_backtrace from show_stack+0x10/0x14
>>>>>>>> show_stack from dump_stack_lvl+0x7c/0xb0
>>>>>>>> dump_stack_lvl from __warn+0x80/0x198
>>>>>>>> __warn from warn_slowpath_fmt+0xc0/0x124
>>>>>>>> warn_slowpath_fmt from __request_resource+0x38/0xc8
>>>>>>>> __request_resource from __request_region+0xc4/0x1e8
>>>>>>>> __request_region from __devm_request_region+0x80/0xac
>>>>>>>> __devm_request_region from __devm_ioremap_resource+0xcc/0x160
>>>>>>>> __devm_ioremap_resource from rcar_pci_probe+0x58/0x350
>>>>>>>> rcar_pci_probe from platform_probe+0x58/0x90
>>>>>>>>
>>>>>>>> I.e. the call to devm_platform_get_and_ioremap_resource() in
>>>>>>>> drivers/pci/controller/pci-rcar-gen2.c:rcar_pci_probe().
>>>>>>>
>>>>>>> Thanks, the patch below might help BAR0 (but I'm not sure if it's the
>>>>>>> correct solution due to being so unfamiliar with these kind of platforms
>>>>>>> and how they're initialized).
>>>>>>
>>>>>> Thanks, that has the following impact on dmesg:
>>>>>>
>>>>>> 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_bus 0000:00: root bus resource [mem 0xee080000-0xee090bff]
>>>>>> 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 0 [mem 0xee090800-0xee090bff]
>>>>>> 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)
>>>>>
>>>>> Did you e.g. forget to change pci_dbg() to pci_info() as these all went
>>>>> away, I cannot see why it should disappear because of my patch?
>>>>>
>>>>> (No need to apologize if that was the case, just confirming if that
>>>>> explains it is enough. :-))
>>>>
>>>> I indeed dropped that change. Adding it back...
>>>>
>>>>>> 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
>>>>>
>>>>> Perhaps print here what's the parent resource of these resource.
>>>>
>>>> pci 0000:00:01.0: BAR 0 [mem 0xee080000-0xee080fff]: assigned (parent
>>>> [mem 0xee080000-0xee08ffff])
>>>> pci 0000:00:02.0: BAR 0 [mem 0xee081000-0xee0810ff]: assigned (parent
>>>> [mem 0xee080000-0xee08ffff])
>>>
>>> Were these from a kernel without the problematic commit at all or with the
>>> problematic commit and my fix? They look like the former case. The full
>>> /proc/iomem also shows all the parent resources I think.
>>
>> With commit 06b77d5647a4d6a7 ("PCI: Mark resources IORESOURCE_UNSET when
>> outside bridge windows"), but without adding
>> "pci_add_resource(&bridge->windows, cfg_res);" in .probe().
>>
>>>>>> -pci_bus 0000:00: resource 4 [mem 0xee080000-0xee08ffff]
>>>>>> +pci_bus 0000:00: resource 4 [mem 0xee080000-0xee090bff]
>>>>>> pci-rcar-gen2 ee0d0000.pci: adding to PM domain always-on
>>>>>> PM: genpd_add_device: Add ee0d0000.pci to always-on
>>>>>> pci-rcar-gen2 ee0d0000.pci: host bridge /soc/pci@...d0000 ranges:
>>>>>> @@ -414,26 +416,22 @@ PM: ==== always-on/ee0d0000.pci: start
>>>>>> pci-rcar-gen2 ee0d0000.pci: PCI: revision 11
>>>>>> pci-rcar-gen2 ee0d0000.pci: PCI host bridge to bus 0001:01
>>>>>> pci_bus 0001:01: root bus resource [bus 01]
>>>>>> -pci_bus 0001:01: root bus resource [mem 0xee0c0000-0xee0cffff]
>>>>>> +pci_bus 0001:01: root bus resource [mem 0xee0c0000-0xee0d0bff]
>>>>>> pci 0001:01:00.0: [1033:0000] type 00 class 0x060000 conventional PCI endpoint
>>>>>> -pci 0001:01:00.0: BAR 0 [mem 0xee0d0800-0xee0d0bff]: no initial claim (no window)
>>>>>> -pci 0001:01:00.0: BAR 0 [mem size 0x00000400]
>>>>>> -pci 0001:01:00.0: BAR 1 [mem 0x40000000-0x7fffffff pref]: no initial claim (no window)
>>>>>> +pci 0001:01:00.0: BAR 0 [mem 0xee0d0800-0xee0d0bff]
>>>>>> pci 0001:01:00.0: BAR 1 [mem size 0x40000000 pref]
>>>>>> pci 0001:01:01.0: [1033:0035] type 00 class 0x0c0310 conventional PCI endpoint
>>>>>> -pci 0001:01:01.0: BAR 0 [mem 0x00000000-0x00000fff]: no initial claim (no window)
>>>>>> pci 0001:01:01.0: BAR 0 [mem size 0x00001000]
>>>>>> pci 0001:01:01.0: supports D1 D2
>>>>>> pci 0001:01:01.0: PME# supported from D0 D1 D2 D3hot
>>>>>> pci 0001:01:02.0: [1033:00e0] type 00 class 0x0c0320 conventional PCI endpoint
>>>>>> -pci 0001:01:02.0: BAR 0 [mem 0x00000000-0x000000ff]: no initial claim (no window)
>>>>>> pci 0001:01:02.0: BAR 0 [mem size 0x00000100]
>>>>>> pci 0001:01:02.0: supports D1 D2
>>>>>> pci 0001:01:02.0: PME# supported from D0 D1 D2 D3hot
>>>>>> PCI: bus1: Fast back to back transfers disabled
>>>>>> pci 0001:01:01.0: BAR 0 [mem 0xee0c0000-0xee0c0fff]: assigned
>>>>>> pci 0001:01:02.0: BAR 0 [mem 0xee0c1000-0xee0c10ff]: assigned
>>>>>> -pci_bus 0001:01: resource 4 [mem 0xee0c0000-0xee0cffff]
>>>>>> +pci_bus 0001:01: resource 4 [mem 0xee0c0000-0xee0d0bff]
>>>>>> rcar-pcie fe000000.pcie: adding to PM domain always-on
>>>>>> PM: genpd_add_device: Add fe000000.pcie to always-on
>>>>>> rcar-pcie fe000000.pcie: host bridge /soc/pcie@...00000 ranges:
>>>>>>
>>>>>> and on /proc/iomem (I used "diff -w" to reduce clutter):
>>>>>>
>>>>>> ec700000-ec70ffff : ec700000.dma-controller dma-controller@...00000
>>>>>> ec720000-ec72ffff : ec720000.dma-controller dma-controller@...20000
>>>>>> ec740000-ec7401ff : ec500000.sound audmapp
>>>>>> -ee080000-ee08ffff : pci@...90000
>>>>>
>>>>> So what did add this previous? (Maybe use the same WARN_ON() trick as
>>>>> previously to find out.)
>>>>
>>>> First call:
>>>>
>>>> + __request_resource from request_resource_conflict+0x24/0x3c
>>>> + request_resource_conflict from devm_request_resource+0x48/0x9c
>>>> + devm_request_resource from devm_request_pci_bus_resources+0x5c/0x70
>>>> + devm_request_pci_bus_resources from devm_of_pci_bridge_init+0x7c/0x1c0
>>>> + devm_of_pci_bridge_init from devm_pci_alloc_host_bridge+0x44/0x6c
>>>> + devm_pci_alloc_host_bridge from rcar_pci_probe+0x34/0x384
>>>> + rcar_pci_probe from platform_probe+0x58/0x90
>>>
>>> Thanks. So this is the call of interest, the rest are just the childs of
>>> it with the same address.
>>>
>>> I'm looking devm_of_pci_get_host_bridge_resources(), it seems to read
>>> "ranges" property but not "reg" at all.
>>>
>>> I wonder if devm_of_pci_get_host_bridge_resources() should also loop
>>> through "reg" addresses and add those to host resources which are outside
>>> of the "ranges"? Or if there should be a "range" that covers all what's
>>> in "reg" to get them added into host bridge resources? That would seem the
>>> generic solution instead of trying to do this in rcar_pci_probe().
>>>
>>> (Perhaps these are stupid questions, please excuse my lack of knowledge
>>> about OF things.)
>>>
>>> While looking at another issue report, I also notice of_pci_prop_ranges()
>>> assumes there are only bridge windows or BARs, but not both (not sure if
>>> this relates to anything, just an observation while reading these code
>>> paths).
>>
>> We still have Rob in CC...
>
> While we wait, can you simply try to make the "ranges" large enough to fit
> the BAR0 too?
>
> I think it will results in making the larger "ranges" entry the parent of
> ee090000-ee090bff : ee090000.pci pci@...90000 entry in the resource tree
> which would also avoid the coalescing issue.
>
> Again, I'm not entirely sure if that would be an acceptable solution.
>
>>>> Second call:
>>>>
>>>> + __request_resource from allocate_resource+0x1b8/0x1d4
>>>> + allocate_resource from pci_bus_alloc_from_region+0x1e0/0x220
>>>> + pci_bus_alloc_from_region from pci_bus_alloc_resource+0x84/0xb8
>>>> + pci_bus_alloc_resource from _pci_assign_resource+0x78/0x150
>>>> + _pci_assign_resource from pci_assign_resource+0x10c/0x310
>>>> + pci_assign_resource from assign_requested_resources_sorted+0x78/0xac
>>>> + assign_requested_resources_sorted from
>>>> __assign_resources_sorted+0x74/0x5c4
>>>> + __assign_resources_sorted from __pci_bus_assign_resources+0x50/0x1f0
>>>> + __pci_bus_assign_resources from
>>>> pci_assign_unassigned_root_bus_resources+0xa8/0x190
>>>> + pci_assign_unassigned_root_bus_resources from pci_host_probe+0x5c/0xb0
>>>> + pci_host_probe from rcar_pci_probe+0x2e0/0x384
>>>> + rcar_pci_probe from platform_probe+0x58/0x90
>>>>
>>>> Third call:
>>>>
>>>> + __request_resource from __request_region+0xc4/0x1e8
>>>> + __request_region from __devm_request_region+0x80/0xac
>>>> + __devm_request_region from usb_hcd_pci_probe+0x15c/0x35c
>>>> + usb_hcd_pci_probe from pci_device_probe+0x94/0x104
>>>> + pci_device_probe from really_probe+0x128/0x28c
>>>>
>>>> Fourth call:
>>>>
>>>> + __request_region from __devm_request_region+0x80/0xac
>>>> + __devm_request_region from usb_hcd_pci_probe+0x15c/0x35c
>>>> + usb_hcd_pci_probe from pci_device_probe+0x94/0x104
>>>> + pci_device_probe from really_probe+0x128/0x28c
>>>>
>>>> Fifth call:
>>>>
>>>> + __request_region from __devm_request_region+0x80/0xac
>>>> + __devm_request_region from usb_hcd_pci_probe+0x15c/0x35c
>>>> + usb_hcd_pci_probe from pci_device_probe+0x94/0x104
>>>> + pci_device_probe from really_probe+0x128/0x28c
>>>>
>>>>> It might have gotten broken because the coalesed resource
>>>>> ee080000-ee090bff overlaps with that other resource in the resource tree.
>>>>> But I don't see anything to that effect in the log so it's either silent
>>>>> failure or there's much filtered from the log.
>>>>>
>>>>>> - 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
>>>>>> -ee0c0000-ee0cffff : pci@...d0000
>>>>>> - ee0c0000-ee0c0fff : 0001:01:01.0
>>>>>> ee0c0000-ee0c0fff : ohci_hcd
>>>>>> - ee0c1000-ee0c10ff : 0001:01:02.0
>>>>>> ee0c1000-ee0c10ff : ehci_hcd
>>>>>> ee0d0000-ee0d0bff : ee0d0000.pci pci@...d0000
>>>>>> ee100000-ee100327 : ee100000.mmc mmc@...00000
>>>>>>
>>>>>> Removing the pci@...x0000 and 000x:0x:0x.0 entries doesn't look
>>>>>> right to me? Or am I missing something?
>>>>>
>>>>> I cannot understand this output, these resources seem to have been now
>>>>> left without a parent and due to -w I don't know what's their real
>>>>> indentation level.
>>>>
>>>> The *_hcd resources are now at the top level.
>>>>
>>>> ec700000-ec70ffff : ec700000.dma-controller dma-controller@...00000
>>>> ec720000-ec72ffff : ec720000.dma-controller dma-controller@...20000
>>>> ec740000-ec7401ff : ec500000.sound audmapp
>>>> -ee080000-ee08ffff : pci@...90000
>>>> - ee080000-ee080fff : 0000:00:01.0
>>>> - ee080000-ee080fff : ohci_hcd
>>>> - ee081000-ee0810ff : 0000:00:02.0
>>>> - ee081000-ee0810ff : ehci_hcd
>>>> +ee080000-ee080fff : ohci_hcd
>>>> +ee081000-ee0810ff : ehci_hcd
>>>> ee090000-ee090bff : ee090000.pci pci@...90000
>>>> -ee0c0000-ee0cffff : pci@...d0000
>>>> - ee0c0000-ee0c0fff : 0001:01:01.0
>>>> - ee0c0000-ee0c0fff : ohci_hcd
>>>> - ee0c1000-ee0c10ff : 0001:01:02.0
>>>> - ee0c1000-ee0c10ff : ehci_hcd
>>>> +ee0c0000-ee0c0fff : ohci_hcd
>>>> +ee0c1000-ee0c10ff : ehci_hcd
>>>> ee0d0000-ee0d0bff : ee0d0000.pci pci@...d0000
>>>> ee100000-ee100327 : ee100000.mmc mmc@...00000
>>>> ee140000-ee1400ff : ee140000.mmc mmc@...40000
>>>>
>>>> I assume the others are gone because they now conflict with the *_hcd
>>>> resources at the top level.
>>>
>>> Like you initially assumed, it is wrong (while it works as the resources
>>> themselves don't care that much about their parent they're under as long
>>> as the resource is assigned, it's still not intended to be that way).
>>>
>>> It might be worth to see if the coalescing in pci_register_host_bridge()
>>> somehow messes these resources up either by not doing the coalesing loop
>>> at all or by adding:
>>>
>>> if (res->parent || next_res->parent) {
>>> if (res->parent)
>>> pr_info("Has parent %pR\n", res);
>>> if (next_res->parent)
>>> pr_info("Has parent %pR\n", next_res);
>>> continue;
>>> }
>>>
>>> ...into the coalescing loop in case one of them happens to have a parent
>>> (this is to be tested with the rcar_probe() fix).
>>
>> The above restores the missing entries in /proc/iomem.
>> Changes to dmesg:
>>
>> 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
>> +Has parent [mem 0xee080000-0xee08ffff]
>> pci_bus 0000:00: root bus resource [bus 00]
>> pci_bus 0000:00: root bus resource [mem 0xee080000-0xee08ffff]
>> +pci_bus 0000:00: root bus resource [mem 0xee090000-0xee090bff]
>> 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 0 [mem 0xee090800-0xee090bff]
>> 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
>> @@ -403,21 +397,24 @@ pci 0000:00:02.0: BAR 0 [mem size 0x0000
>> 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_bus 0000:00: resource 5 [mem 0xee090000-0xee090bff]
>> pci-rcar-gen2 ee0d0000.pci: adding to PM domain always-on
>> pci-rcar-gen2 ee0d0000.pci: host bridge /soc/pci@...d0000 ranges:
>> pci-rcar-gen2 ee0d0000.pci: MEM 0x00ee0c0000..0x00ee0cffff
>> -> 0x00ee0c0000
>> pci-rcar-gen2 ee0d0000.pci: PCI: revision 11
>> pci-rcar-gen2 ee0d0000.pci: PCI host bridge to bus 0001:01
>> +Has parent [mem 0xee0c0000-0xee0cffff]
>
> So this ended up locating another bug in the coalescing loop.
>
> It's very dangerous to mess with resources like that as the backing
> struct resource is shared with whatever added that resource, which is
> different sites in this case.
>
> I'm preparing a better approach to do the resource merge, however, I'm
> left unsure if the clean up of everything will happen "correctly" as this
> coalescing is removing resources from the resource tree which were added
> there by something else so it's then becomes very muddy who is responsible
> for releasing it in the end.
So what about a helper function pci_try_contiguous_apertures() to allocate
across resources when single resource isn't large enough?
Anyway, I am happy to try your new approach.
Kai-Heng
>
>> pci_bus 0001:01: root bus resource [bus 01]
>> pci_bus 0001:01: root bus resource [mem 0xee0c0000-0xee0cffff]
>> +pci_bus 0001:01: root bus resource [mem 0xee0d0000-0xee0d0bff]
>> pci 0001:01:00.0: [1033:0000] type 00 class 0x060000 conventional
>> PCI endpoint
>> -pci 0001:01:00.0: BAR 0 [mem 0xee0d0800-0xee0d0bff]: no initial
>> claim (no window)
>> -pci 0001:01:00.0: BAR 0 [mem size 0x00000400]
>> +pci 0001:01:00.0: BAR 0 [mem 0xee0d0800-0xee0d0bff]
>> pci 0001:01:00.0: BAR 1 [mem 0x40000000-0x7fffffff pref]: no
>> initial claim (no window)
>> pci 0001:01:00.0: BAR 1 [mem size 0x40000000 pref]
>> pci 0001:01:01.0: [1033:0035] type 00 class 0x0c0310 conventional
>> PCI endpoint
>> @@ -431,9 +428,10 @@ pci 0001:01:02.0: BAR 0 [mem size 0x0000
>> pci 0001:01:02.0: supports D1 D2
>> pci 0001:01:02.0: PME# supported from D0 D1 D2 D3hot
>> PCI: bus1: Fast back to back transfers disabled
>> pci 0001:01:01.0: BAR 0 [mem 0xee0c0000-0xee0c0fff]: assigned
>> pci 0001:01:02.0: BAR 0 [mem 0xee0c1000-0xee0c10ff]: assigned
>> pci_bus 0001:01: resource 4 [mem 0xee0c0000-0xee0cffff]
>> +pci_bus 0001:01: resource 5 [mem 0xee0d0000-0xee0d0bff]
>>
>>>>>>> If this works, we'll also have to decide what to do with the BAR1 (it
>>>>>>> didn't appear in your (partial?) /proc/iomem quote so I'm left unsure how
>>>>>>> to approach it).
>>>>>>
>>>>>> That is indeed not visible in /proc/iomem.
>>>>>
>>>>> I meant before the commit 06b77d5647a4d6a7 ("PCI Mark resources
>>>>> IORESOURCE_UNSET when outside bridge windows"), was it present?
>>>>
>>>> No, it was not present.
>>>>
>>>>>> I tried the following (whitespace-damaged):
>>>>>>
>>>>>> --- a/drivers/pci/controller/pci-rcar-gen2.c
>>>>>> +++ b/drivers/pci/controller/pci-rcar-gen2.c
>>>>>> @@ -179,6 +179,7 @@ static void rcar_pci_setup(struct rcar_pci *priv)
>>>>>> unsigned long window_size;
>>>>>> unsigned long window_addr;
>>>>>> unsigned long window_pci;
>>>>>> + struct resource res;
>>>>>> u32 val;
>>>>>>
>>>>>> entry = resource_list_first_type(&bridge->dma_ranges, IORESOURCE_MEM);
>>>>>> @@ -191,6 +192,8 @@ static void rcar_pci_setup(struct rcar_pci *priv)
>>>>>> window_pci = entry->res->start - entry->offset;
>>>>>> window_size = resource_size(entry->res);
>>>>>> }
>>>>>> + resource_set_range(&res, window_addr, window_size);
>>>>>
>>>>> You need to set flags properly too as this now tried to insert BUS, not
>>>>> MEM resource (DEFINE_RES() might be the more appropriate in that case
>>>>> anyway).
>>>>>
>>>>> However, if there's not &bridge->dma_ranges ranges entry, rcar_pci_setup()
>>>>> seems to initialize the resource to 0x40000000-0x7fffffff and I'm not sure
>>>>
>>>> I guess the not &bridge->dma_ranges case dates back to the time the
>>>> DTS didn't have dma-ranges yet. However, upon closer look, the DTS
>>>> still doesn't have dma_ranges, thus always using the default.
>>>>
>>>>> how it's supposed to work if there's more than one of these devices as per
>>>>> the log above.
>>>>
>>>> Upon closer look, this is not a resource of the device, but an inbound
>>>> memory region. Hence there is no issue if multiple devices use the
>>>> same region.
>>>>
>>>>>
>>>>>> + pci_add_resource(&bridge->windows, &res);
>>>>>
>>>>> What would be the backing resource in the resource tree for this? I'm not
>>>>> sure if pci_add_resource() is going to result in adding one into the
>>>>> resource tree.
>>>>
>>>> Likewise, it should not appear in /proc/ioem.
>>>
>>> Thanks for checking it out.
>>>
>>> I wonder how it would be supposed to work if PCI resource fitting logic
>>> finds place for it and changes its address. I don't think it would ever
>>> happen because it should never fit...
>>>
>>> ...But the logic still is a bit fishy if rcar2 code expects that address
>>> to be fixed but doesn't flag the resource to have a fixed address.
>>
>> How can the PCI resource fitting logic change it? It is an inbound
>> memory region, not a normal BAR?
>
> I thought it can happen because it appears as BAR1, all resources from
> BARs are eligible for the normal assignment but now that I think it more,
> the underlying struct resource is going different for that BAR1 and the
> actual dma_ranges entry.
>
> --
> i.
Powered by blists - more mailing lists