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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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