Re: [PATCH] PCI: release empty sibling bridge resources during window resize

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

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

On Fri, 10 Apr 2026, Geramy Loveless wrote:

Please try to remember to increment the version counter whenever 
resubmitting the patch (if there's even a slightest change to the patch).
This should have been v3 I think.

> pbus_reassign_bridge_resources() refuses to release bridge windows that
> have child resources.  On multi-port PCIe switches (e.g. Thunderbolt
> docks), empty sibling downstream ports hold small reservations that
> prevent the parent window from being freed and re-sized for rebar.
> 
> Add a bottom-up recursive release that saves and frees each empty bridge
> window individually, so the parent window can then be released and grown.

As this starts to get close being ready, put the parts of the log here 
which show the pinning problem so it's easier to find this with search 
engine. Only include relevant parts (things like timestamps and totally 
unrelated lines can be cut out).

If it seems reasonable in length, consider adding also the post change 
log.
 
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Geramy Loveless <gloveless@jqluv.com>
> ---

Please start adding version history of the patch here (what was changed 
compared with the previous version).

>  drivers/pci/setup-bus.c | 63 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 4cf120ebe5ad..0c1fb654c9cc 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -2292,6 +2292,60 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>  }
>  EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
>  
> +/*
> + * pci_bus_release_empty_bridge_resources - Bottom-up release of bridge
> + * window resources in empty subtrees.

This is summary, so I'd change "bridge window resources" to just "bridge 
windows"

> + * @bus: PCI bus whose child bridges to process
> + * @b_win: Ancestor bridge window; only children of this resource are released
> + * @saved: List to save released resources for rollback
> + *
> + * Recurses depth-first into subordinate buses, then releases bridge windows

... that are (grand)parented by @b_win

> + * on the way back up.  Each resource is individually saved before release so

individually saved -> stored into @saved

Nit, one whitespace is enough.

> + * the entire operation can be rolled back.
> + */
> +static void pci_bus_release_empty_bridge_resources(struct pci_bus *bus,
> +						   struct resource *b_win,
> +						   struct list_head *saved)
> +{
> +	struct pci_dev *dev;
> +
> +	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		struct resource *r;
> +		unsigned int i;
> +
> +		if (!dev->subordinate)
> +			continue;
> +
> +		/* Recurse first — release deepest resources before parents */
> +		pci_bus_release_empty_bridge_resources(dev->subordinate,
> +						      b_win, saved);
> +
> +		pci_dev_for_each_resource(dev, r, i) {
> +			if (!pci_resource_is_bridge_win(i))
> +				continue;
> +
> +			if (!resource_assigned(r))
> +				continue;
> +
> +			if (r->parent != b_win)

This has to also walk upstream as in nested topologies, b_win is not 
necessarily the immediate parent.

When converting to the suggested iteration form (see below), r itself can 
be b_win too but it's relatively easy to handle once you convert this into 
a while loop.

> +				continue;
> +


> +			if (r->child) {
> +				const char *res_name = pci_resource_name(dev, i);
> +
> +				pci_info(dev, "%s %pR: not released, children still present\n",
> +					 res_name, r);
> +				continue;
> +			}
> +
> +			if (pci_dev_res_add_to_list(saved, dev, r, 0, 0))
> +				continue;
> +
> +			pci_release_resource(dev, i);

These three things are effectively what the caller does right after 
calling this function so the current way to iterate duplicates the 
release to two places. This is what I meant when I said you may want to 
restructure things a bit.

I think this structure would allow you to remove the caller side release 
entirely:

	list_for_each_entry(dev, &bus->devices, bus_list) {
		... recursion ...
	}

	pci_bus_for_each_resource(bus, ...) {
		...checks and release here...
	}

I suspect you don't even need to check pci_resource_is_bridge_win() when 
iterating over them as bus resources.

> +		}
> +	}
> +}
> +
>  /*
>   * Walk to the root bus, find the bridge window relevant for @res and
>   * release it when possible. If the bridge window contains assigned
> @@ -2316,7 +2370,12 @@ static int pbus_reassign_bridge_resources(struct pci_bus *bus, struct resource *
>  
>  		i = pci_resource_num(bridge, res);
>  
> -		/* Ignore BARs which are still in use */
> +		/* Release empty sibling bridge windows bottom-up */
> +		if (bridge->subordinate)
> +			pci_bus_release_empty_bridge_resources(
> +				bridge->subordinate, res, saved);

If you call is pbus_... it is a bit shorter and you can fit the first arg 
on the first line.

Use braces with multi-line constructs.

> +
> +		/* Ignore bridge windows which are still in use */
>  		if (!res->child) {
>  			ret = pci_dev_res_add_to_list(saved, bridge, res, 0, 0);
>  			if (ret)
> @@ -2327,7 +2386,7 @@ static int pbus_reassign_bridge_resources(struct pci_bus *bus, struct resource *
>  			const char *res_name = pci_resource_name(bridge, i);
>  
>  			pci_warn(bridge,
> -				 "%s %pR: was not released (still contains assigned resources)\n",
> +				 "%s %pR: not released, active children present\n",
>  				 res_name, res);
>  		}
>  
> 

-- 
 i.