Message-ID: <6612c4d2-2533-98ef-7c89-f61d80c3e3e2@linux.intel.com>
Date: Tue, 1 Apr 2025 15:07:52 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
cc: Guenter Roeck <linux@...ck-us.net>, 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>
Subject: Re: [PATCH 24/25] PCI: Perform reset_resource() and build fail list
 in sync

On Tue, 1 Apr 2025, Ilpo Järvinen wrote:

> On Mon, 31 Mar 2025, Guenter Roeck wrote:
> > On Mon, Dec 16, 2024 at 07:56:31PM +0200, Ilpo Järvinen wrote:
> > > Resetting resource is problematic as it prevent attempting to allocate
> > > the resource later, unless something in between restores the resource.
> > > Similarly, if fail_head does not contain all resources that were reset,
> > > those resource cannot be restored later.
> > > 
> > > The entire reset/restore cycle adds complexity and leaving resources
> > > into reseted state causes issues to other code such as for checks done
> > > in pci_enable_resources(). Take a small step towards not resetting
> > > resources by delaying reset until the end of resource assignment and
> > > build failure list (fail_head) in sync with the reset to avoid leaving
> > > behind resources that cannot be restored (for the case where the caller
> > > provides fail_head in the first place to allow restore somewhere in the
> > > callchain, as is not all callers pass non-NULL fail_head).
> > > 
> > > The Expansion ROM check is temporarily left in place while building the
> > > failure list until the upcoming change which reworks optional resource
> > > handling.
> > > 
> > > Ideally, whole resource reset could be removed but doing that in a big
> > > step would make the impact non-tractable due to complexity of all
> > > related code.
> > > 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > 
> > With this patch in the mainline kernel, all mips:boston qemu emulations
> > fail when running a 64-bit little endian configuration (64r6el_defconfig).
> > 
> > The problem is that the PCI based IDE/ATA controller is not initialized.
> > There are a number of pci error messages.
> > 
> > pci_bus 0002:01: extended config space not accessible
> > pci 0002:01:00.0: [8086:2922] type 00 class 0x010601 conventional PCI endpoint
> > pci 0002:01:00.0: BAR 4 [io  0x0000-0x001f]
> > pci 0002:01:00.0: BAR 5 [mem 0x00000000-0x00000fff]
> > pci 0002:00:00.0: PCI bridge to [bus 01-ff]
> > pci_bus 0002:01: busn_res: [bus 01-ff] end is updated to 01
> > pci 0002:00:00.0: bridge window [mem 0x16000000-0x160fffff]: assigned
> > pci 0002:00:00.0: bridge window [mem size 0x00100000 64bit pref]: can't assign; no space
> > pci 0002:00:00.0: bridge window [mem size 0x00100000 64bit pref]: failed to assign
> > pci 0002:00:00.0: bridge window [io  size 0x1000]: can't assign; no space
> > pci 0002:00:00.0: bridge window [io  size 0x1000]: failed to assign
> > pci 0002:00:00.0: bridge window [mem size 0x00100000]: can't assign; bogus alignment
> > pci 0002:00:00.0: bridge window [mem 0x16000000-0x160fffff 64bit pref]: assigned
> > pci 0002:00:00.0: bridge window [io  size 0x1000]: can't assign; no space
> > pci 0002:00:00.0: bridge window [io  size 0x1000]: failed to assign
> > pci 0002:01:00.0: BAR 5 [mem size 0x00001000]: can't assign; no space
> > pci 0002:01:00.0: BAR 5 [mem size 0x00001000]: failed to assign
> > pci 0002:01:00.0: BAR 4 [io  size 0x0020]: can't assign; no space
> > pci 0002:01:00.0: BAR 4 [io  size 0x0020]: failed to assign
> > pci 0002:01:00.0: BAR 5 [mem size 0x00001000]: can't assign; no space
> > pci 0002:01:00.0: BAR 5 [mem size 0x00001000]: failed to assign
> > pci 0002:01:00.0: BAR 4 [io  size 0x0020]: can't assign; no space
> > pci 0002:01:00.0: BAR 4 [io  size 0x0020]: failed to assign
> > pci 0002:00:00.0: PCI bridge to [bus 01]
> > pci 0002:00:00.0:   bridge window [mem 0x16000000-0x160fffff 64bit pref]
> > pci_bus 0002:00: Some PCI device resources are unassigned, try booting with pci=realloc
> > pci_bus 0002:00: resource 4 [mem 0x16000000-0x160fffff]
> > pci_bus 0002:01: resource 2 [mem 0x16000000-0x160fffff 64bit pref]
> > ...
> > pci 0002:00:00.0: enabling device (0000 -> 0002)
> > ahci 0002:01:00.0: probe with driver ahci failed with error -12
> > 
> > Bisect points to this patch. Reverting it together with "PCI: Rework
> > optional resource handling" fixes the problem. For comparison, after
> > reverting the offending patches, the log messages are as follows.
> > 
> > pci_bus 0002:00: root bus resource [bus 00-ff]
> > pci_bus 0002:00: root bus resource [mem 0x16000000-0x160fffff]
> > pci 0002:00:00.0: [10ee:7021] type 01 class 0x060400 PCIe Root Complex Integrated Endpoint
> > pci 0002:00:00.0: PCI bridge to [bus 00]
> > pci 0002:00:00.0:   bridge window [io  0x0000-0x0fff]
> > pci 0002:00:00.0:   bridge window [mem 0x00000000-0x000fffff]
> > pci 0002:00:00.0:   bridge window [mem 0x00000000-0x000fffff 64bit pref]
> > pci 0002:00:00.0: enabling Extended Tags
> > pci 0002:00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> > pci_bus 0002:01: extended config space not accessible
> > pci 0002:01:00.0: [8086:2922] type 00 class 0x010601 conventional PCI endpoint
> > pci 0002:01:00.0: BAR 4 [io  0x0000-0x001f]
> > pci 0002:01:00.0: BAR 5 [mem 0x00000000-0x00000fff]
> > pci 0002:00:00.0: PCI bridge to [bus 01-ff]
> > pci_bus 0002:01: busn_res: [bus 01-ff] end is updated to 01
> > pci 0002:00:00.0: bridge window [mem 0x16000000-0x160fffff]: assigned
> > pci 0002:00:00.0: bridge window [mem size 0x00100000 64bit pref]: can't assign; no space
> > pci 0002:00:00.0: bridge window [mem size 0x00100000 64bit pref]: failed to assign
> > pci 0002:00:00.0: bridge window [io  size 0x1000]: can't assign; no space
> > pci 0002:00:00.0: bridge window [io  size 0x1000]: failed to assign
> > pci 0002:01:00.0: BAR 5 [mem 0x16000000-0x16000fff]: assigned
> > pci 0002:01:00.0: BAR 4 [io  size 0x0020]: can't assign; no space
> > pci 0002:01:00.0: BAR 4 [io  size 0x0020]: failed to assign
> > pci 0002:00:00.0: PCI bridge to [bus 01]
> > pci 0002:00:00.0:   bridge window [mem 0x16000000-0x160fffff]
> > pci_bus 0002:00: Some PCI device resources are unassigned, try booting with pci=realloc
> > pci_bus 0002:00: resource 4 [mem 0x16000000-0x160fffff]
> > pci_bus 0002:01: resource 1 [mem 0x16000000-0x160fffff]
> > ...
> > pci 0002:00:00.0: enabling device (0000 -> 0002)
> > ahci 0002:01:00.0: enabling device (0000 -> 0002)
> > ahci 0002:01:00.0: AHCI vers 0001.0000, 32 command slots, 1.5 Gbps, SATA mode
> > ahci 0002:01:00.0: 6/6 ports implemented (port mask 0x3f)
> > ahci 0002:01:00.0: flags: 64bit ncq only
> 
> Hi,
> 
> Thanks for reporting. Please add this to the command line to get the 
> resource releasing between the steps to show:
> 
> dyndbg="file drivers/pci/setup-bus.c +p"
> 
> Also, the log snippet just shows it fails but it is impossible to know 
> from it why the resource assigments do not fit so could you please provide 
> a complete dmesg logs. Also providing the contents of /proc/iomem from the 
> working case would save me quite a bit of decoding the iomem layout from 
> the dmesgs.

Hi again,

If you could kindly include this patch into the test with pci_dbg() 
enabled so the resource reset/restore is better tracked.


From e7cb98904ab2ee235bbc13b3e981332e944dd476 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@...ux.intel.com>
Date: Tue, 1 Apr 2025 15:05:13 +0300
Subject: [PATCH 1/1] PCI: Log reset and restore of resources
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PCI resource fitting and assignment is complicated to track because it
performs many actions without any logging. One of these is resource
reset (zeroing the resource) and the restore during the multi-passed
resource fitting algorithm.

Resource reset does not play well with the other PCI code if the code
later wants to reattempt assignment of that resource, knowing that a
resource was left into the reset state without a pairing restore is
useful for understanding issues that show up as resource assignment
failures.

Add pci_dbg() to both reset and restore to be better able to track
what's going on within the resource fitting algorithm.

The resource fitting and assignment tracking should eventually be
convert to use trace events to cover all changes made into the
resources fully but that requires significant effort. In the meantime,
logging resource reset and restore with pci_dbg() seems low-hanging
fruit.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
---
 drivers/pci/setup-bus.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 54d6f4fa3ce1..e67af19cb876 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -130,10 +130,15 @@ static resource_size_t get_res_add_align(struct list_head *head,
 static void restore_dev_resource(struct pci_dev_resource *dev_res)
 {
 	struct resource *res = dev_res->res;
+	struct pci_dev *dev = dev_res->dev;
+	int idx = pci_resource_num(dev, res);
+	const char *res_name = pci_resource_name(dev, idx);
 
 	res->start = dev_res->start;
 	res->end = dev_res->end;
 	res->flags = dev_res->flags;
+
+	pci_dbg(dev_res->dev, "%s %pR: resource restored\n", res_name, res);
 }
 
 static bool pdev_resources_assignable(struct pci_dev *dev)
@@ -218,8 +223,15 @@ bool pci_resource_is_optional(const struct pci_dev *dev, int resno)
 	return false;
 }
 
-static inline void reset_resource(struct resource *res)
+static void reset_resource(struct pci_dev_resource *dev_res)
 {
+	struct resource *res = dev_res->res;
+	struct pci_dev *dev = dev_res->dev;
+	int idx = pci_resource_num(dev, res);
+	const char *res_name = pci_resource_name(dev, idx);
+
+	pci_dbg(dev_res->dev, "%s %pR: resetting resource\n", res_name, res);
+
 	res->start = 0;
 	res->end = 0;
 	res->flags = 0;
@@ -573,7 +585,7 @@ static void __assign_resources_sorted(struct list_head *head,
 				    0 /* don't care */);
 		}
 
-		reset_resource(res);
+		reset_resource(dev_res);
 	}
 
 	free_list(head);

base-commit: 7d06015d936c861160803e020f68f413b5c3cd9d
-- 
2.39.5

Powered by blists - more mailing lists