[{"id":3683983,"web_url":"http://patchwork.ozlabs.org/comment/3683983/","msgid":"","list_archive_url":null,"date":"2026-04-29T10:34:19","subject":"Re: [PATCH v3 RESEND] PCI: release empty sibling bridge windows\n during window resize","submitter":{"id":83553,"url":"http://patchwork.ozlabs.org/api/people/83553/","name":"Ilpo Järvinen","email":"ilpo.jarvinen@linux.intel.com"},"content":"On Tue, 28 Apr 2026, Geramy Loveless wrote:\n\n> pbus_reassign_bridge_resources() refuses to release bridge windows that\n> have child resources. On multi-port PCIe switches (e.g. Thunderbolt\n> docks), empty sibling downstream ports hold small reservations that\n> prevent the parent window from being freed and re-sized for rebar.\n> \n> Walk descendants of the target window depth-first, saving and freeing\n> each empty bridge window so the parent can then be released and grown.\n> \n> Without this change, attempts to grow a window deep below a switch\n> fabric leave nested empty windows pinning the ancestor:\n> \n> pcieport 0000:65:00.0: bridge window [mem 0x8880000000-0x98800fffff\n> 64bit pref]: not released, active children present\n> pcieport 0000:00:03.2: bridge window [mem 0x8880000000-0x98800fffff\n> 64bit pref]: not released, active children present\n> \n> With the bottom-up walk and upstream ancestry check, the chain unwinds\n> and the rebar succeeds:\n> \n> pcieport 0000:96:00.0: bridge window [...]: releasing\n> pcieport 0000:95:00.0: bridge window [...]: releasing\n> pcieport 0000:94:00.0: bridge window [...]: releasing\n> pcieport 0000:65:00.0: bridge window [...]: releasing\n> amdgpu 0000:97:00.0: BAR 0 [mem 0x9000000000-0x97ffffffff 64bit pref]:\n> assigned\n> \n> Suggested-by: Ilpo Järvinen \n> Signed-off-by: Geramy Loveless \n> ---\n> RESEND: my earlier v3 (Message-ID 20260428225146.3104063-1-gloveless@jqluv.com)\n> was sent with stale code — the diff was byte-identical to v2 because the\n> v3 edits weren't staged before commit --amend. This resend contains the\n> actual v3 content described below. Apologies for the noise.\n\nHi,\n\nI already came to that conclusion before seeing this... It happens, no \nworries. :-)\n\n> v3:\n> - Restructure helper: outer loop only recurses; inner iterates bus\n> resources via pci_bus_for_each_resource(). The release path is now\n> unified, so the caller-side block in pbus_reassign_bridge_resources()\n> is gone.\n> - Walk the resource tree upstream when checking ancestry of @b_win,\n> so descendants in nested topologies are also released (Ilpo).\n> - Drop pci_resource_is_bridge_win() filter; bus resources are\n> inherently bridge windows.\n> - Rename pci_bus_release_empty_bridge_resources() to\n> pbus_release_empty_bridge_resources().\n> - Drop misleading \"Uses PCI bus/device iterators...\" paragraph from\n> commit message.\n> - Kerneldoc tweaks per Ilpo's review.\n> \n> v2:\n> - Use PCI bus/device iterators instead of walking raw resource tree.\n> - Save released resources to rollback list.\n> - Filter via pci_resource_is_bridge_win() (now removed in v3).\n> - Call release before the !res->child check.\n> - Check bridge->subordinate before recursion.\n> ---\n> drivers/pci/setup-bus.c | 78 ++++++++++++++++++++++++++++++++---------\n> 1 file changed, 61 insertions(+), 17 deletions(-)\n> \n> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c\n> index 4cf120ebe5a..b47dc1a2925 100644\n> --- a/drivers/pci/setup-bus.c\n> +++ b/drivers/pci/setup-bus.c\n> @@ -2292,6 +2292,63 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)\n> }\n> EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);\n> \n> +/*\n> + * pbus_release_empty_bridge_resources - Bottom-up release of bridge windows\n> + * in empty subtrees.\n> + * @bus: PCI bus whose child bridges to process\n> + * @b_win: Ancestor bridge window; only resources that are (grand)parented\n> + *\t by @b_win are released\n> + * @saved: List to store released resources into for rollback\n> + *\n> + * Recurses depth-first into subordinate buses, then releases bridge windows\n> + * that are (grand)parented by @b_win on the way back up. Each resource is\n> + * stored into @saved before release so the entire operation can be rolled\n> + * back.\n> + */\n> +static void pbus_release_empty_bridge_resources(struct pci_bus *bus,\n> +\t\t\t\t\t\tstruct resource *b_win,\n> +\t\t\t\t\t\tstruct list_head *saved)\n> +{\n> +\tstruct pci_dev *dev;\n> +\tstruct resource *r;\n> +\tunsigned int i;\n> +\n> +\tlist_for_each_entry(dev, &bus->devices, bus_list) {\n> +\t\tif (dev->subordinate)\n> +\t\t\tpbus_release_empty_bridge_resources(dev->subordinate,\n> +\t\t\t\t\t\t\t b_win, saved);\n> +\t}\n> +\n> +\tpci_bus_for_each_resource(bus, r, i) {\n\nWhile looking into i vs idx (which should be the same thing), I realized\nthere's one bad bug here:\n\npci_bus_for_each_resource() also iterates over subtractive decode bridge \nresources for which you cannot take pci_resource_num(bus->self, r) \nbecause that's invalid memorywise (they're resource of another struct \npci_dev). And passing idx then to pci_release_resource() is also invalid \n(it might actually \"work\" as the second bug might reverse the effects of \nthe first one but it's still really bad).\n\nWhat we want here is to only consider resources local to this bridge \n(bus->self) so we need to add:\n\n\t\tif (i < PCI_BRIDGE_RESOURCE_NUM)\n\t\t\tbreak;\n\nBUT, it might be worth adding a new iterator that doesn't have this trap:\n\n\tpci_bridge_for_each_window(bridge, r, ...)\n\n...There would be some use for it in the existing code as well so I've \nbeen considering adding it anyway to eliminate the various loop forms \nthat do this same iteration.\n\n> +\t\tstruct resource *p;\n> +\t\tunsigned int idx;\n> +\n> +\t\tif (!r || !resource_assigned(r))\n> +\t\t\tcontinue;\n> +\n> +\t\t/* Walk upstream to confirm r descends from (or equals) b_win */\n> +\t\tfor (p = r; p && p != b_win; p = p->parent)\n> +\t\t\t;\n> +\t\tif (!p)\n> +\t\t\tcontinue;\n\nCan you place this loop into a new function that returns bool. I believe \nit will also make the code easier to follow as you don't need p and can \nreturn immmediately on match. With a reasonable name, you don't need the \ncomment anymore as the naming explains what the code does.\n\nI've need for the same check myself (in some other changes I'm \ndeveloping), in preparation for that, I suggest placing the function under \npbus_select_window() as they're kind of related.\n\n> +\n> +\t\tidx = pci_resource_num(bus->self, r);\n\nThis can be dropped.\n\nYou can use i (+ PCI_BRIDGE_RESOURCES) in pci_release_resource() call.\n\n\nThe general structure of the code seems fine now.\n\n> +\n> +\t\tif (r->child) {\n> +\t\t\tconst char *res_name = pci_resource_name(bus->self, idx);\n> +\n> +\t\t\tpci_info(bus->self, \"%s %pR: not released, active children present\\n\",\n> +\t\t\t\t res_name, r);\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (pci_dev_res_add_to_list(saved, bus->self, r, 0, 0))\n> +\t\t\tcontinue;\n> +\n> +\t\tpci_release_resource(bus->self, idx);\n> +\t}\n> +}\n> +\n> /*\n> * Walk to the root bus, find the bridge window relevant for @res and\n> * release it when possible. If the bridge window contains assigned\n> @@ -2305,7 +2362,6 @@ static int pbus_reassign_bridge_resources(struct pci_bus *bus, struct resource *\n> \tstruct pci_dev *bridge = NULL;\n> \tLIST_HEAD(added);\n> \tLIST_HEAD(failed);\n> -\tunsigned int i;\n> \tint ret = 0;\n> \n> \twhile (!pci_is_root_bus(bus)) {\n> @@ -2314,22 +2370,10 @@ static int pbus_reassign_bridge_resources(struct pci_bus *bus, struct resource *\n> \t\tif (!res)\n> \t\t\tbreak;\n> \n> -\t\ti = pci_resource_num(bridge, res);\n> -\n> -\t\t/* Ignore BARs which are still in use */\n> -\t\tif (!res->child) {\n> -\t\t\tret = pci_dev_res_add_to_list(saved, bridge, res, 0, 0);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn ret;\n> -\n> -\t\t\tpci_release_resource(bridge, i);\n> -\t\t} else {\n> -\t\t\tconst char *res_name = pci_resource_name(bridge, i);\n> -\n> -\t\t\tpci_warn(bridge,\n> -\t\t\t\t \"%s %pR: was not released (still contains assigned resources)\\n\",\n> -\t\t\t\t res_name, res);\n> -\t\t}\n> +\t\t/* Release this bridge window and any empty descendants bottom-up */\n> +\t\tif (bridge->subordinate)\n> +\t\t\tpbus_release_empty_bridge_resources(bridge->subordinate,\n> +\t\t\t\t\t\t\t res, saved);\n> \n> \t\tbus = bus->parent;\n> \t}\n>","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256\n header.s=Intel header.b=SE4ymJhz;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c04:e001:36c::12fc:5321; helo=tor.lore.kernel.org;\n envelope-from=linux-pci+bounces-53381-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com\n header.b=\"SE4ymJhz\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=192.198.163.19","smtp.subspace.kernel.org;\n dmarc=pass (p=none dis=none) header.from=linux.intel.com","smtp.subspace.kernel.org;\n spf=pass smtp.mailfrom=linux.intel.com"],"Received":["from tor.lore.kernel.org (tor.lore.kernel.org\n [IPv6:2600:3c04:e001:36c::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4g5DFc1xmmz1yHX\n\tfor ; Wed, 29 Apr 2026 20:34:44 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby tor.lore.kernel.org (Postfix) with ESMTP id 10495302158A\n\tfor ; Wed, 29 Apr 2026 10:34:41 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 569C5382388;\n\tWed, 29 Apr 2026 10:34:40 +0000 (UTC)","from mgamail.intel.com (mgamail.intel.com [192.198.163.19])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 8AF9A39B4A6\n\tfor ; Wed, 29 Apr 2026 10:34:38 +0000 (UTC)","from orviesa006.jf.intel.com ([10.64.159.146])\n by fmvoesa113.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n 29 Apr 2026 03:34:38 -0700","from pgcooper-mobl3.ger.corp.intel.com (HELO localhost)\n ([10.245.245.212])\n by orviesa006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n 29 Apr 2026 03:34:31 -0700"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1777458880; cv=none;\n b=TW+lg2Z6Qt9IzK/vBrD7/l1aoOp6zzuXojYb5epReZXDiO5HWoVfsPlaRIaiipaq5XTjJHkd2JIMCOvUuBMa4ylKbwllWrWqHfHKa7Gd3blXn+wjWkOoNQSFw81X4WrAkj9AGuJOyHfH8bY2E5qyuLEBKw56thvklwx/pV6n6NE=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1777458880; c=relaxed/simple;\n\tbh=e9NkbKCzkNP4G7jvLwasOvurNANukm/JUadhuTmUFho=;\n\th=From:Date:To:cc:Subject:In-Reply-To:Message-ID:References:\n\t MIME-Version:Content-Type;\n b=H7E3rC/ZIOtU4zb0T+HYhlUhAWqFpmha4LEEv4XkvbxXWQ4LFonqxYZuQ7C5WdyvQetWrW/Vdqi/QzrDgHm1ecFjGYvNPyI927NAwClGex93IxcZmVbDloWxw+akDBvdEmxLT3rTnxUW3S3Iw6dbY0qpuD45ybwLfBC4gxAZFUg=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dmarc=pass (p=none dis=none) header.from=linux.intel.com;\n spf=pass smtp.mailfrom=linux.intel.com;\n dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com\n header.b=SE4ymJhz; arc=none smtp.client-ip=192.198.163.19","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n d=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n t=1777458878; x=1808994878;\n h=from:date:to:cc:subject:in-reply-to:message-id:\n references:mime-version;\n bh=e9NkbKCzkNP4G7jvLwasOvurNANukm/JUadhuTmUFho=;\n b=SE4ymJhzsA+KEQRIqYIULWjnfP2j3HjdfirHY5/hF66GcvfrAU4KHjbW\n 19+vKY2AN6kOiDV/tIp/zQ7U8DlNX1VYPf3WBgC3H3MzEQu01JfKKMjCw\n pdD2P9cD8GmtSrNfAaH43KssHGZKSxtDn9Gl4BJ6IYTiNXDaqVd5mgGwI\n SaxusxqueHGgFs9wzCvprM24k2LLh5kMDMKsT4Cztlxjr3l0tKyDinwHv\n zkDkLlx+lfs8AuPdN6omzLIJQlg5ZKGfP+kJDUj0BMfo2JT1KRiDH4Q8o\n RLMiAc181AN6GV/drG6xuSshqVbyWKXschNp4TqLV+KrQltDMq3rCvhSj\n g==;","X-CSE-ConnectionGUID":["rBtrRs/OQl2GXYY1VYpx3Q==","If0y8vxtREqklJFhCnoA2w=="],"X-CSE-MsgGUID":["JfqZAm4NSTmqpJYndmiLSA==","t8J7k2KVSsKb/B6DDlF9Ag=="],"X-IronPort-AV":["E=McAfee;i=\"6800,10657,11770\"; a=\"77413969\"","E=Sophos;i=\"6.23,205,1770624000\";\n d=\"scan'208\";a=\"77413969\"","E=Sophos;i=\"6.23,206,1770624000\";\n d=\"scan'208\";a=\"233200201\""],"X-ExtLoop1":"1","From":"=?utf-8?q?Ilpo_J=C3=A4rvinen?= ","Date":"Wed, 29 Apr 2026 13:34:19 +0300 (EEST)","To":"Geramy Loveless ","cc":"Cristian Cocos ,\n =?iso-8859-15?q?Christian_K=F6nig?= ,\n linux-pci@vger.kernel.org","Subject":"Re: [PATCH v3 RESEND] PCI: release empty sibling bridge windows\n during window resize","In-Reply-To":"<20260428225532.3108047-1-gloveless@jqluv.com>","Message-ID":"","References":"<20260428225532.3108047-1-gloveless@jqluv.com>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"multipart/mixed; boundary=\"8323328-1052843889-1777458859=:966\""}}]