Message-ID: <a56284a4-755d-4eb4-ba77-9ea30e18d08f@linaro.org>
Date: Mon, 2 Jun 2025 15:40:18 +0100
From: Tudor Ambarus <tudor.ambarus@...aro.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
 Michał Winiarski <michal.winiarski@...el.com>,
 Igor Mammedov <imammedo@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
 Mika Westerberg <mika.westerberg@...ux.intel.com>,
 William McVicker <willmcvicker@...gle.com>
Subject: Re: [PATCH 24/25] PCI: Perform reset_resource() and build fail list
 in sync



On 5/30/25 3:48 PM, Ilpo Järvinen wrote:
>>> I added the suggested prints
>>> (https://paste.ofcode.org/DgmZGGgS6D36nWEzmfCqMm) on top of v6.15 with
>>> the downstream PCIe pixel driver and I obtain the following. Note that
>>> all added prints contain "tudor" for differentiation.
>>>
>>> [   15.211179][ T1107] pci 0001:01:00.0: [144d:a5a5] type 00 class
>>> 0x000000 PCIe Endpoint
>>> [   15.212248][ T1107] pci 0001:01:00.0: BAR 0 [mem
>>> 0x00000000-0x000fffff 64bit]
>>> [   15.212775][ T1107] pci 0001:01:00.0: ROM [mem 0x00000000-0x0000ffff
>>> pref]
>>> [   15.213195][ T1107] pci 0001:01:00.0: enabling Extended Tags
>>> [   15.213720][ T1107] pci 0001:01:00.0: PME# supported from D0 D3hot
>>> D3cold
>>> [   15.214035][ T1107] pci 0001:01:00.0: 15.752 Gb/s available PCIe
>>> bandwidth, limited by 8.0 GT/s PCIe x2 link at 0001:00:00.0 (capable of
>>> 31.506 Gb/s with 16.0 GT/s PCIe x2 link)
>>> [   15.222286][ T1107] pci 0001:01:00.0: tudor: 1: pbus_size_mem: BAR 0
>>> [mem 0x00000000-0x000fffff 64bit] list empty? 1
>>> [   15.222813][ T1107] pci 0001:01:00.0: tudor: 1: pbus_size_mem: ROM
>>> [mem 0x00000000-0x0000ffff pref] list empty? 1
>>> [   15.224429][ T1107] pci 0001:01:00.0: tudor: 2: pbus_size_mem: ROM
>>> [mem 0x00000000-0x0000ffff pref] list empty? 0
>>> [   15.224750][ T1107] pcieport 0001:00:00.0: bridge window [mem
>>> 0x00100000-0x001fffff] to [bus 01-ff] add_size 100000 add_align 100000
>>>
>>> [   15.225393][ T1107] tudor : pci_assign_unassigned_bus_resources:
>>> before __pci_bus_assign_resources -> list empty? 0
>>> [   15.225594][ T1107] pcieport 0001:00:00.0: tudor:
>>> pdev_sort_resources: bridge window [mem 0x00100000-0x001fffff] resource
>>> added in head list
>>> [   15.226078][ T1107] pcieport 0001:00:00.0: bridge window [mem
>>> 0x40000000-0x401fffff]: assigned
>> So here it ends up assigning the resource here I think.
>>
>>
>> That print isn't one of yours in reassign_resources_sorted() so the 
>> assignment must have been made in assign_requested_resources_sorted(). But 
>> then nothing is printed out from reassign_resources_sorted() so I suspect 
>> __assign_resources_sorted() has short-circuited.
>>
>> We know that realloc_head is not empty, so that leaves the goto out from 
>> if (list_empty(&local_fail_head)), which kind of makes sense, all 
>> entries on the head list were assigned. But the code there tries to remove 
>> all head list resources from realloc_head so why it doesn't get removed is 
>> still a mystery. assign_requested_resources_sorted() doesn't seem to 
>> remove anything from the head list so that resource should still be on the 
>> head list AFAICT so it should call that remove_from_list(realloc_head, 
>> dev_res->res) for it.
>>
>> So can you see if that theory holds water and it short-circuits without 
>> removing the entry from realloc_head?
> I think I figured out more about the reason. It's not related to that 
> bridge window resource.
> 
> pbus_size_mem() will add also that ROM resource into realloc_head 
> as it is considered (intentionally) optional after the optional change
> (as per "tudor: 2:" line). And that resource is never assigned because 

right, the ROM resource is added into realloc_head here:
https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/setup-bus.c#n1202

Then in the failing case, and extra resource is added:
[   15.224750][ T1107] pcieport 0001:00:00.0: bridge window [mem
0x00100000-0x001fffff] to [bus 01-ff] add_size 100000 add_align 100000

The above extra print happens just in the failing case. Here's where the
extra resource is added:
https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/setup-bus.c#n1285

It seems that in the failing case 2 resources are added into
realloc_head at the pbus_size_mem() time, whereas with the patch
reverted - none.

Also, in the failing case a smaller resource is added into the list:
pdev_sort_resources: bridge window [mem 0x00100000-0x001fffff]
compared to the working case:
pdev_sort_resources: bridge window [mem 0x00100000-0x002fffff]

Can this make a difference?

> pdev_sort_resources() didn't pick it up into the head list. The next 
> question is why the ROM resource isn't in the head list.
> 

It seems the ROM resource is skipped at:
https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/setup-bus.c#n175

tudor: pdev_sort_resources: ROM [??? 0x00000000 flags 0x0] resource
skipped due to !(r->flags) || r->parent

> 
> While it is not necessarily related to issue, I think the bridge sizing 
> functions too should consider pdev_resources_assignable() so that it
> won't ever add resources from such devices onto the realloc_head. This is 
> yet another small inconsistency within all this fitting/assignment logic.
> 
> pbus_size_mem() seems to consider IORESOURCE_PCI_FIXED so that cannot 
> explain it as the ROM resource wouldn't be on the realloc_head list in 
> that case.
> 
> 
> Just wanted to let you know early even if I don't fully understand 
> everything so you can hopefully avoid unnecessary debugging.

Thanks! Would adding some prints in pbus_size_mem() to describe the code
paths in the working and non-working case help?

Cheers,
ta

Powered by blists - more mailing lists