Re: [PATCH 24/25] PCI: Perform reset_resource() and build fail list in sync

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

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

On Tue, 3 Jun 2025, Tudor Ambarus wrote:
> On 6/3/25 11:48 AM, Tudor Ambarus wrote:
> > On 6/3/25 11:36 AM, Tudor Ambarus wrote:
> >> On 6/3/25 9:13 AM, Ilpo Järvinen wrote:
> >>> So please test if this patch solves your problem:
> >>
> >> It fails in a different way, the bridge window resource never gets
> >> assigned with the proposed patch.
> >>
> >> With the patch applied: https://termbin.com/h3w0
> > 
> > above is no revert and with the proposed fix. It also contains the
> > prints https://termbin.com/g4zn
> > 
> > It seems the prints in pbus_size_mem are not longer hit, likely because
> > of the new condition added: ``!pdev_resources_assignable(dev) ||``,
> > pci_dev_for_each_resource() finishes without doing anything.
> > 
> >> With the blamed commit reverted: https://termbin.com/3rh6
> > 
> 
> I think I found the inconsistency.
> 
> __pci_bus_assign_resources()
> 	pbus_assign_resources_sorted()
> 		pdev_sort_resources(dev, &head);
> 
> But pdev_sort_resources() is called with a newly LIST_HEAD(head), not
> with realloc_head, thus the resources never get sorted.

pdev_sort_resources() is not supposed to add resources into realloc_head 
but just collects all the relevant resources in the descending order by
size.

There are two main lists here. The head list contains all relevant 
resources we're going to process and realloc_head keeps track which of 
them are optional (or optional in part, that is, some resources have the 
base size and the optional size).

__assign_resources_sorted() will apply the optional sizes from 
realloc_head and re-sorts the head list while changing the sizes.
If not all resources can be assigned, rollback happens and base sizes are 
assigned first, and then reassign_resources_sorted() handles the 
realloc_head ones afterwards for as many resources as possible.

> pdev_sort_resources() exits early at
> 	``if (!pdev_resources_assignable(dev))``

Yes it does, for 0001:01:00.0.

-- 
 i.