linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] PCI: Two resource fitting algorith fixes
@ 2025-06-10 10:20 Ilpo Järvinen
  2025-06-10 10:21 ` [PATCH 1/2] PCI: Relaxed tail alignment should never increase min_align Ilpo Järvinen
  2025-06-10 10:21 ` [PATCH 2/2] PCI: Fix pdev_resources_assignable() disparity Ilpo Järvinen
  0 siblings, 2 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2025-06-10 10:20 UTC (permalink / raw)
  To: Bjorn Helgaas, linux-pci, Tudor Ambarus, Rio,
	Krzysztof Wilczyński
  Cc: linux-kernel, Ilpo Järvinen

Thanks to reporters bisecting their issues into the recent PCI
resource fitting and assignment rework changes, I've located two
issues in the algorithm which are addressed by this series.

Ilpo Järvinen (2):
  PCI: Relaxed tail alignment should never increase min_align
  PCI: Fix pdev_resources_assignable() disparity

 drivers/pci/setup-bus.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.39.5


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] PCI: Relaxed tail alignment should never increase min_align
  2025-06-10 10:20 [PATCH 0/2] PCI: Two resource fitting algorith fixes Ilpo Järvinen
@ 2025-06-10 10:21 ` Ilpo Järvinen
  2025-06-10 11:31   ` Rio Liu
  2025-06-10 10:21 ` [PATCH 2/2] PCI: Fix pdev_resources_assignable() disparity Ilpo Järvinen
  1 sibling, 1 reply; 6+ messages in thread
From: Ilpo Järvinen @ 2025-06-10 10:21 UTC (permalink / raw)
  To: Bjorn Helgaas, linux-pci, Tudor Ambarus, Rio,
	Krzysztof Wilczyński, Ilpo Järvinen, linux-kernel
  Cc: stable

When using relaxed tail alignment for the bridge window,
pbus_size_mem() also tries to minimize min_align, which can under
certain scenarios end up increasing min_align from that found by
calculate_mem_align().

Ensure min_align is not increased by the relaxed tail alignment.

Eventually, it would be better to add calculate_relaxed_head_align()
similar to calculate_mem_align() which finds out what alignment can be
used for the head without introducing any gaps into the bridge window
to give flexibility on head address too. But that looks relatively
complex algorithm so it requires much more testing than fixing the
immediate problem causing a regression.

Fixes: 67f9085596ee ("PCI: Allow relaxed bridge window tail sizing for optional resources")
Reported-by: Rio <rio@r26.me>
Tested-by: Rio <rio@r26.me>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: <stable@vger.kernel.org>
---
 drivers/pci/setup-bus.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 07c3d021a47e..f90d49cd07da 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1169,6 +1169,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	resource_size_t children_add_size = 0;
 	resource_size_t children_add_align = 0;
 	resource_size_t add_align = 0;
+	resource_size_t relaxed_align;
 
 	if (!b_res)
 		return -ENOSPC;
@@ -1246,8 +1247,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	if (bus->self && size0 &&
 	    !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
 					   size0, min_align)) {
-		min_align = 1ULL << (max_order + __ffs(SZ_1M));
-		min_align = max(min_align, win_align);
+		relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
+		relaxed_align = max(relaxed_align, win_align);
+		min_align = min(min_align, relaxed_align);
 		size0 = calculate_memsize(size, min_size, 0, 0, resource_size(b_res), win_align);
 		pci_info(bus->self, "bridge window %pR to %pR requires relaxed alignment rules\n",
 			 b_res, &bus->busn_res);
@@ -1261,8 +1263,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 		if (bus->self && size1 &&
 		    !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
 						   size1, add_align)) {
-			min_align = 1ULL << (max_order + __ffs(SZ_1M));
-			min_align = max(min_align, win_align);
+			relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
+			relaxed_align = max(min_align, win_align);
+			min_align = min(min_align, relaxed_align);
 			size1 = calculate_memsize(size, min_size, add_size, children_add_size,
 						  resource_size(b_res), win_align);
 			pci_info(bus->self,
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] PCI: Fix pdev_resources_assignable() disparity
  2025-06-10 10:20 [PATCH 0/2] PCI: Two resource fitting algorith fixes Ilpo Järvinen
  2025-06-10 10:21 ` [PATCH 1/2] PCI: Relaxed tail alignment should never increase min_align Ilpo Järvinen
@ 2025-06-10 10:21 ` Ilpo Järvinen
  2025-06-10 11:10   ` Tudor Ambarus
  1 sibling, 1 reply; 6+ messages in thread
From: Ilpo Järvinen @ 2025-06-10 10:21 UTC (permalink / raw)
  To: Bjorn Helgaas, linux-pci, Tudor Ambarus, Rio,
	Krzysztof Wilczyński, Ilpo Järvinen, linux-kernel
  Cc: stable

pdev_sort_resources() uses pdev_resources_assignable() helper to decide
if device's resources cannot be assigned. pbus_size_mem(), on the other
hand, does not do the same check. This could lead into a situation
where a resource ends up on realloc_head list but is not on the head
list, which is turn prevents emptying the resource from the
realloc_head list in __assign_resources_sorted().

A non-empty realloc_head is unacceptable because it triggers an
internal sanity check as show in this log with a device that has class
0 (PCI_CLASS_NOT_DEFINED):

pci 0001:01:00.0: [144d:a5a5] type 00 class 0x000000 PCIe Endpoint
pci 0001:01:00.0: BAR 0 [mem 0x00000000-0x000fffff 64bit]
pci 0001:01:00.0: ROM [mem 0x00000000-0x0000ffff pref]
pci 0001:01:00.0: enabling Extended Tags
pci 0001:01:00.0: PME# supported from D0 D3hot D3cold
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)
pcieport 0001:00:00.0: bridge window [mem 0x00100000-0x001fffff] to [bus 01-ff] add_size 100000 add_align 100000
pcieport 0001:00:00.0: bridge window [mem 0x40000000-0x401fffff]: assigned
------------[ cut here ]------------
kernel BUG at drivers/pci/setup-bus.c:2532!
Internal error: Oops - BUG: 00000000f2000800 [#1]  SMP
...
Call trace:
 pci_assign_unassigned_bus_resources+0x110/0x114 (P)
 pci_rescan_bus+0x28/0x48

Use pdev_resources_assignable() also within pbus_size_mem() to skip
processing of non-assignable resources which removes the disparity in
between what resources pdev_sort_resources() and pbus_size_mem()
consider. As non-assignable resources are no longer processed, they are
not added to the realloc_head list, thus the sanity check no longer
triggers.

This disparity problem is very old but only now became apparent after
the commit 2499f5348431 ("PCI: Rework optional resource handling") that
made the ROM resources optional when calculating bridge window sizes
which required adding the resource to the realloc_head list.
Previously, bridge windows were just sized larger than necessary.

Fixes: 2499f5348431 ("PCI: Rework optional resource handling")
Reported-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: <stable@vger.kernel.org>
---

The reporter was perhaps not happy with this fix as behavior of PCI core
isn't identical after this fix even if this patch fixes the problem on
the PCI core side which causes the internal sanity check to fire.

It seems that in the reporter's case, an out-of-tree driver was involved
that performed things and made assumptions a driver should not do in its
probe function such as assuming a bridge window is assigned even if there
are not child resources to be put into it (the child device in reporter's
case doesn't have a valid class and gets therefore skipped by the resource
fitting/assignment):

https://lore.kernel.org/all/bd579412-d07c-476d-8932-55c1f69adc9f@linaro.org/

In other words, the out-of-tree driver relies on the disparity in the
PCI core's resource fitting code which is now eliminated by this fix.

 drivers/pci/setup-bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index f90d49cd07da..24863d8d0053 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1191,6 +1191,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			resource_size_t r_size;
 
 			if (r->parent || (r->flags & IORESOURCE_PCI_FIXED) ||
+			    !pdev_resources_assignable(dev) ||
 			    ((r->flags & mask) != type &&
 			     (r->flags & mask) != type2 &&
 			     (r->flags & mask) != type3))
-- 
2.39.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] PCI: Fix pdev_resources_assignable() disparity
  2025-06-10 10:21 ` [PATCH 2/2] PCI: Fix pdev_resources_assignable() disparity Ilpo Järvinen
@ 2025-06-10 11:10   ` Tudor Ambarus
  0 siblings, 0 replies; 6+ messages in thread
From: Tudor Ambarus @ 2025-06-10 11:10 UTC (permalink / raw)
  To: Ilpo Järvinen, Bjorn Helgaas, linux-pci, Rio,
	Krzysztof Wilczyński, linux-kernel
  Cc: stable



On 6/10/25 11:21 AM, Ilpo Järvinen wrote:
> The reporter was perhaps not happy 

No, no, very happy in fact. Thanks for all the help!
I figured out that I need to get familiar with the resource fitting and
assignment code, and PCI in general, before trying to fix the problem
with the downstream drivers. Which is something that I can't allocate
time for right now.

So even if I couldn't fix what's going on downstream, I'd like to thank
you for all the help and time!

Cheers,
ta

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] PCI: Relaxed tail alignment should never increase min_align
  2025-06-10 10:21 ` [PATCH 1/2] PCI: Relaxed tail alignment should never increase min_align Ilpo Järvinen
@ 2025-06-10 11:31   ` Rio Liu
  2025-06-10 11:35     ` Ilpo Järvinen
  0 siblings, 1 reply; 6+ messages in thread
From: Rio Liu @ 2025-06-10 11:31 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Bjorn Helgaas, linux-pci, Tudor Ambarus,
	Krzysztof Wilczyński, linux-kernel, stable

Thanks for fixing the problem so quick!

> Reported-by: Rio <rio@r26.me>
> Tested-by: Rio <rio@r26.me>


My full name is Rio Liu if you want to include it, sorry for not mentioning it earlier.

Thanks,
Rio


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] PCI: Relaxed tail alignment should never increase min_align
  2025-06-10 11:31   ` Rio Liu
@ 2025-06-10 11:35     ` Ilpo Järvinen
  0 siblings, 0 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2025-06-10 11:35 UTC (permalink / raw)
  To: Rio Liu
  Cc: Bjorn Helgaas, linux-pci, Tudor Ambarus,
	Krzysztof Wilczyński, LKML, stable

On Tue, 10 Jun 2025, Rio Liu wrote:

> Thanks for fixing the problem so quick!
>
> > Reported-by: Rio <rio@r26.me>
> > Tested-by: Rio <rio@r26.me>
> 
> 
> My full name is Rio Liu if you want to include it, sorry for not 
> mentioning it earlier. 

Ah, sorry. I missed it was fully shown by the later emails on the From 
line.

-- 
 i.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-06-10 11:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-10 10:20 [PATCH 0/2] PCI: Two resource fitting algorith fixes Ilpo Järvinen
2025-06-10 10:21 ` [PATCH 1/2] PCI: Relaxed tail alignment should never increase min_align Ilpo Järvinen
2025-06-10 11:31   ` Rio Liu
2025-06-10 11:35     ` Ilpo Järvinen
2025-06-10 10:21 ` [PATCH 2/2] PCI: Fix pdev_resources_assignable() disparity Ilpo Järvinen
2025-06-10 11:10   ` Tudor Ambarus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).