From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: "Bjorn Helgaas" <bhelgaas@google.com>,
linux-pci@vger.kernel.org,
"Michał Winiarski" <michal.winiarski@intel.com>,
"Igor Mammedov" <imammedo@redhat.com>,
LKML <linux-kernel@vger.kernel.org>,
"Mika Westerberg" <mika.westerberg@linux.intel.com>,
"William McVicker" <willmcvicker@google.com>
Subject: Re: [PATCH 24/25] PCI: Perform reset_resource() and build fail list in sync
Date: Mon, 2 Jun 2025 18:08:40 +0300 (EEST) [thread overview]
Message-ID: <7e882cfb-a35a-bab0-c333-76a4e79243b6@linux.intel.com> (raw)
In-Reply-To: <a56284a4-755d-4eb4-ba77-9ea30e18d08f@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 6771 bytes --]
On Mon, 2 Jun 2025, Tudor Ambarus wrote:
> 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?
I don't think the bridge window resource different matters, that bridge
resource got assigned just fine so it is a red herring. The code just
calculates optional sizes in different place after the rework than before
it. The successful assignment itself is for the same size so there's
nothing wrong there AFAICT.
> > 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
I don't see the device in this print, hope it is for the same device.
In any case, I don't understand what reset resource's flags in between
pbus_size_mem() and pdev_sort_resources(), or alternative, why type
checking in pbus_size_mem() matches if flags == 0 at that point.
Those two functions should work on the same resources, if one skips
something, the other should too. Disparity between them can cause issues,
but despite reading the code multiple times, I couldn't figure out how
that disparity occurs (except for the !pdev_resources_assignable() case).
> > 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?
It is of interest to know why the same resource is treated differently.
So what were the resource flags, type* args when it's processed by
pbus_size_mem()? If resource's flags are zero at that point but it matches
one of the types, that would be a bug.
--
i.
next prev parent reply other threads:[~2025-06-02 15:11 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-16 17:56 [PATCH 00/25] PCI: Resource fitting/assignment fixes and cleanups Ilpo Järvinen
2025-02-14 9:59 ` Xiaochun Lee
2024-12-16 17:56 ` [PATCH 01/25] PCI: Remove add_align overwrite unrelated to size0 Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 02/25] PCI: size0 is unrelated to add_align Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 03/25] PCI: Simplify size1 assignment logic Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 04/25] PCI: Optional bridge window size too may need relaxing Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 05/25] PCI: Fix old_size lower bound in calculate_iosize() too Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 06/25] PCI: Use SZ_* instead of literals in setup-bus.c Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 07/25] PCI: resource_set_range/size() conversions Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 08/25] PCI: Add a helper to identify IOV resources Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 09/25] PCI: Check resource_size() separately Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 10/25] PCI: Add pci_resource_num() helper Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 11/25] PCI: Add dev & res local variables to resource assignment funcs Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 12/25] PCI: Converge return paths in __assign_resources_sorted() Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 13/25] PCI: Refactor pdev_sort_resources() & __dev_sort_resources() Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 14/25] PCI: Use while loop and break instead of gotos Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 15/25] PCI: Rename retval to ret Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 16/25] PCI: Consolidate assignment loop next round preparation Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 17/25] PCI: Remove wrong comment from pci_reassign_resource() Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 18/25] PCI: Add restore_dev_resource() Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 19/25] PCI: Extend enable to check for any optional resource Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 20/25] PCI: Always have realloc_head in __assign_resources_sorted() Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 21/25] PCI: Indicate optional resource assignment failures Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 22/25] PCI: Add debug print when releasing resources before retry Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 23/25] PCI: Use res->parent to check is resource is assigned Ilpo Järvinen
2024-12-16 17:56 ` [PATCH 24/25] PCI: Perform reset_resource() and build fail list in sync Ilpo Järvinen
2025-04-01 2:35 ` Guenter Roeck
2025-04-01 10:18 ` Ilpo Järvinen
2025-04-01 12:07 ` Ilpo Järvinen
2025-04-01 14:15 ` Guenter Roeck
2025-04-01 17:38 ` Ilpo Järvinen
2025-04-11 19:37 ` Ondřej Jirman
2025-04-14 9:52 ` Ilpo Järvinen
2025-04-14 12:19 ` Ondřej Jirman
2025-04-14 13:15 ` Ilpo Järvinen
2025-04-14 13:43 ` Ondřej Jirman
2025-04-14 13:52 ` Ilpo Järvinen
2025-04-01 13:28 ` Guenter Roeck
2025-05-06 15:03 ` Tudor Ambarus
2025-05-06 15:53 ` Ilpo Järvinen
2025-05-28 11:22 ` Tudor Ambarus
2025-05-28 11:39 ` Tudor Ambarus
2025-05-28 13:09 ` Tudor Ambarus
2025-05-30 6:55 ` Ilpo Järvinen
2025-05-30 6:38 ` Ilpo Järvinen
2025-05-30 14:48 ` Ilpo Järvinen
2025-06-02 14:40 ` Tudor Ambarus
2025-06-02 15:08 ` Ilpo Järvinen [this message]
2025-06-02 18:42 ` Tudor Ambarus
2025-06-03 8:13 ` Ilpo Järvinen
2025-06-03 10:36 ` Tudor Ambarus
2025-06-03 10:48 ` Tudor Ambarus
2025-06-03 11:43 ` Tudor Ambarus
2025-06-03 14:23 ` Ilpo Järvinen
2025-06-03 14:43 ` Ilpo Järvinen
2025-06-03 14:13 ` Ilpo Järvinen
2025-06-03 15:25 ` Tudor Ambarus
2025-06-03 17:03 ` Ilpo Järvinen
2025-06-03 17:09 ` Ilpo Järvinen
2025-06-02 12:32 ` Tudor Ambarus
2025-06-19 0:30 ` D Scott Phillips
2025-06-24 12:48 ` Ilpo Järvinen
2025-06-25 17:45 ` Ilpo Järvinen
2025-06-25 20:33 ` D Scott Phillips
2025-06-26 9:22 ` Ilpo Järvinen
2025-06-26 14:53 ` D Scott Phillips
2024-12-16 17:56 ` [PATCH 25/25] PCI: Rework optional resource handling Ilpo Järvinen
2025-02-13 21:46 ` [PATCH 00/25] PCI: Resource fitting/assignment fixes and cleanups Bjorn Helgaas
2025-02-14 8:18 ` Xiaochun XC17 Li | 李小春 Xavier
2025-02-14 11:53 ` Ilpo Järvinen
2025-02-14 21:28 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7e882cfb-a35a-bab0-c333-76a4e79243b6@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=bhelgaas@google.com \
--cc=imammedo@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=michal.winiarski@intel.com \
--cc=mika.westerberg@linux.intel.com \
--cc=tudor.ambarus@linaro.org \
--cc=willmcvicker@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.