Re: [PATCH v2] PCI: release empty sibling bridge windows during rebar expansion

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

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

On Thu, 9 Apr 2026, Geramy Loveless wrote:

> When pbus_reassign_bridge_resources() walks up the bridge hierarchy
> to expand a window (e.g. for resizable BAR), it refuses to release
> any bridge window that has children.  This prevents BAR resize on
> devices behind multi-port PCIe switches (such as Thunderbolt docks)
> where empty sibling downstream ports hold small reservations that
> block the parent bridge window from being freed and re-sized.
> 
> Add pci_bus_subtree_empty() to check whether a bus subtree contains
> any assigned device BARs, and pci_bus_release_empty_bridges() to
> release bridge window resources of empty sibling bridges, saving
> them to the rollback list so failures can be properly unwound.
> 
> In pbus_reassign_bridge_resources(), call pci_bus_release_empty_bridges()
> before checking res->child, so empty sibling windows are cleared first
> and the parent window can then be released and grown.
> 
> Uses PCI bus/device iterators rather than walking the raw resource
> tree, which avoids issues with stale sibling pointers after resource
> release.

This paragraph can be dropped. And it's not exactly correct either as 
the pointers are only stale for resource entries that reside outside of 
the resource tree (after they've been released in a specific way) so if 
you start from a resource tree entry, you should never encounter a stale 
pointer.

> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Geramy Loveless <gloveless@jqluv.com>
> ---
>  drivers/pci/setup-bus.c | 99 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 4cf120ebe5a..7a182cd7e4d 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -2292,6 +2292,94 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
>  }
>  EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
>  
> +/*
> + * pci_bus_subtree_empty - Check whether a bus subtree has any assigned
> + * non-bridge device resources.
> + * @bus: PCI bus to check
> + *
> + * Returns true if no device on @bus or its descendant buses has any
> + * assigned BARs (bridge window resources are not considered).
> + */
> +static bool pci_bus_subtree_empty(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev;
> +
> +	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		struct resource *r;
> +		unsigned int i;
> +
> +		pci_dev_for_each_resource(dev, r, i) {
> +			if (i >= PCI_BRIDGE_RESOURCES)
> +				break;
> +			if (resource_assigned(r))
> +				return false;
> +		}
> +
> +		if (dev->subordinate &&
> +		    !pci_bus_subtree_empty(dev->subordinate))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * pci_bus_release_empty_bridges - Release bridge window resources of
> + * empty sibling bridges so the parent window can be freed and re-sized.
> + * @bus: PCI bus whose child bridges to scan
> + * @b_win: Parent bridge window resource; only children of this window
> + *         are released
> + * @saved: List to save released resources for rollback
> + *
> + * For each PCI-to-PCI bridge on @bus whose subtree is empty (no assigned
> + * device BARs), releases bridge window resources that are children of
> + * @b_win, saving them for rollback via @saved.
> + *
> + * Returns 0 on success, negative errno on failure.
> + */
> +static int pci_bus_release_empty_bridges(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;
> +
> +		if ((dev->class >> 8) != PCI_CLASS_BRIDGE_PCI)
> +			continue;

I suppose dev->subordinate check is enough for what we're doing so this 
looks redundant.

> +
> +		if (!pci_bus_subtree_empty(dev->subordinate))
> +			continue;
> +
> +		pci_dev_for_each_resource(dev, r, i) {
> +			int ret;
> +
> +			if (!pci_resource_is_bridge_win(i))
> +				continue;
> +
> +			if (!resource_assigned(r))
> +				continue;
> +
> +			if (r->parent != b_win)
> +				continue;
> +
> +			ret = pci_dev_res_add_to_list(saved, dev, r, 0, 0);
> +			if (ret)
> +				return ret;
> +
> +			release_child_resources(r);

Unfortunately you cannot call this low-level function because it 
recursively frees child resources which means you won't be able to 
rollback them as they were not added to the saved list.

I think the release algorithm should basically do this:

- Recurse to the subordinate buses
- Loop through bridge window resources of this bus
	- Skip resources that are not assigned or are not parented by b_win
	- If the resource still has childs, leave the resource alone
	  (+ log it for easier troubleshooting these cases; any failure
	     will also cascade to upstream so it may be possible to 
	     shortcut something but it will also make the algorithm more
	     complicated)
	- Save and free the resource

It might be better to move some of the code from 
pbus_reassign_bridge_resources() here as there's overlap with the sketched 
algorithm (but I'm not sure until I see the updated version but keep this 
in mind).

Doing pci_bus_subtree_empty() before any removal is fine with me, but I 
see it just an optimization.

> +			pci_release_resource(dev, i);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * 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 +2404,14 @@ 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 */

I don't know why you removed this comment (I admit though "BARs" could 
have been worded better as it's bridge windows we're dealing here).

> +		/* Release empty sibling bridge windows first */
> +		if (bridge->subordinate) {
> +			ret = pci_bus_release_empty_bridges(
> +					bridge->subordinate, res, saved);

First arg fits to the previous line.

Align the second line to (.

But consider also rearranging code as I mentioned above.

> +			if (ret)
> +				return ret;

Consider proceeding with the resize even if something failed as there are 
cases where the bridge windows are large enough (admittedly, you seem to 
only bail out in case of alloc error).

In to the same vein, there seems to be one existing goto restore (that was 
added by me), which could also probably do continue instead (but changing 
it would be worth another patch).

> +		}
> +
>  		if (!res->child) {
>  			ret = pci_dev_res_add_to_list(saved, bridge, res, 0, 0);
>  			if (ret)
> @@ -2327,7 +2422,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.