From: Bjorn Helgaas <helgaas@kernel.org>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Jinhui Guo <guojinhui.liam@bytedance.com>,
Keith Busch <kbusch@kernel.org>,
"Anthony Pighin (Nokia)" <anthony.pighin@nokia.com>,
Alex Williamson <alex@shazbot.org>,
Bjorn Helgaas <bhelgaas@google.com>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] PCI: Consolidate pci_bus/slot_lock/unlock/trylock()
Date: Thu, 5 Mar 2026 17:39:02 -0600 [thread overview]
Message-ID: <20260305233902.GA80310@bhelgaas> (raw)
In-Reply-To: <20260304122139.1479-1-ilpo.jarvinen@linux.intel.com>
On Wed, Mar 04, 2026 at 02:21:38PM +0200, Ilpo Järvinen wrote:
> pci_bus/slot_lock/unlock/trylock() largely duplicate the bus iteration
> loop with variation only due to slot filter handling. The only
> differences in the loops is where the struct bus is found (directly in
> the argument vs in slot->bus) and whether slot filter is applied. Those
> difference are simple to handle using function parameters.
>
> Consolidate the bus iteration loop to one place by creating
> __pci_bus_{lock,unlock,trylock}() and call them from the non-underscore
> locking functions.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Applied to pci/reset for v7.1, thanks, Ilpo.
> ---
>
> v2:
> - Rebased
> - Fixed rollback path dereferencing slot->bus (changed to use the bus
> parameter directly as slot can be NULL)
>
> drivers/pci/pci.c | 115 ++++++++++++++++++++--------------------------
> 1 file changed, 49 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8479c2e1f74f..c248224e0861 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5292,13 +5292,21 @@ static bool pci_bus_resettable(struct pci_bus *bus)
> return true;
> }
>
> +static void pci_bus_lock(struct pci_bus *bus);
> +static void pci_bus_unlock(struct pci_bus *bus);
> +static int pci_bus_trylock(struct pci_bus *bus);
> +
> /* Lock devices from the top of the tree down */
> -static void pci_bus_lock(struct pci_bus *bus)
> +static void __pci_bus_lock(struct pci_bus *bus, struct pci_slot *slot)
> {
> - struct pci_dev *dev;
> + struct pci_dev *dev, *bridge = bus->self;
> +
> + if (bridge)
> + pci_dev_lock(bridge);
>
> - pci_dev_lock(bus->self);
> list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (slot && (!dev->slot || dev->slot != slot))
> + continue;
> if (dev->subordinate)
> pci_bus_lock(dev->subordinate);
> else
> @@ -5307,28 +5315,34 @@ static void pci_bus_lock(struct pci_bus *bus)
> }
>
> /* Unlock devices from the bottom of the tree up */
> -static void pci_bus_unlock(struct pci_bus *bus)
> +static void __pci_bus_unlock(struct pci_bus *bus, struct pci_slot *slot)
> {
> - struct pci_dev *dev;
> + struct pci_dev *dev, *bridge = bus->self;
>
> list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (slot && (!dev->slot || dev->slot != slot))
> + continue;
> if (dev->subordinate)
> pci_bus_unlock(dev->subordinate);
> else
> pci_dev_unlock(dev);
> }
> - pci_dev_unlock(bus->self);
> +
> + if (bridge)
> + pci_dev_unlock(bridge);
> }
>
> /* Return 1 on successful lock, 0 on contention */
> -static int pci_bus_trylock(struct pci_bus *bus)
> +static int __pci_bus_trylock(struct pci_bus *bus, struct pci_slot *slot)
> {
> - struct pci_dev *dev;
> + struct pci_dev *dev, *bridge = bus->self;
>
> - if (!pci_dev_trylock(bus->self))
> + if (bridge && !pci_dev_trylock(bridge))
> return 0;
>
> list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (slot && (!dev->slot || dev->slot != slot))
> + continue;
> if (dev->subordinate) {
> if (!pci_bus_trylock(dev->subordinate))
> goto unlock;
> @@ -5339,15 +5353,37 @@ static int pci_bus_trylock(struct pci_bus *bus)
>
> unlock:
> list_for_each_entry_continue_reverse(dev, &bus->devices, bus_list) {
> + if (slot && (!dev->slot || dev->slot != slot))
> + continue;
> if (dev->subordinate)
> pci_bus_unlock(dev->subordinate);
> else
> pci_dev_unlock(dev);
> }
> - pci_dev_unlock(bus->self);
> +
> + if (bridge)
> + pci_dev_unlock(bridge);
> return 0;
> }
>
> +/* Lock devices from the top of the tree down */
> +static void pci_bus_lock(struct pci_bus *bus)
> +{
> + __pci_bus_lock(bus, NULL);
> +}
> +
> +/* Unlock devices from the bottom of the tree up */
> +static void pci_bus_unlock(struct pci_bus *bus)
> +{
> + __pci_bus_unlock(bus, NULL);
> +}
> +
> +/* Return 1 on successful lock, 0 on contention */
> +static int pci_bus_trylock(struct pci_bus *bus)
> +{
> + return __pci_bus_trylock(bus, NULL);
> +}
> +
> /* Do any devices on or below this slot prevent a bus reset? */
> static bool pci_slot_resettable(struct pci_slot *slot)
> {
> @@ -5370,72 +5406,19 @@ static bool pci_slot_resettable(struct pci_slot *slot)
> /* Lock devices from the top of the tree down */
> static void pci_slot_lock(struct pci_slot *slot)
> {
> - struct pci_dev *dev, *bridge = slot->bus->self;
> -
> - if (bridge)
> - pci_dev_lock(bridge);
> -
> - list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> - if (!dev->slot || dev->slot != slot)
> - continue;
> - if (dev->subordinate)
> - pci_bus_lock(dev->subordinate);
> - else
> - pci_dev_lock(dev);
> - }
> + __pci_bus_lock(slot->bus, slot);
> }
>
> /* Unlock devices from the bottom of the tree up */
> static void pci_slot_unlock(struct pci_slot *slot)
> {
> - struct pci_dev *dev, *bridge = slot->bus->self;
> -
> - list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> - if (!dev->slot || dev->slot != slot)
> - continue;
> - if (dev->subordinate)
> - pci_bus_unlock(dev->subordinate);
> - else
> - pci_dev_unlock(dev);
> - }
> -
> - if (bridge)
> - pci_dev_unlock(bridge);
> + __pci_bus_unlock(slot->bus, slot);
> }
>
> /* Return 1 on successful lock, 0 on contention */
> static int pci_slot_trylock(struct pci_slot *slot)
> {
> - struct pci_dev *dev, *bridge = slot->bus->self;
> -
> - if (bridge && !pci_dev_trylock(bridge))
> - return 0;
> -
> - list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> - if (!dev->slot || dev->slot != slot)
> - continue;
> - if (dev->subordinate) {
> - if (!pci_bus_trylock(dev->subordinate))
> - goto unlock;
> - } else if (!pci_dev_trylock(dev))
> - goto unlock;
> - }
> - return 1;
> -
> -unlock:
> - list_for_each_entry_continue_reverse(dev,
> - &slot->bus->devices, bus_list) {
> - if (!dev->slot || dev->slot != slot)
> - continue;
> - if (dev->subordinate)
> - pci_bus_unlock(dev->subordinate);
> - else
> - pci_dev_unlock(dev);
> - }
> -
> - if (bridge)
> - pci_dev_unlock(bridge);
> - return 0;
> + return __pci_bus_trylock(slot->bus, slot);
> }
>
> /*
>
> base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
> --
> 2.39.5
>
prev parent reply other threads:[~2026-03-05 23:39 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-04 12:21 [PATCH v2 1/1] PCI: Consolidate pci_bus/slot_lock/unlock/trylock() Ilpo Järvinen
2026-03-05 23:39 ` Bjorn Helgaas [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260305233902.GA80310@bhelgaas \
--to=helgaas@kernel.org \
--cc=alex@shazbot.org \
--cc=anthony.pighin@nokia.com \
--cc=bhelgaas@google.com \
--cc=guojinhui.liam@bytedance.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=kbusch@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox