[PATCH 0/7] PCI: Solve two bridge window sizing issues

[PATCH 0/7] PCI: Solve two bridge window sizing issues

[Ilpo Järvinen]

Hi all,

Here's a series that contains two fixes to PCI bridge window sizing
algorithm. Together, they should enable remove & rescan cycle to work
for a PCI bus that has PCI devices with optional resources and/or
disparity in BAR sizes.

For the second fix, I chose to expose find_empty_resource_slot() from
kernel/resource.c because it should increase accuracy of the cannot-fit
decision (currently that function is called find_resource()). In order
to do that sensibly, a few improvements seemed in order to make its
interface and name of the function sane before exposing it. Thus, the
few extra patches on resource side.

Unfortunately I don't have a reason to suspect these would help with
the issues related to the currently ongoing resource regression
thread [1].

[1] https://lore.kernel.org/linux-pci/ZXpaNCLiDM+Kv38H@marvin.atrad.com.au/

Ilpo Järvinen (7):
  PCI: Fix resource double counting on remove & rescan
  resource: Rename find_resource() to find_empty_resource_slot()
  resource: Document find_empty_resource_slot() and resource_constraint
  resource: Use typedef for alignf callback
  resource: Handle simple alignment inside __find_empty_resource_slot()
  resource: Export find_empty_resource_slot()
  PCI: Relax bridge window tail sizing rules

 drivers/pci/bus.c       | 10 +----
 drivers/pci/setup-bus.c | 81 ++++++++++++++++++++++++++++++++++++++---
 include/linux/ioport.h  | 44 ++++++++++++++++++++--
 include/linux/pci.h     |  5 +--
 kernel/resource.c       | 69 ++++++++++++++++-------------------
 5 files changed, 150 insertions(+), 59 deletions(-)

-- 
2.30.2

[PATCH 1/7] PCI: Fix resource double counting on remove & rescan

[Ilpo Järvinen]

pbus_size_mem() keeps the size of the optional resources in
children_add_size. When calculating the PCI bridge window size,
calculate_memsize() lower bounds size by old_size before adding
children_add_size and performing the window size alignment. This
results in double counting for the resources in children_add_size
because old_size may be based on the previous size of the bridge
window after it has already included children_add_size (that is,
size1 in pbus_size_mem() from an earlier invocation of that
function).

As a result, on repeated remove of the bus & rescan cycles the resource
size keeps increasing when children_add_size is non-zero as can be seen
from this extract:

iomem0:  23fffd00000-23fffdfffff : PCI Bus 0000:03
iomem1:  20000000000-200001fffff : PCI Bus 0000:03
iomem2:  20000000000-200002fffff : PCI Bus 0000:03
iomem3:  20000000000-200003fffff : PCI Bus 0000:03
iomem4:  20000000000-200004fffff : PCI Bus 0000:03

Solve the double counting by moving old_size check later in
calculate_memsize() so that children_add_size is already accounted for.

After the patch, the bridge window retains its size as expected:

iomem0:  23fffd00000-23fffdfffff : PCI Bus 0000:03
iomem1:  20000000000-200000fffff : PCI Bus 0000:03
iomem2:  20000000000-200000fffff : PCI Bus 0000:03

Fixes: a4ac9fea016f ("PCI : Calculate right add_size")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/setup-bus.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index fd74f1c99dba..e3e6ff8854a7 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -820,11 +820,9 @@ static resource_size_t calculate_memsize(resource_size_t size,
 		size = min_size;
 	if (old_size == 1)
 		old_size = 0;
-	if (size < old_size)
-		size = old_size;
 
-	size = ALIGN(max(size, add_size) + children_add_size, align);
-	return size;
+	size = max(size, add_size) + children_add_size;
+	return ALIGN(max(size, old_size), align);
 }
 
 resource_size_t __weak pcibios_window_alignment(struct pci_bus *bus,
-- 
2.30.2

[PATCH 2/7] resource: Rename find_resource() to find_empty_resource_slot()

[Ilpo Järvinen]

Rename find_resource() to find_empty_resource_slot() to better describe
what the functions does. This is a preparation for exposing it beyond
resource.c which is needed by PCI core. Also rename the __ variant to
match the names.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 kernel/resource.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 866ef3663a0b..94f67005e1e2 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -574,10 +574,9 @@ static void resource_clip(struct resource *res, resource_size_t min,
  * Find empty slot in the resource tree with the given range and
  * alignment constraints
  */
-static int __find_resource(struct resource *root, struct resource *old,
-			 struct resource *new,
-			 resource_size_t  size,
-			 struct resource_constraint *constraint)
+static int __find_empty_resource_slot(struct resource *root, struct resource *old,
+				      struct resource *new, resource_size_t size,
+				      struct resource_constraint *constraint)
 {
 	struct resource *this = root->child;
 	struct resource tmp = *new, avail, alloc;
@@ -633,11 +632,11 @@ next:		if (!this || this->end == root->end)
 /*
  * Find empty slot in the resource tree given range and alignment.
  */
-static int find_resource(struct resource *root, struct resource *new,
-			resource_size_t size,
-			struct resource_constraint  *constraint)
+static int find_empty_resource_slot(struct resource *root, struct resource *new,
+				    resource_size_t size,
+				    struct resource_constraint *constraint)
 {
-	return  __find_resource(root, NULL, new, size, constraint);
+	return  __find_empty_resource_slot(root, NULL, new, size, constraint);
 }
 
 /**
@@ -660,7 +659,7 @@ static int reallocate_resource(struct resource *root, struct resource *old,
 
 	write_lock(&resource_lock);
 
-	if ((err = __find_resource(root, old, &new, newsize, constraint)))
+	if ((err = __find_empty_resource_slot(root, old, &new, newsize, constraint)))
 		goto out;
 
 	if (resource_contains(&new, old)) {
@@ -729,7 +728,7 @@ int allocate_resource(struct resource *root, struct resource *new,
 	}
 
 	write_lock(&resource_lock);
-	err = find_resource(root, new, size, &constraint);
+	err = find_empty_resource_slot(root, new, size, &constraint);
 	if (err >= 0 && __request_resource(root, new))
 		err = -EBUSY;
 	write_unlock(&resource_lock);
-- 
2.30.2

[PATCH 3/7] resource: Document find_empty_resource_slot() and resource_constraint

[Ilpo Järvinen]

Document find_empty_resource_slot() and the struct resource_constraint
as they are going to be exposed outside of resource.c.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 kernel/resource.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 94f67005e1e2..ed4bb8ad701a 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -46,7 +46,18 @@ struct resource iomem_resource = {
 };
 EXPORT_SYMBOL(iomem_resource);
 
-/* constraints to be met while allocating resources */
+/**
+ * resource_constraint - constraints to be met while searching empty resource slots
+ * @min:		The minimum address for the memory range
+ * @max:		The maximum address for the memory range
+ * @align:		Alignment for the start address of the empty slot
+ * @alignf:		Additional alignment constraints callback
+ * @alignf_data:	Data provided for @alignf callback
+ *
+ * Contains the range and alignment constraints that have to be met during
+ * find_empty_resource_slot(). @alignf can be NULL indicating no alignment
+ * beyond @align is necessary.
+ */
 struct resource_constraint {
 	resource_size_t min, max, align;
 	resource_size_t (*alignf)(void *, const struct resource *,
@@ -629,8 +640,19 @@ next:		if (!this || this->end == root->end)
 	return -EBUSY;
 }
 
-/*
- * Find empty slot in the resource tree given range and alignment.
+/**
+ * find_empty_resource_slot - Find empty slot in the resource tree
+ * @root:	Root resource descriptor
+ * @new:	Resource descriptor awaiting an empty resource slot
+ * @size:	The minimum size of the empty slot
+ * @constraint:	The range and alignment constraints to be met
+ *
+ * Finds an empty slot under @root in the resource tree satisfying range and
+ * alignment @constraints.
+ *
+ * Return:
+ * * %0		- if successful, @new members start, end, and flags are altered.
+ * * %-EBUSY	- if no empty slot was found.
  */
 static int find_empty_resource_slot(struct resource *root, struct resource *new,
 				    resource_size_t size,
-- 
2.30.2

[PATCH 4/7] resource: Use typedef for alignf callback

[Ilpo Järvinen]

To make it simpler to declare resource constraint alignf callbacks, add
typedef for it and document it.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/bus.c      | 10 ++--------
 include/linux/ioport.h | 22 ++++++++++++++++++----
 include/linux/pci.h    |  5 +----
 kernel/resource.c      |  8 ++------
 4 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9c2137dae429..be5a4bb4d2fb 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -176,10 +176,7 @@ static void pci_clip_resource_to_region(struct pci_bus *bus,
 static int pci_bus_alloc_from_region(struct pci_bus *bus, struct resource *res,
 		resource_size_t size, resource_size_t align,
 		resource_size_t min, unsigned long type_mask,
-		resource_size_t (*alignf)(void *,
-					  const struct resource *,
-					  resource_size_t,
-					  resource_size_t),
+		resource_alignf alignf,
 		void *alignf_data,
 		struct pci_bus_region *region)
 {
@@ -250,10 +247,7 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, struct resource *res,
 int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
 		resource_size_t size, resource_size_t align,
 		resource_size_t min, unsigned long type_mask,
-		resource_size_t (*alignf)(void *,
-					  const struct resource *,
-					  resource_size_t,
-					  resource_size_t),
+		resource_alignf alignf,
 		void *alignf_data)
 {
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 14f5cfabbbc8..ce02f45e9c2c 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -188,6 +188,23 @@ enum {
 #define DEFINE_RES_DMA(_dma)						\
 	DEFINE_RES_DMA_NAMED((_dma), NULL)
 
+/**
+ * resource_alignf - Resource alignment callback
+ * @data:	Private data used by the callback
+ * @res:	Resource candidate range (an empty resource slot)
+ * @size:	The minimum size of the empty slot
+ * @align:	Alignment from the constraints
+ *
+ * Callback allows calculating resource placement and alignment beyond min,
+ * max, and align fields in the struct resource_constraint.
+ *
+ * Return: Start address for the resource.
+ */
+typedef resource_size_t (*resource_alignf)(void *data,
+					   const struct resource *res,
+					   resource_size_t size,
+					   resource_size_t align);
+
 /* PC/ISA/whatever - the normal PC address spaces: IO and memory */
 extern struct resource ioport_resource;
 extern struct resource iomem_resource;
@@ -207,10 +224,7 @@ extern void arch_remove_reservations(struct resource *avail);
 extern int allocate_resource(struct resource *root, struct resource *new,
 			     resource_size_t size, resource_size_t min,
 			     resource_size_t max, resource_size_t align,
-			     resource_size_t (*alignf)(void *,
-						       const struct resource *,
-						       resource_size_t,
-						       resource_size_t),
+			     resource_alignf alignf,
 			     void *alignf_data);
 struct resource *lookup_resource(struct resource *root, resource_size_t start);
 int adjust_resource(struct resource *res, resource_size_t start,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 60ca768bc867..d635e64debd9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1546,10 +1546,7 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 			struct resource *res, resource_size_t size,
 			resource_size_t align, resource_size_t min,
 			unsigned long type_mask,
-			resource_size_t (*alignf)(void *,
-						  const struct resource *,
-						  resource_size_t,
-						  resource_size_t),
+			resource_alignf alignf,
 			void *alignf_data);
 
 
diff --git a/kernel/resource.c b/kernel/resource.c
index ed4bb8ad701a..9d7920104120 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -60,8 +60,7 @@ EXPORT_SYMBOL(iomem_resource);
  */
 struct resource_constraint {
 	resource_size_t min, max, align;
-	resource_size_t (*alignf)(void *, const struct resource *,
-			resource_size_t, resource_size_t);
+	resource_alignf alignf;
 	void *alignf_data;
 };
 
@@ -725,10 +724,7 @@ static int reallocate_resource(struct resource *root, struct resource *old,
 int allocate_resource(struct resource *root, struct resource *new,
 		      resource_size_t size, resource_size_t min,
 		      resource_size_t max, resource_size_t align,
-		      resource_size_t (*alignf)(void *,
-						const struct resource *,
-						resource_size_t,
-						resource_size_t),
+		      resource_alignf alignf,
 		      void *alignf_data)
 {
 	int err;
-- 
2.30.2

[PATCH 5/7] resource: Handle simple alignment inside __find_empty_resource_slot()

[Ilpo Järvinen]

allocate_resource() accepts alignf callback to perform custom alignment
beyond constraint->align. If alignf is NULL, simple_align_resource() is
used which only returns avail->start (no change).

Using avail->start directly is natural and can be done with a
conditional in __find_empty_resource_slot() instead which avoids
unnecessarily using callback. It makes the code inside
__find_empty_resource_slot() more obvious and removes the need for the
caller to provide constraint->alignf unnecessarily.

This is preparation for exporting find_empty_resource_slot().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 kernel/resource.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 9d7920104120..80d64b6467b3 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -563,14 +563,6 @@ void __weak arch_remove_reservations(struct resource *avail)
 {
 }
 
-static resource_size_t simple_align_resource(void *data,
-					     const struct resource *avail,
-					     resource_size_t size,
-					     resource_size_t align)
-{
-	return avail->start;
-}
-
 static void resource_clip(struct resource *res, resource_size_t min,
 			  resource_size_t max)
 {
@@ -590,6 +582,7 @@ static int __find_empty_resource_slot(struct resource *root, struct resource *ol
 {
 	struct resource *this = root->child;
 	struct resource tmp = *new, avail, alloc;
+	resource_alignf alignf = constraint->alignf;
 
 	tmp.start = root->start;
 	/*
@@ -618,8 +611,13 @@ static int __find_empty_resource_slot(struct resource *root, struct resource *ol
 		avail.flags = new->flags & ~IORESOURCE_UNSET;
 		if (avail.start >= tmp.start) {
 			alloc.flags = avail.flags;
-			alloc.start = constraint->alignf(constraint->alignf_data, &avail,
-					size, constraint->align);
+			if (alignf) {
+				alloc.start = alignf(constraint->alignf_data,
+						     &avail, size,
+						     constraint->align);
+			} else {
+				alloc.start = avail.start;
+			}
 			alloc.end = alloc.start + size - 1;
 			if (alloc.start <= alloc.end &&
 			    resource_contains(&avail, &alloc)) {
@@ -730,9 +728,6 @@ int allocate_resource(struct resource *root, struct resource *new,
 	int err;
 	struct resource_constraint constraint;
 
-	if (!alignf)
-		alignf = simple_align_resource;
-
 	constraint.min = min;
 	constraint.max = max;
 	constraint.align = align;
-- 
2.30.2

[PATCH 6/7] resource: Export find_empty_resource_slot()

[Ilpo Järvinen]

PCI bridge window logic needs to find out in advance to the actual
allocation if there is an empty slot big enough to fit the window.

Export find_empty_resource_slot() for the purpose. Also move the struct
resource_constraint into generic header to be able to use the new
interface.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 include/linux/ioport.h | 22 ++++++++++++++++++++++
 kernel/resource.c      | 25 ++++---------------------
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index ce02f45e9c2c..0e911393dfb2 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -205,6 +205,24 @@ typedef resource_size_t (*resource_alignf)(void *data,
 					   resource_size_t size,
 					   resource_size_t align);
 
+/**
+ * resource_constraint - constraints to be met while searching empty resource slots
+ * @min:		The minimum address for the memory range
+ * @max:		The maximum address for the memory range
+ * @align:		Alignment for the start address of the empty slot
+ * @alignf:		Additional alignment constraints callback
+ * @alignf_data:	Data provided for @alignf callback
+ *
+ * Contains the range and alignment constraints that have to be met during
+ * find_empty_resource_slot(). @alignf can be NULL indicating no alignment
+ * beyond @align is necessary.
+ */
+struct resource_constraint {
+	resource_size_t min, max, align;
+	resource_alignf alignf;
+	void *alignf_data;
+};
+
 /* PC/ISA/whatever - the normal PC address spaces: IO and memory */
 extern struct resource ioport_resource;
 extern struct resource iomem_resource;
@@ -278,6 +296,10 @@ static inline bool resource_union(const struct resource *r1, const struct resour
 	return true;
 }
 
+int find_empty_resource_slot(struct resource *root, struct resource *new,
+			     resource_size_t size,
+			     struct resource_constraint *constraint);
+
 /* Convenience shorthand with allocation */
 #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
 #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
diff --git a/kernel/resource.c b/kernel/resource.c
index 80d64b6467b3..2dfe04ba24f3 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -46,24 +46,6 @@ struct resource iomem_resource = {
 };
 EXPORT_SYMBOL(iomem_resource);
 
-/**
- * resource_constraint - constraints to be met while searching empty resource slots
- * @min:		The minimum address for the memory range
- * @max:		The maximum address for the memory range
- * @align:		Alignment for the start address of the empty slot
- * @alignf:		Additional alignment constraints callback
- * @alignf_data:	Data provided for @alignf callback
- *
- * Contains the range and alignment constraints that have to be met during
- * find_empty_resource_slot(). @alignf can be NULL indicating no alignment
- * beyond @align is necessary.
- */
-struct resource_constraint {
-	resource_size_t min, max, align;
-	resource_alignf alignf;
-	void *alignf_data;
-};
-
 static DEFINE_RWLOCK(resource_lock);
 
 static struct resource *next_resource(struct resource *p, bool skip_children)
@@ -651,12 +633,13 @@ next:		if (!this || this->end == root->end)
  * * %0		- if successful, @new members start, end, and flags are altered.
  * * %-EBUSY	- if no empty slot was found.
  */
-static int find_empty_resource_slot(struct resource *root, struct resource *new,
-				    resource_size_t size,
-				    struct resource_constraint *constraint)
+int find_empty_resource_slot(struct resource *root, struct resource *new,
+			     resource_size_t size,
+			     struct resource_constraint *constraint)
 {
 	return  __find_empty_resource_slot(root, NULL, new, size, constraint);
 }
+EXPORT_SYMBOL_GPL(find_empty_resource_slot);
 
 /**
  * reallocate_resource - allocate a slot in the resource tree given range & alignment.
-- 
2.30.2

[PATCH 7/7] PCI: Relax bridge window tail sizing rules

[Ilpo Järvinen]

During remove & rescan cycle, PCI subsystem will recalculate and adjust
the bridge window sizing that was initially done by "BIOS". The size
calculation is based on the required alignment of the largest resource
among the downstream resources as per pbus_size_mem() (unimportant or
zero parameters marked with "..."):

	min_align = calculate_mem_align(aligns, max_order);
	size0 = calculate_memsize(size, ..., min_align);

and then in calculate_memsize():
	size = ALIGN(max(size, ...) + ..., align);

If the original bridge window sizing tried to conserve space, this will
lead to massive increase of the required bridge window size when the
downstream has a large disparity in BAR sizes. E.g., with 16MiB and
16GiB BARs this results in 32GiB bridge window size even if 16MiB BAR
does not require gigabytes of space to fit.

When doing remove & rescan for a bus that contains such a PCI device, a
larger bridge window is suddenly required on rescan but when there is a
bridge window upstream that is already assigned based on the original
size, it cannot be enlarged to the new requirement. This causes the
allocation of the bridge window to fail (0x600000000 > 0x400ffffff):

pci 0000:02:01.0: PCI bridge to [bus 03]
pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
pci 0000:01:00.0: PCI bridge to [bus 02-04]
pci 0000:01:00.0:   bridge window [mem 0x40400000-0x406fffff]
pci 0000:01:00.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
...
pci_bus 0000:03: busn_res: [bus 03] is released
pci 0000:03:00.0: reg 0x10: [mem 0x6400000000-0x6400ffffff 64bit pref]
pci 0000:03:00.0: reg 0x18: [mem 0x6000000000-0x63ffffffff 64bit pref]
pci 0000:03:00.0: reg 0x30: [mem 0x40400000-0x405fffff pref]
pci 0000:02:01.0: PCI bridge to [bus 03]
pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
pci 0000:02:01.0: BAR 9: no space for [mem size 0x600000000 64bit pref]
pci 0000:02:01.0: BAR 9: failed to assign [mem size 0x600000000 64bit pref]
pci 0000:02:01.0: BAR 8: assigned [mem 0x40400000-0x405fffff]
pci 0000:03:00.0: BAR 2: no space for [mem size 0x400000000 64bit pref]
pci 0000:03:00.0: BAR 2: failed to assign [mem size 0x400000000 64bit pref]
pci 0000:03:00.0: BAR 0: no space for [mem size 0x01000000 64bit pref]
pci 0000:03:00.0: BAR 0: failed to assign [mem size 0x01000000 64bit pref]
pci 0000:03:00.0: BAR 6: assigned [mem 0x40400000-0x405fffff pref]
pci 0000:02:01.0: PCI bridge to [bus 03]
pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]

This is a major surprise for users who are suddenly left with a PCIe
device that was working fine with the original bridge window sizing.

Even if the already assigned bridge window could be enlarged by
reallocation in some cases (something the current code does not attempt
to do), it is not possible in general case and the large amount of
wasted space at the tail of the bridge window may lead to other
resource exhaustion problems on Root Complex level (think of multiple
PCIe cards with VFs and BAR size disparity in a single system).

PCI specifications only expect natural alignment for BARs (PCI Express
Base Specification, rev. 6.1 sect. 7.5.1.2.1) and minimum of 1MiB
alignment for the bridge window (PCI Express Base Specification,
rev 6.1 sect. 7.5.1.3). The current bridge window tail alignment rule
was introduced in the commit 5d0a8965aea9 ("[PATCH] 2.5.14: New PCI
allocation code (alpha, arm, parisc) [2/2]") that only states:
"pbus_size_mem: core stuff; tested with randomly generated sets of
resources". It does not explain the motivation for the extra tail space
allocated that is not truly needed by the downstream resources. As
such, it is far from clear if it ever has been required by any HW.

To prevent PCIe cards with BAR size disparity from becoming unusable
after remove & rescan cycle, attempt to do a truly minimal allocation
for memory resources if needed. First check if the normally calculated
bridge window will not fit into an already assigned upstream resource.
In such case, try with relaxed bridge window tail sizing rules instead
where no extra tail space is requested beyond what the downstream
resources require. Only enforce the alignment requirement of the bridge
window itself (normally 1MiB).

With this patch, the resources are successfully allocated:

pci 0000:02:01.0: PCI bridge to [bus 03]
pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
pci 0000:02:01.0: bridge window [mem 0x6000000000-0x6400ffffff 64bit pref] to [bus 03] requires relaxed alignment rules
pci 0000:02:01.0: BAR 9: assigned [mem 0x6000000000-0x6400ffffff 64bit pref]
pci 0000:02:01.0: BAR 8: assigned [mem 0x40400000-0x405fffff]
pci 0000:03:00.0: BAR 2: assigned [mem 0x6000000000-0x63ffffffff 64bit pref]
pci 0000:03:00.0: BAR 0: assigned [mem 0x6400000000-0x6400ffffff 64bit pref]
pci 0000:03:00.0: BAR 6: assigned [mem 0x40400000-0x405fffff pref]
pci 0000:02:01.0: PCI bridge to [bus 03]
pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]

This patch draws inspiration from the initial investigations and work
by Mika Westerberg.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=216795
Link: https://lore.kernel.org/linux-pci/20190812144144.2646-1-mika.westerberg@linux.intel.com/
Fixes: 5d0a8965aea9 ("[PATCH] 2.5.14: New PCI allocation code (alpha, arm, parisc) [2/2]")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
---

During our internal review, Andy Shevchenko noted that the check for
ioport_resource & iomem_resource might not be needed. I've left them
into pbus_upstream_assigned_limit() because I couldn't prove myself
they're unnecessary and find_bus_resource_of_type() also does the
same check. If the checks are not needed in
pbus_upstream_assigned_limit() they should not be required in
find_bus_resource_of_type() either.
---
 drivers/pci/setup-bus.c | 75 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index e3e6ff8854a7..cafc43a1512d 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -960,6 +960,63 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
 	return min_align;
 }
 
+/**
+ * pbus_upstream_assigned_limit - Check no upstream resource limits allocation
+ * @bus:	The bus
+ * @mask:	Mask the resource flag, then compare it with type
+ * @type:	The type of resource from bridge
+ * @size:	The size required from the bridge window
+ * @align:	Required alignment for the resource
+ *
+ * Checks that @size can fit inside the upstream bridge resources that are
+ * already assigned.
+ *
+ * Return: -ENOSPC if @size cannot fit into an already assigned resource
+ * upstream resource.
+ */
+static int pbus_upstream_assigned_limit(struct pci_bus *bus, unsigned long mask,
+					unsigned long type, resource_size_t size,
+					resource_size_t align)
+{
+	struct resource_constraint constraint = { .max = (resource_size_t)~0ULL,
+						  .align = align };
+	struct pci_bus *downstream = bus;
+	struct resource *r;
+
+	while ((bus = bus->parent)) {
+		if (pci_is_root_bus(bus))
+			break;
+
+		pci_bus_for_each_resource(bus, r) {
+			if (!r || r == &ioport_resource || r == &iomem_resource)
+				continue;
+
+			if (!r->parent || (r->flags & mask) != type)
+				continue;
+
+			if (resource_size(r) >= size) {
+				struct resource gap = {};
+
+				if (!find_empty_resource_slot(r, &gap, size, &constraint))
+					return 0;
+			}
+
+			if (bus->self) {
+				pci_dbg(bus->self,
+					"Assigned bridge window %pR to %pR cannot fit 0x%llx required for %s bridging to %pR\n",
+					r, &bus->busn_res,
+					(unsigned long long)size,
+					pci_name(downstream->self),
+					&downstream->busn_res);
+			}
+
+			return -ENOSPC;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * pbus_size_mem() - Size the memory window of a given bus
  *
@@ -986,7 +1043,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			 struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
-	resource_size_t min_align, align, size, size0, size1;
+	resource_size_t min_align, win_align, align, size, size0, size1;
 	resource_size_t aligns[24]; /* Alignments from 1MB to 8TB */
 	int order, max_order;
 	struct resource *b_res = find_bus_resource_of_type(bus,
@@ -1064,10 +1121,24 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 		}
 	}
 
+	win_align = window_alignment(bus, b_res->flags);
 	min_align = calculate_mem_align(aligns, max_order);
-	min_align = max(min_align, window_alignment(bus, b_res->flags));
+	min_align = max(min_align, win_align);
 	size0 = calculate_memsize(size, min_size, 0, 0, resource_size(b_res), min_align);
 	add_align = max(min_align, add_align);
+
+	if (bus->self && size0 &&
+	    pbus_upstream_assigned_limit(bus, mask | IORESOURCE_PREFETCH, type,
+					 size0, add_align)) {
+		min_align = 1ULL << (max_order + 20);
+		min_align = max(min_align, win_align);
+		size0 = calculate_memsize(size, min_size, 0, 0,
+					  resource_size(b_res), win_align);
+		add_align = win_align;
+		pci_info(bus->self, "bridge window %pR to %pR requires relaxed alignment rules\n",
+			 b_res, &bus->busn_res);
+	}
+
 	size1 = (!realloc_head || (realloc_head && !add_size && !children_add_size)) ? size0 :
 		calculate_memsize(size, min_size, add_size, children_add_size,
 				resource_size(b_res), add_align);
-- 
2.30.2

Re: [PATCH 2/7] resource: Rename find_resource() to find_empty_resource_slot()

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 02:28:56PM +0200, Ilpo Järvinen wrote:
> Rename find_resource() to find_empty_resource_slot() to better describe
> what the functions does. This is a preparation for exposing it beyond
> resource.c which is needed by PCI core. Also rename the __ variant to
> match the names.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/7] resource: Document find_empty_resource_slot() and resource_constraint

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 02:28:57PM +0200, Ilpo Järvinen wrote:
> Document find_empty_resource_slot() and the struct resource_constraint
> as they are going to be exposed outside of resource.c.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] resource: Use typedef for alignf callback

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 02:28:58PM +0200, Ilpo Järvinen wrote:
> To make it simpler to declare resource constraint alignf callbacks, add
> typedef for it and document it.

Suggested-by?

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> +/**
> + * resource_alignf - Resource alignment callback
> + * @data:	Private data used by the callback
> + * @res:	Resource candidate range (an empty resource slot)
> + * @size:	The minimum size of the empty slot
> + * @align:	Alignment from the constraints
> + *
> + * Callback allows calculating resource placement and alignment beyond min,
> + * max, and align fields in the struct resource_constraint.
> + *
> + * Return: Start address for the resource.
> + */
> +typedef resource_size_t (*resource_alignf)(void *data,
> +					   const struct resource *res,
> +					   resource_size_t size,
> +					   resource_size_t align);

Never saw typedef kernel-doc before, so hopefully this will be rendered
just fine.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/7] resource: Handle simple alignment inside __find_empty_resource_slot()

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 02:28:59PM +0200, Ilpo Järvinen wrote:
> allocate_resource() accepts alignf callback to perform custom alignment

s/alignf callback/->alignf() callback/

?

> beyond constraint->align. If alignf is NULL, simple_align_resource() is
> used which only returns avail->start (no change).
> 
> Using avail->start directly is natural and can be done with a
> conditional in __find_empty_resource_slot() instead which avoids
> unnecessarily using callback. It makes the code inside
> __find_empty_resource_slot() more obvious and removes the need for the
> caller to provide constraint->alignf unnecessarily.
> 
> This is preparation for exporting find_empty_resource_slot().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

>  		if (avail.start >= tmp.start) {
>  			alloc.flags = avail.flags;
> -			alloc.start = constraint->alignf(constraint->alignf_data, &avail,
> -					size, constraint->align);
> +			if (alignf) {
> +				alloc.start = alignf(constraint->alignf_data,

> +						     &avail, size,
> +						     constraint->align);

Maybe one line? It used to be a longer line in the old code.

> +			} else {
> +				alloc.start = avail.start;
> +			}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] resource: Export find_empty_resource_slot()

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 02:29:00PM +0200, Ilpo Järvinen wrote:
> PCI bridge window logic needs to find out in advance to the actual
> allocation if there is an empty slot big enough to fit the window.
> 
> Export find_empty_resource_slot() for the purpose. Also move the struct
> resource_constraint into generic header to be able to use the new
> interface.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/7] PCI: Relax bridge window tail sizing rules

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 02:29:01PM +0200, Ilpo Järvinen wrote:
> During remove & rescan cycle, PCI subsystem will recalculate and adjust
> the bridge window sizing that was initially done by "BIOS". The size
> calculation is based on the required alignment of the largest resource
> among the downstream resources as per pbus_size_mem() (unimportant or
> zero parameters marked with "..."):
> 
> 	min_align = calculate_mem_align(aligns, max_order);
> 	size0 = calculate_memsize(size, ..., min_align);
> 
> and then in calculate_memsize():
> 	size = ALIGN(max(size, ...) + ..., align);
> 
> If the original bridge window sizing tried to conserve space, this will
> lead to massive increase of the required bridge window size when the
> downstream has a large disparity in BAR sizes. E.g., with 16MiB and
> 16GiB BARs this results in 32GiB bridge window size even if 16MiB BAR
> does not require gigabytes of space to fit.
> 
> When doing remove & rescan for a bus that contains such a PCI device, a
> larger bridge window is suddenly required on rescan but when there is a
> bridge window upstream that is already assigned based on the original
> size, it cannot be enlarged to the new requirement. This causes the
> allocation of the bridge window to fail (0x600000000 > 0x400ffffff):
> 
> pci 0000:02:01.0: PCI bridge to [bus 03]
> pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> pci 0000:01:00.0: PCI bridge to [bus 02-04]
> pci 0000:01:00.0:   bridge window [mem 0x40400000-0x406fffff]
> pci 0000:01:00.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> ...
> pci_bus 0000:03: busn_res: [bus 03] is released
> pci 0000:03:00.0: reg 0x10: [mem 0x6400000000-0x6400ffffff 64bit pref]
> pci 0000:03:00.0: reg 0x18: [mem 0x6000000000-0x63ffffffff 64bit pref]
> pci 0000:03:00.0: reg 0x30: [mem 0x40400000-0x405fffff pref]
> pci 0000:02:01.0: PCI bridge to [bus 03]
> pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> pci 0000:02:01.0: BAR 9: no space for [mem size 0x600000000 64bit pref]
> pci 0000:02:01.0: BAR 9: failed to assign [mem size 0x600000000 64bit pref]
> pci 0000:02:01.0: BAR 8: assigned [mem 0x40400000-0x405fffff]
> pci 0000:03:00.0: BAR 2: no space for [mem size 0x400000000 64bit pref]
> pci 0000:03:00.0: BAR 2: failed to assign [mem size 0x400000000 64bit pref]
> pci 0000:03:00.0: BAR 0: no space for [mem size 0x01000000 64bit pref]
> pci 0000:03:00.0: BAR 0: failed to assign [mem size 0x01000000 64bit pref]
> pci 0000:03:00.0: BAR 6: assigned [mem 0x40400000-0x405fffff pref]
> pci 0000:02:01.0: PCI bridge to [bus 03]
> pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> 
> This is a major surprise for users who are suddenly left with a PCIe
> device that was working fine with the original bridge window sizing.
> 
> Even if the already assigned bridge window could be enlarged by
> reallocation in some cases (something the current code does not attempt
> to do), it is not possible in general case and the large amount of
> wasted space at the tail of the bridge window may lead to other
> resource exhaustion problems on Root Complex level (think of multiple
> PCIe cards with VFs and BAR size disparity in a single system).
> 
> PCI specifications only expect natural alignment for BARs (PCI Express
> Base Specification, rev. 6.1 sect. 7.5.1.2.1) and minimum of 1MiB
> alignment for the bridge window (PCI Express Base Specification,
> rev 6.1 sect. 7.5.1.3). The current bridge window tail alignment rule
> was introduced in the commit 5d0a8965aea9 ("[PATCH] 2.5.14: New PCI
> allocation code (alpha, arm, parisc) [2/2]") that only states:
> "pbus_size_mem: core stuff; tested with randomly generated sets of
> resources". It does not explain the motivation for the extra tail space
> allocated that is not truly needed by the downstream resources. As
> such, it is far from clear if it ever has been required by any HW.
> 
> To prevent PCIe cards with BAR size disparity from becoming unusable
> after remove & rescan cycle, attempt to do a truly minimal allocation
> for memory resources if needed. First check if the normally calculated
> bridge window will not fit into an already assigned upstream resource.
> In such case, try with relaxed bridge window tail sizing rules instead
> where no extra tail space is requested beyond what the downstream
> resources require. Only enforce the alignment requirement of the bridge
> window itself (normally 1MiB).
> 
> With this patch, the resources are successfully allocated:
> 
> pci 0000:02:01.0: PCI bridge to [bus 03]
> pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> pci 0000:02:01.0: bridge window [mem 0x6000000000-0x6400ffffff 64bit pref] to [bus 03] requires relaxed alignment rules
> pci 0000:02:01.0: BAR 9: assigned [mem 0x6000000000-0x6400ffffff 64bit pref]
> pci 0000:02:01.0: BAR 8: assigned [mem 0x40400000-0x405fffff]
> pci 0000:03:00.0: BAR 2: assigned [mem 0x6000000000-0x63ffffffff 64bit pref]
> pci 0000:03:00.0: BAR 0: assigned [mem 0x6400000000-0x6400ffffff 64bit pref]
> pci 0000:03:00.0: BAR 6: assigned [mem 0x40400000-0x405fffff pref]
> pci 0000:02:01.0: PCI bridge to [bus 03]
> pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> 
> This patch draws inspiration from the initial investigations and work
> by Mika Westerberg.

...

> +	struct resource_constraint constraint = { .max = (resource_size_t)~0ULL,

RESOURCE_SIZE_MAX from limits.h.

> +						  .align = align };

Also I prefer the style

	struct resource_constraint constraint = {
		.max = RESOURCE_SIZE_MAX,
		.align = align,
	};


...

> +			if (!r || r == &ioport_resource || r == &iomem_resource)
> +				continue;

> +			if (!r->parent || (r->flags & mask) != type)

Thinking more about these checks, r->parent should be NULL for the root
resources, hence this check basically covers the second part of the above.

But like you said it's a material for a separate investigation.

> +				continue;

...

> +				pci_dbg(bus->self,
> +					"Assigned bridge window %pR to %pR cannot fit 0x%llx required for %s bridging to %pR\n",
> +					r, &bus->busn_res,

> +					(unsigned long long)size,

Yeah, casting here is a compromise between good looking message and
kernel code.

> +					pci_name(downstream->self),
> +					&downstream->busn_res);
> +			}

...

> +	    pbus_upstream_assigned_limit(bus, mask | IORESOURCE_PREFETCH, type,
> +					 size0, add_align)) {

One line?

...

> +		size0 = calculate_memsize(size, min_size, 0, 0,
> +					  resource_size(b_res), win_align);

One line?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/7] PCI: Relax bridge window tail sizing rules

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 6825 bytes --]

On Fri, 22 Dec 2023, Andy Shevchenko wrote:

> On Fri, Dec 22, 2023 at 02:29:01PM +0200, Ilpo Järvinen wrote:
> > During remove & rescan cycle, PCI subsystem will recalculate and adjust
> > the bridge window sizing that was initially done by "BIOS". The size
> > calculation is based on the required alignment of the largest resource
> > among the downstream resources as per pbus_size_mem() (unimportant or
> > zero parameters marked with "..."):
> > 
> > 	min_align = calculate_mem_align(aligns, max_order);
> > 	size0 = calculate_memsize(size, ..., min_align);
> > 
> > and then in calculate_memsize():
> > 	size = ALIGN(max(size, ...) + ..., align);
> > 
> > If the original bridge window sizing tried to conserve space, this will
> > lead to massive increase of the required bridge window size when the
> > downstream has a large disparity in BAR sizes. E.g., with 16MiB and
> > 16GiB BARs this results in 32GiB bridge window size even if 16MiB BAR
> > does not require gigabytes of space to fit.
> > 
> > When doing remove & rescan for a bus that contains such a PCI device, a
> > larger bridge window is suddenly required on rescan but when there is a
> > bridge window upstream that is already assigned based on the original
> > size, it cannot be enlarged to the new requirement. This causes the
> > allocation of the bridge window to fail (0x600000000 > 0x400ffffff):
> > 
> > pci 0000:02:01.0: PCI bridge to [bus 03]
> > pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> > pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> > pci 0000:01:00.0: PCI bridge to [bus 02-04]
> > pci 0000:01:00.0:   bridge window [mem 0x40400000-0x406fffff]
> > pci 0000:01:00.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> > ...
> > pci_bus 0000:03: busn_res: [bus 03] is released
> > pci 0000:03:00.0: reg 0x10: [mem 0x6400000000-0x6400ffffff 64bit pref]
> > pci 0000:03:00.0: reg 0x18: [mem 0x6000000000-0x63ffffffff 64bit pref]
> > pci 0000:03:00.0: reg 0x30: [mem 0x40400000-0x405fffff pref]
> > pci 0000:02:01.0: PCI bridge to [bus 03]
> > pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> > pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> > pci 0000:02:01.0: BAR 9: no space for [mem size 0x600000000 64bit pref]
> > pci 0000:02:01.0: BAR 9: failed to assign [mem size 0x600000000 64bit pref]
> > pci 0000:02:01.0: BAR 8: assigned [mem 0x40400000-0x405fffff]
> > pci 0000:03:00.0: BAR 2: no space for [mem size 0x400000000 64bit pref]
> > pci 0000:03:00.0: BAR 2: failed to assign [mem size 0x400000000 64bit pref]
> > pci 0000:03:00.0: BAR 0: no space for [mem size 0x01000000 64bit pref]
> > pci 0000:03:00.0: BAR 0: failed to assign [mem size 0x01000000 64bit pref]
> > pci 0000:03:00.0: BAR 6: assigned [mem 0x40400000-0x405fffff pref]
> > pci 0000:02:01.0: PCI bridge to [bus 03]
> > pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> > 
> > This is a major surprise for users who are suddenly left with a PCIe
> > device that was working fine with the original bridge window sizing.
> > 
> > Even if the already assigned bridge window could be enlarged by
> > reallocation in some cases (something the current code does not attempt
> > to do), it is not possible in general case and the large amount of
> > wasted space at the tail of the bridge window may lead to other
> > resource exhaustion problems on Root Complex level (think of multiple
> > PCIe cards with VFs and BAR size disparity in a single system).
> > 
> > PCI specifications only expect natural alignment for BARs (PCI Express
> > Base Specification, rev. 6.1 sect. 7.5.1.2.1) and minimum of 1MiB
> > alignment for the bridge window (PCI Express Base Specification,
> > rev 6.1 sect. 7.5.1.3). The current bridge window tail alignment rule
> > was introduced in the commit 5d0a8965aea9 ("[PATCH] 2.5.14: New PCI
> > allocation code (alpha, arm, parisc) [2/2]") that only states:
> > "pbus_size_mem: core stuff; tested with randomly generated sets of
> > resources". It does not explain the motivation for the extra tail space
> > allocated that is not truly needed by the downstream resources. As
> > such, it is far from clear if it ever has been required by any HW.
> > 
> > To prevent PCIe cards with BAR size disparity from becoming unusable
> > after remove & rescan cycle, attempt to do a truly minimal allocation
> > for memory resources if needed. First check if the normally calculated
> > bridge window will not fit into an already assigned upstream resource.
> > In such case, try with relaxed bridge window tail sizing rules instead
> > where no extra tail space is requested beyond what the downstream
> > resources require. Only enforce the alignment requirement of the bridge
> > window itself (normally 1MiB).
> > 
> > With this patch, the resources are successfully allocated:
> > 
> > pci 0000:02:01.0: PCI bridge to [bus 03]
> > pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> > pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> > pci 0000:02:01.0: bridge window [mem 0x6000000000-0x6400ffffff 64bit pref] to [bus 03] requires relaxed alignment rules
> > pci 0000:02:01.0: BAR 9: assigned [mem 0x6000000000-0x6400ffffff 64bit pref]
> > pci 0000:02:01.0: BAR 8: assigned [mem 0x40400000-0x405fffff]
> > pci 0000:03:00.0: BAR 2: assigned [mem 0x6000000000-0x63ffffffff 64bit pref]
> > pci 0000:03:00.0: BAR 0: assigned [mem 0x6400000000-0x6400ffffff 64bit pref]
> > pci 0000:03:00.0: BAR 6: assigned [mem 0x40400000-0x405fffff pref]
> > pci 0000:02:01.0: PCI bridge to [bus 03]
> > pci 0000:02:01.0:   bridge window [mem 0x40400000-0x405fffff]
> > pci 0000:02:01.0:   bridge window [mem 0x6000000000-0x6400ffffff 64bit pref]
> > 
> > This patch draws inspiration from the initial investigations and work
> > by Mika Westerberg.

> > +			if (!r || r == &ioport_resource || r == &iomem_resource)
> > +				continue;
> 
> > +			if (!r->parent || (r->flags & mask) != type)
> 
> Thinking more about these checks, r->parent should be NULL for the root
> resources, hence this check basically covers the second part of the above.
>
> But like you said it's a material for a separate investigation.

This new argument, however, is more convincing than the one you presented 
earlier. It is indeed true that even if we'd somehow end up into one of 
those root resources, its ->parent should be NULL (which differentiates 
this code from the other function which doesn't have similar ->parent 
check).

Also, the code has evolved to stop at the Root Port so it's another thing 
which should prevent even reaching that far.

> > +	    pbus_upstream_assigned_limit(bus, mask | IORESOURCE_PREFETCH, type,
> > +					 size0, add_align)) {
> 
> One line?

I don't think so, 101 chars.

-- 
 i.

Re: [PATCH 4/7] resource: Use typedef for alignf callback

[Ilpo Järvinen]

On Fri, 22 Dec 2023, Andy Shevchenko wrote:

> > +/**
> > + * resource_alignf - Resource alignment callback
> > + * @data:	Private data used by the callback
> > + * @res:	Resource candidate range (an empty resource slot)
> > + * @size:	The minimum size of the empty slot
> > + * @align:	Alignment from the constraints
> > + *
> > + * Callback allows calculating resource placement and alignment beyond min,
> > + * max, and align fields in the struct resource_constraint.
> > + *
> > + * Return: Start address for the resource.
> > + */
> > +typedef resource_size_t (*resource_alignf)(void *data,
> > +					   const struct resource *res,
> > +					   resource_size_t size,
> > +					   resource_size_t align);
> 
> Never saw typedef kernel-doc before, so hopefully this will be rendered
> just fine.

This was a good point. It seems that one has to prefix the name with 
typedef like this:

/**
 * typedef resource_alignf - Resource alignment callback

...otherwise scripts/kernel-doc attempts to parse it as a function 
kerneldoc.

-- 
 i.

Re: [PATCH 4/7] resource: Use typedef for alignf callback

[Randy Dunlap]

On 12/27/23 07:54, Ilpo Järvinen wrote:
> On Fri, 22 Dec 2023, Andy Shevchenko wrote:
> 
>>> +/**
>>> + * resource_alignf - Resource alignment callback
>>> + * @data:	Private data used by the callback
>>> + * @res:	Resource candidate range (an empty resource slot)
>>> + * @size:	The minimum size of the empty slot
>>> + * @align:	Alignment from the constraints
>>> + *
>>> + * Callback allows calculating resource placement and alignment beyond min,
>>> + * max, and align fields in the struct resource_constraint.
>>> + *
>>> + * Return: Start address for the resource.
>>> + */
>>> +typedef resource_size_t (*resource_alignf)(void *data,
>>> +					   const struct resource *res,
>>> +					   resource_size_t size,
>>> +					   resource_size_t align);
>>
>> Never saw typedef kernel-doc before, so hopefully this will be rendered
>> just fine.
> 
> This was a good point. It seems that one has to prefix the name with 
> typedef like this:

That's correct.
> 
> /**
>  * typedef resource_alignf - Resource alignment callback
> 
> ...otherwise scripts/kernel-doc attempts to parse it as a function 
> kerneldoc.

ack.

Thanks.

-- 
#Randy

Re: [PATCH 0/7] PCI: Solve two bridge window sizing issues

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

On Fri, 22 Dec 2023, Ilpo Järvinen wrote:

> Hi all,
> 
> Here's a series that contains two fixes to PCI bridge window sizing
> algorithm. Together, they should enable remove & rescan cycle to work
> for a PCI bus that has PCI devices with optional resources and/or
> disparity in BAR sizes.
> 
> For the second fix, I chose to expose find_empty_resource_slot() from
> kernel/resource.c because it should increase accuracy of the cannot-fit
> decision (currently that function is called find_resource()). In order
> to do that sensibly, a few improvements seemed in order to make its
> interface and name of the function sane before exposing it. Thus, the
> few extra patches on resource side.

Hi Bjorn,

Can you consider applying this series or do you have some comments on it?

I'm a bit unsure these days if my emails even reach you successfully as I 
tend to often receive complaints from Gmail that it has blocked the emails 
I send with git send-email detecting them as "unsolicited mail".

-- 
 i.


> Unfortunately I don't have a reason to suspect these would help with
> the issues related to the currently ongoing resource regression
> thread [1].
> 
> [1] https://lore.kernel.org/linux-pci/ZXpaNCLiDM+Kv38H@marvin.atrad.com.au/
> 
> Ilpo Järvinen (7):
>   PCI: Fix resource double counting on remove & rescan
>   resource: Rename find_resource() to find_empty_resource_slot()
>   resource: Document find_empty_resource_slot() and resource_constraint
>   resource: Use typedef for alignf callback
>   resource: Handle simple alignment inside __find_empty_resource_slot()
>   resource: Export find_empty_resource_slot()
>   PCI: Relax bridge window tail sizing rules

Re: [PATCH 0/7] PCI: Solve two bridge window sizing issues

[Bjorn Helgaas]

On Tue, Mar 05, 2024 at 05:37:49PM +0200, Ilpo Järvinen wrote:
> On Fri, 22 Dec 2023, Ilpo Järvinen wrote:
> 
> > Hi all,
> > 
> > Here's a series that contains two fixes to PCI bridge window sizing
> > algorithm. Together, they should enable remove & rescan cycle to work
> > for a PCI bus that has PCI devices with optional resources and/or
> > disparity in BAR sizes.
> > 
> > For the second fix, I chose to expose find_empty_resource_slot() from
> > kernel/resource.c because it should increase accuracy of the cannot-fit
> > decision (currently that function is called find_resource()). In order
> > to do that sensibly, a few improvements seemed in order to make its
> > interface and name of the function sane before exposing it. Thus, the
> > few extra patches on resource side.
> 
> Hi Bjorn,
> 
> Can you consider applying this series or do you have some comments on it?
> 
> I'm a bit unsure these days if my emails even reach you successfully as I 
> tend to often receive complaints from Gmail that it has blocked the emails 
> I send with git send-email detecting them as "unsolicited mail".

Sorry, I'm pretty sure I receive your emails since I read via lei and
if it appears on lore with linux-pci or me in the to/cc, I should see
it.  Just a little overwhelmed and struggling to prioritize and
respond.  But thanks for the ping; I will take a look.

Bjorn