[PATCH v2 0/3] PCI: Resource fitting algorith fixes

[PATCH v2 0/3] PCI: Resource fitting algorith fixes

[Ilpo Järvinen]

This series addresses three issues in the PCI resource fitting and
assignment algorithm.

v2:
- Add fix to resize problem (new patch)

Ilpo Järvinen (3):
  PCI: Relaxed tail alignment should never increase min_align
  PCI: Fix pdev_resources_assignable() disparity
  PCI: Fix failure detection during resource resize

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


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.39.5

[PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Ilpo Järvinen]

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

[PATCH v2 2/3] PCI: Fix pdev_resources_assignable() disparity

[Ilpo Järvinen]

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>
---
 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

[PATCH v2 3/3] PCI: Fix failure detection during resource resize

[Ilpo Järvinen]

Since the commit 96336ec70264 ("PCI: Perform reset_resource() and build
fail list in sync") the failed list is always built and returned to let
the caller decide what to do with the failures. The caller may want to
retry resource fitting and assignment and before that can happen, the
resources should be restored to their original state (a reset
effectively clears the struct resource), which requires returning them
on the failed list so that the original state remains stored in the
associated struct pci_dev_resource.

Resource resizing is different from the ordinary resource fitting and
assignment in that it only considers part of the resources. This means
failures for other resource types are not relevant at all and should be
ignored. As resize doesn't unassign such unrelated resources, those
resource ending up into the failed list implies assignment of that
resource must have failed before resize too. The check in
pci_reassign_bridge_resources() to decide if the whole assignment is
successful, however, is based on list emptiness which will cause false
negatives when the failed list has resources with an unrelated type.

If the failed list is not empty, call pci_required_resource_failed()
and extend it to be able to filter on specific resource types too (if
provided).

Calling pci_required_resource_failed() at this point is slightly
problematic because the resource itself is reset when the failed list
is constructed in __assign_resources_sorted(). As a result,
pci_resource_is_optional() does not have access to the original
resource flags. This could be worked around by restoring and
re-reseting the resource around the call to pci_resource_is_optional(),
however, it shouldn't cause issue as resource resizing is meant for
64-bit prefetchable resources according to Christian König (see the
Link which unfortunately doesn't point directly to Christian's reply
because lore didn't store that email at all).

Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
Link: https://lore.kernel.org/all/c5d1b5d8-8669-5572-75a7-0b480f581ac1@linux.intel.com/
Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
Tested-by: D Scott Phillips <scott@os.amperecomputing.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: D Scott Phillips <scott@os.amperecomputing.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: <stable@vger.kernel.org>
---
 drivers/pci/setup-bus.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 24863d8d0053..dbbd80d78d3d 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -28,6 +28,10 @@
 #include <linux/acpi.h>
 #include "pci.h"
 
+#define PCI_RES_TYPE_MASK \
+	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
+	 IORESOURCE_MEM_64)
+
 unsigned int pci_flags;
 EXPORT_SYMBOL_GPL(pci_flags);
 
@@ -384,13 +388,19 @@ static bool pci_need_to_release(unsigned long mask, struct resource *res)
 }
 
 /* Return: @true if assignment of a required resource failed. */
-static bool pci_required_resource_failed(struct list_head *fail_head)
+static bool pci_required_resource_failed(struct list_head *fail_head,
+					 unsigned long type)
 {
 	struct pci_dev_resource *fail_res;
 
+	type &= PCI_RES_TYPE_MASK;
+
 	list_for_each_entry(fail_res, fail_head, list) {
 		int idx = pci_resource_num(fail_res->dev, fail_res->res);
 
+		if (type && (fail_res->flags & PCI_RES_TYPE_MASK) != type)
+			continue;
+
 		if (!pci_resource_is_optional(fail_res->dev, idx))
 			return true;
 	}
@@ -504,7 +514,7 @@ static void __assign_resources_sorted(struct list_head *head,
 	}
 
 	/* Without realloc_head and only optional fails, nothing more to do. */
-	if (!pci_required_resource_failed(&local_fail_head) &&
+	if (!pci_required_resource_failed(&local_fail_head, 0) &&
 	    list_empty(realloc_head)) {
 		list_for_each_entry(save_res, &save_head, list) {
 			struct resource *res = save_res->res;
@@ -1708,10 +1718,6 @@ static void __pci_bridge_assign_resources(const struct pci_dev *bridge,
 	}
 }
 
-#define PCI_RES_TYPE_MASK \
-	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
-	 IORESOURCE_MEM_64)
-
 static void pci_bridge_release_resources(struct pci_bus *bus,
 					 unsigned long type)
 {
@@ -2449,8 +2455,12 @@ int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
 		free_list(&added);
 
 	if (!list_empty(&failed)) {
-		ret = -ENOSPC;
-		goto cleanup;
+		if (pci_required_resource_failed(&failed, type)) {
+			ret = -ENOSPC;
+			goto cleanup;
+		}
+		/* Only resources with unrelated types failed (again) */
+		free_list(&failed);
 	}
 
 	list_for_each_entry(dev_res, &saved, list) {
-- 
2.39.5

Re: [PATCH v2 0/3] PCI: Resource fitting algorith fixes

[Ilpo Järvinen]

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

On Mon, 30 Jun 2025, Ilpo Järvinen wrote:

> This series addresses three issues in the PCI resource fitting and
> assignment algorithm.
> 
> v2:
> - Add fix to resize problem (new patch)
> 
> Ilpo Järvinen (3):
>   PCI: Relaxed tail alignment should never increase min_align
>   PCI: Fix pdev_resources_assignable() disparity
>   PCI: Fix failure detection during resource resize
> 
>  drivers/pci/setup-bus.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)

Hi Bjorn,

You might have perhaps forgotten this series that addresses issues people 
have reported over the last few months.

-- 
 i.

Re: [PATCH v2 0/3] PCI: Resource fitting algorith fixes

[Ilpo Järvinen]

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

On Wed, 23 Jul 2025, Ilpo Järvinen wrote:
> On Mon, 30 Jun 2025, Ilpo Järvinen wrote:
> 
> > This series addresses three issues in the PCI resource fitting and
> > assignment algorithm.
> > 
> > v2:
> > - Add fix to resize problem (new patch)
> > 
> > Ilpo Järvinen (3):
> >   PCI: Relaxed tail alignment should never increase min_align
> >   PCI: Fix pdev_resources_assignable() disparity
> >   PCI: Fix failure detection during resource resize
> > 
> >  drivers/pci/setup-bus.c | 38 ++++++++++++++++++++++++++------------
> >  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> Hi Bjorn,
> 
> You might have perhaps forgotten this series that addresses issues people 
> have reported over the last few months.

Hi again,

Another ping.

-- 
 i.

Re: [PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Bjorn Helgaas]

On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> 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>

Was there a regression report URL we could include here?

> 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
> 

Re: [PATCH v2 2/3] PCI: Fix pdev_resources_assignable() disparity

[Bjorn Helgaas]

On Mon, Jun 30, 2025 at 05:26:40PM +0300, Ilpo Järvinen wrote:
> 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):

Is the class relevant here?

> 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>

Any URL we can include here for the report?

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> ---
>  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
> 

Re: [PATCH v2 3/3] PCI: Fix failure detection during resource resize

[Bjorn Helgaas]

On Mon, Jun 30, 2025 at 05:26:41PM +0300, Ilpo Järvinen wrote:
> Since the commit 96336ec70264 ("PCI: Perform reset_resource() and build
> fail list in sync") the failed list is always built and returned to let
> the caller decide what to do with the failures. The caller may want to
> retry resource fitting and assignment and before that can happen, the
> resources should be restored to their original state (a reset
> effectively clears the struct resource), which requires returning them
> on the failed list so that the original state remains stored in the
> associated struct pci_dev_resource.
> 
> Resource resizing is different from the ordinary resource fitting and
> assignment in that it only considers part of the resources. This means
> failures for other resource types are not relevant at all and should be
> ignored. As resize doesn't unassign such unrelated resources, those
> resource ending up into the failed list implies assignment of that
> resource must have failed before resize too. The check in
> pci_reassign_bridge_resources() to decide if the whole assignment is
> successful, however, is based on list emptiness which will cause false
> negatives when the failed list has resources with an unrelated type.
> 
> If the failed list is not empty, call pci_required_resource_failed()
> and extend it to be able to filter on specific resource types too (if
> provided).
> 
> Calling pci_required_resource_failed() at this point is slightly
> problematic because the resource itself is reset when the failed list
> is constructed in __assign_resources_sorted(). As a result,
> pci_resource_is_optional() does not have access to the original
> resource flags. This could be worked around by restoring and
> re-reseting the resource around the call to pci_resource_is_optional(),
> however, it shouldn't cause issue as resource resizing is meant for
> 64-bit prefetchable resources according to Christian König (see the
> Link which unfortunately doesn't point directly to Christian's reply
> because lore didn't store that email at all).
> 
> Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
> Link: https://lore.kernel.org/all/c5d1b5d8-8669-5572-75a7-0b480f581ac1@linux.intel.com/
> Reported-by: D Scott Phillips <scott@os.amperecomputing.com>

Any URL we can include here?

> Tested-by: D Scott Phillips <scott@os.amperecomputing.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: D Scott Phillips <scott@os.amperecomputing.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/pci/setup-bus.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 24863d8d0053..dbbd80d78d3d 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -28,6 +28,10 @@
>  #include <linux/acpi.h>
>  #include "pci.h"
>  
> +#define PCI_RES_TYPE_MASK \
> +	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
> +	 IORESOURCE_MEM_64)
> +
>  unsigned int pci_flags;
>  EXPORT_SYMBOL_GPL(pci_flags);
>  
> @@ -384,13 +388,19 @@ static bool pci_need_to_release(unsigned long mask, struct resource *res)
>  }
>  
>  /* Return: @true if assignment of a required resource failed. */
> -static bool pci_required_resource_failed(struct list_head *fail_head)
> +static bool pci_required_resource_failed(struct list_head *fail_head,
> +					 unsigned long type)
>  {
>  	struct pci_dev_resource *fail_res;
>  
> +	type &= PCI_RES_TYPE_MASK;
> +
>  	list_for_each_entry(fail_res, fail_head, list) {
>  		int idx = pci_resource_num(fail_res->dev, fail_res->res);
>  
> +		if (type && (fail_res->flags & PCI_RES_TYPE_MASK) != type)
> +			continue;
> +
>  		if (!pci_resource_is_optional(fail_res->dev, idx))
>  			return true;
>  	}
> @@ -504,7 +514,7 @@ static void __assign_resources_sorted(struct list_head *head,
>  	}
>  
>  	/* Without realloc_head and only optional fails, nothing more to do. */
> -	if (!pci_required_resource_failed(&local_fail_head) &&
> +	if (!pci_required_resource_failed(&local_fail_head, 0) &&
>  	    list_empty(realloc_head)) {
>  		list_for_each_entry(save_res, &save_head, list) {
>  			struct resource *res = save_res->res;
> @@ -1708,10 +1718,6 @@ static void __pci_bridge_assign_resources(const struct pci_dev *bridge,
>  	}
>  }
>  
> -#define PCI_RES_TYPE_MASK \
> -	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
> -	 IORESOURCE_MEM_64)
> -
>  static void pci_bridge_release_resources(struct pci_bus *bus,
>  					 unsigned long type)
>  {
> @@ -2449,8 +2455,12 @@ int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
>  		free_list(&added);
>  
>  	if (!list_empty(&failed)) {
> -		ret = -ENOSPC;
> -		goto cleanup;
> +		if (pci_required_resource_failed(&failed, type)) {
> +			ret = -ENOSPC;
> +			goto cleanup;
> +		}
> +		/* Only resources with unrelated types failed (again) */
> +		free_list(&failed);
>  	}
>  
>  	list_for_each_entry(dev_res, &saved, list) {
> -- 
> 2.39.5
> 

Re: [PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Ilpo Järvinen]

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

On Thu, 21 Aug 2025, Bjorn Helgaas wrote:

> On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> > 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>
> 
> Was there a regression report URL we could include here?

There's the Lore thread only:

https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/

(It's so far back that if there was something else, I've forgotten them 
by now but looking at the exchanges in the thread, it doesn't look like 
bugzilla entry or so made out of it.)

-- 
 i.

> > 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
> > 
> 

Re: [PATCH v2 0/3] PCI: Resource fitting algorith fixes

[Bjorn Helgaas]

On Mon, Jun 30, 2025 at 05:26:38PM +0300, Ilpo Järvinen wrote:
> This series addresses three issues in the PCI resource fitting and
> assignment algorithm.
> 
> v2:
> - Add fix to resize problem (new patch)
> 
> Ilpo Järvinen (3):
>   PCI: Relaxed tail alignment should never increase min_align
>   PCI: Fix pdev_resources_assignable() disparity
>   PCI: Fix failure detection during resource resize
> 
>  drivers/pci/setup-bus.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)

Applied to pci/resources for v6.18, thanks!  If you have any URLs or
other tweaks, I'll update with them.

Re: [PATCH v2 2/3] PCI: Fix pdev_resources_assignable() disparity

[Ilpo Järvinen]

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

On Thu, 21 Aug 2025, Bjorn Helgaas wrote:

> On Mon, Jun 30, 2025 at 05:26:40PM +0300, Ilpo Järvinen wrote:
> > 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):
> 
> Is the class relevant here?

It actually is, because pdev_resources_assignable() checks for it. In case 
of this particular device there was 0 there causing leading eventually to 
this internal sanity check problem.

> > 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>
> 
> Any URL we can include here for the report?

This was discussed in the thread of the original patch/series:

Link: https://lore.kernel.org/all/5f103643-5e1c-43c6-b8fe-9617d3b5447c@linaro.org/

> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  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
> > 
> 

-- 
 i.

Re: [PATCH v2 3/3] PCI: Fix failure detection during resource resize

[Ilpo Järvinen]

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

On Thu, 21 Aug 2025, Bjorn Helgaas wrote:
> On Mon, Jun 30, 2025 at 05:26:41PM +0300, Ilpo Järvinen wrote:
> > Since the commit 96336ec70264 ("PCI: Perform reset_resource() and build
> > fail list in sync") the failed list is always built and returned to let
> > the caller decide what to do with the failures. The caller may want to
> > retry resource fitting and assignment and before that can happen, the
> > resources should be restored to their original state (a reset
> > effectively clears the struct resource), which requires returning them
> > on the failed list so that the original state remains stored in the
> > associated struct pci_dev_resource.
> > 
> > Resource resizing is different from the ordinary resource fitting and
> > assignment in that it only considers part of the resources. This means
> > failures for other resource types are not relevant at all and should be
> > ignored. As resize doesn't unassign such unrelated resources, those
> > resource ending up into the failed list implies assignment of that
> > resource must have failed before resize too. The check in
> > pci_reassign_bridge_resources() to decide if the whole assignment is
> > successful, however, is based on list emptiness which will cause false
> > negatives when the failed list has resources with an unrelated type.
> > 
> > If the failed list is not empty, call pci_required_resource_failed()
> > and extend it to be able to filter on specific resource types too (if
> > provided).
> > 
> > Calling pci_required_resource_failed() at this point is slightly
> > problematic because the resource itself is reset when the failed list
> > is constructed in __assign_resources_sorted(). As a result,
> > pci_resource_is_optional() does not have access to the original
> > resource flags. This could be worked around by restoring and
> > re-reseting the resource around the call to pci_resource_is_optional(),
> > however, it shouldn't cause issue as resource resizing is meant for
> > 64-bit prefetchable resources according to Christian König (see the
> > Link which unfortunately doesn't point directly to Christian's reply
> > because lore didn't store that email at all).
> > 
> > Fixes: 96336ec70264 ("PCI: Perform reset_resource() and build fail list in sync")
> > Link: https://lore.kernel.org/all/c5d1b5d8-8669-5572-75a7-0b480f581ac1@linux.intel.com/
> > Reported-by: D Scott Phillips <scott@os.amperecomputing.com>
> 
> Any URL we can include here?

Again, it'sin the thread of the original patch:

Link: https://lore.kernel.org/all/86plf0lgit.fsf@scott-ph-mail.amperecomputing.com/

-- 
 i.

> > Tested-by: D Scott Phillips <scott@os.amperecomputing.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Reviewed-by: D Scott Phillips <scott@os.amperecomputing.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  drivers/pci/setup-bus.c | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 24863d8d0053..dbbd80d78d3d 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -28,6 +28,10 @@
> >  #include <linux/acpi.h>
> >  #include "pci.h"
> >  
> > +#define PCI_RES_TYPE_MASK \
> > +	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
> > +	 IORESOURCE_MEM_64)
> > +
> >  unsigned int pci_flags;
> >  EXPORT_SYMBOL_GPL(pci_flags);
> >  
> > @@ -384,13 +388,19 @@ static bool pci_need_to_release(unsigned long mask, struct resource *res)
> >  }
> >  
> >  /* Return: @true if assignment of a required resource failed. */
> > -static bool pci_required_resource_failed(struct list_head *fail_head)
> > +static bool pci_required_resource_failed(struct list_head *fail_head,
> > +					 unsigned long type)
> >  {
> >  	struct pci_dev_resource *fail_res;
> >  
> > +	type &= PCI_RES_TYPE_MASK;
> > +
> >  	list_for_each_entry(fail_res, fail_head, list) {
> >  		int idx = pci_resource_num(fail_res->dev, fail_res->res);
> >  
> > +		if (type && (fail_res->flags & PCI_RES_TYPE_MASK) != type)
> > +			continue;
> > +
> >  		if (!pci_resource_is_optional(fail_res->dev, idx))
> >  			return true;
> >  	}
> > @@ -504,7 +514,7 @@ static void __assign_resources_sorted(struct list_head *head,
> >  	}
> >  
> >  	/* Without realloc_head and only optional fails, nothing more to do. */
> > -	if (!pci_required_resource_failed(&local_fail_head) &&
> > +	if (!pci_required_resource_failed(&local_fail_head, 0) &&
> >  	    list_empty(realloc_head)) {
> >  		list_for_each_entry(save_res, &save_head, list) {
> >  			struct resource *res = save_res->res;
> > @@ -1708,10 +1718,6 @@ static void __pci_bridge_assign_resources(const struct pci_dev *bridge,
> >  	}
> >  }
> >  
> > -#define PCI_RES_TYPE_MASK \
> > -	(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH |\
> > -	 IORESOURCE_MEM_64)
> > -
> >  static void pci_bridge_release_resources(struct pci_bus *bus,
> >  					 unsigned long type)
> >  {
> > @@ -2449,8 +2455,12 @@ int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
> >  		free_list(&added);
> >  
> >  	if (!list_empty(&failed)) {
> > -		ret = -ENOSPC;
> > -		goto cleanup;
> > +		if (pci_required_resource_failed(&failed, type)) {
> > +			ret = -ENOSPC;
> > +			goto cleanup;
> > +		}
> > +		/* Only resources with unrelated types failed (again) */
> > +		free_list(&failed);
> >  	}
> >  
> >  	list_for_each_entry(dev_res, &saved, list) {
> > -- 
> > 2.39.5
> > 
> 

Re: [PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Ilpo Järvinen]

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

On Thu, 21 Aug 2025, Ilpo Järvinen wrote:

> On Thu, 21 Aug 2025, Bjorn Helgaas wrote:
> 
> > On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> > > 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>
> > 
> > Was there a regression report URL we could include here?
> 
> There's the Lore thread only:
> 
> https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/
> 
> (It's so far back that if there was something else, I've forgotten them 
> by now but looking at the exchanges in the thread, it doesn't look like 
> bugzilla entry or so made out of it.)

Making it "official" tag in case that's easier for you to handle 
automatically...

Link: https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/


-- 
 i.

Re: [PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Bjorn Helgaas]

On Thu, Aug 21, 2025 at 06:24:12PM +0300, Ilpo Järvinen wrote:
> On Thu, 21 Aug 2025, Ilpo Järvinen wrote:
> > On Thu, 21 Aug 2025, Bjorn Helgaas wrote:
> > > On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> > > > 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>
> > > 
> > > Was there a regression report URL we could include here?
> > 
> > There's the Lore thread only:
> > 
> > https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/

The email thread is fine and contains good information about how the
reporter tripped over it.

> > (It's so far back that if there was something else, I've forgotten them 
> > by now but looking at the exchanges in the thread, it doesn't look like 
> > bugzilla entry or so made out of it.)
> 
> Making it "official" tag in case that's easier for you to handle 
> automatically...
> 
> Link: https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/

Thanks for all of these, I added them to the commit logs.

Bjorn

Re: [PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Markus Elfring]

…
> Ensure min_align is not increased by the relaxed tail alignment.
…


…
+++ b/drivers/pci/setup-bus.c
…
@@ -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);
…

I wonder why a variable content would be overwritten here
without using the previous value.
https://cwe.mitre.org/data/definitions/563.html

Regards,
Markus

Re: [PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Ilpo Järvinen]

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

On Thu, 21 Aug 2025, Markus Elfring wrote:

> …
> > Ensure min_align is not increased by the relaxed tail alignment.
> …
> 
> 
> …
> +++ b/drivers/pci/setup-bus.c
> …
> @@ -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);
> …
> 
> I wonder why a variable content would be overwritten here
> without using the previous value.
> https://cwe.mitre.org/data/definitions/563.html

Hi Markus,

This looks a very good catch. I think it too should have been:

relaxed_align = max(relaxed_align, win_align);

...like in the other case.

-- 
 i.

Re: [PATCH v2 1/3] PCI: Relaxed tail alignment should never increase min_align

[Markus Elfring]

>> …
>> +++ b/drivers/pci/setup-bus.c
>> …
>> @@ -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);
>> …
>>
>> I wonder why a variable content would be overwritten here
>> without using the previous value.
>> https://cwe.mitre.org/data/definitions/563.html
…> This looks a very good catch. I think it too should have been:
> 
> relaxed_align = max(relaxed_align, win_align);
> 
> ...like in the other case.

Did any known source code analysis tools point such a questionable implementation detail out
for further development considerations?

Regards,
Markus