[PATCH 0/3] PCI: Locking related improvements

[PATCH 0/3] PCI: Locking related improvements

[Ilpo Järvinen]

Hi all,

Here are a few locking related coding quality improvements, none of
them aims to introduce any function changes. First two convert "must be
asserted" comments into lockdep asserts for easier detection of
violations. The last patch consolidates almost duplicated code in the
bus/slot locking function.

This series based is based on top of the fix (the last change would
obviously conflict with it):

https://patchwork.kernel.org/project/linux-pci/patch/20251212145528.2555-1-guojinhui.liam@bytedance.com/


Ilpo Järvinen (3):
  PCI: Use lockdep_assert_held(pci_bus_sem) to verify lock is held
  PCI: Use device_lock_assert() to verify device lock is held
  PCI: Consolidate pci_bus/slot_lock/unlock/trylock()

 drivers/pci/pci.c | 120 ++++++++++++++++++++++++----------------------
 1 file changed, 63 insertions(+), 57 deletions(-)


base-commit: 270f0a8620a2d8fac3bcab3779df782d85b3b4bf
-- 
2.39.5

[PATCH 1/3] PCI: Use lockdep_assert_held(pci_bus_sem) to verify lock is held

[Ilpo Järvinen]

The function comment for pci_bus_max_d3cold_delay() declares
pci_bus_sem must be held while calling the function which can be
automatically checked. Add lockdep_assert_held(pci_bus_sem) to
confirm pci_bus_sem is held.

Also mark the comment line with Context prefix.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 75a98819db6f..29a365e2dd57 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/init.h>
+#include <linux/lockdep.h>
 #include <linux/msi.h>
 #include <linux/of.h>
 #include <linux/pci.h>
@@ -4622,7 +4623,7 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
  * spec says 100 ms, but firmware can lower it and we allow drivers to
  * increase it as well.
  *
- * Called with @pci_bus_sem locked for reading.
+ * Context: Called with @pci_bus_sem locked for reading.
  */
 static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
 {
@@ -4630,6 +4631,8 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
 	int min_delay = 100;
 	int max_delay = 0;
 
+	lockdep_assert_held(&pci_bus_sem);
+
 	list_for_each_entry(pdev, &bus->devices, bus_list) {
 		if (pdev->d3cold_delay < min_delay)
 			min_delay = pdev->d3cold_delay;
-- 
2.39.5

[PATCH 2/3] PCI: Use device_lock_assert() to verify device lock is held

[Ilpo Järvinen]

Multiple function comments say the function should be called with
device_lock held. Check that by calling device_lock_assert().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29a365e2dd57..e1333539c7b7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4970,6 +4970,7 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
 	 * races with ->remove() by the device lock, which must be held by
 	 * the caller.
 	 */
+	device_lock_assert(&dev->dev);
 	if (err_handler && err_handler->reset_prepare)
 		err_handler->reset_prepare(dev);
 	else if (dev->driver)
@@ -5040,7 +5041,9 @@ const struct pci_reset_fn_method pci_reset_fn_methods[] = {
  * device including MSI, bus mastering, BARs, decoding IO and memory spaces,
  * etc.
  *
- * Returns 0 if the device function was successfully reset or negative if the
+ * Context: The caller must hold the device lock.
+ *
+ * Return: 0 if the device function was successfully reset or negative if the
  * device doesn't support resetting a single function.
  */
 int __pci_reset_function_locked(struct pci_dev *dev)
@@ -5049,6 +5052,7 @@ int __pci_reset_function_locked(struct pci_dev *dev)
 	const struct pci_reset_fn_method *method;
 
 	might_sleep();
+	device_lock_assert(&dev->dev);
 
 	/*
 	 * A reset method returns -ENOTTY if it doesn't support this device and
@@ -5171,13 +5175,17 @@ EXPORT_SYMBOL_GPL(pci_reset_function);
  * over the reset.  It also differs from pci_reset_function() in that it
  * requires the PCI device lock to be held.
  *
- * Returns 0 if the device function was successfully reset or negative if the
+ * Context: The caller must hold the device lock.
+ *
+ * Return: 0 if the device function was successfully reset or negative if the
  * device doesn't support resetting a single function.
  */
 int pci_reset_function_locked(struct pci_dev *dev)
 {
 	int rc;
 
+	device_lock_assert(&dev->dev);
+
 	if (!pci_reset_supported(dev))
 		return -ENOTTY;
 
-- 
2.39.5

[PATCH 3/3] PCI: Consolidate pci_bus/slot_lock/unlock/trylock()

[Ilpo Järvinen]

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>
---
 drivers/pci/pci.c | 103 ++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 54 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e1333539c7b7..622920c1529f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5242,13 +5242,18 @@ 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;
 
-	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
@@ -5257,28 +5262,28 @@ 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;
 
 	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);
 }
 
 /* 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;
 
-	if (!pci_dev_trylock(bus->self))
-		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;
@@ -5288,12 +5293,44 @@ static int pci_bus_trylock(struct pci_bus *bus)
 	return 1;
 
 unlock:
-	list_for_each_entry_continue_reverse(dev, &bus->devices, bus_list) {
+	list_for_each_entry_continue_reverse(dev,
+					     &slot->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);
 	}
+	return 0;
+}
+
+/* Lock devices from the top of the tree down */
+static void pci_bus_lock(struct pci_bus *bus)
+{
+	pci_dev_lock(bus->self);
+	__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);
+	pci_dev_unlock(bus->self);
+}
+
+/* Return 1 on successful lock, 0 on contention */
+static int pci_bus_trylock(struct pci_bus *bus)
+{
+	if (!pci_dev_trylock(bus->self))
+		return 0;
+
+	if (!__pci_bus_trylock(bus, NULL))
+		goto unlock;
+
+	return 1;
+
+unlock:
 	pci_dev_unlock(bus->self);
 	return 0;
 }
@@ -5321,61 +5358,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;
-
-	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;
-
-	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);
-	}
+	__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;
-
-	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);
-	}
-	return 0;
+	return __pci_bus_trylock(slot->bus, slot);
 }
 
 /*
-- 
2.39.5

Re: [PATCH 0/3] PCI: Locking related improvements

[Ilpo Järvinen]

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

On Fri, 16 Jan 2026, Ilpo Järvinen wrote:

> Hi all,
> 
> Here are a few locking related coding quality improvements, none of
> them aims to introduce any function changes. First two convert "must be

s/function change/functional change/

--
 i.

> asserted" comments into lockdep asserts for easier detection of
> violations. The last patch consolidates almost duplicated code in the
> bus/slot locking function.
> 
> This series based is based on top of the fix (the last change would
> obviously conflict with it):
> 
> https://patchwork.kernel.org/project/linux-pci/patch/20251212145528.2555-1-guojinhui.liam@bytedance.com/
> 
> 
> Ilpo Järvinen (3):
>   PCI: Use lockdep_assert_held(pci_bus_sem) to verify lock is held
>   PCI: Use device_lock_assert() to verify device lock is held
>   PCI: Consolidate pci_bus/slot_lock/unlock/trylock()
> 
>  drivers/pci/pci.c | 120 ++++++++++++++++++++++++----------------------
>  1 file changed, 63 insertions(+), 57 deletions(-)
> 
> 
> base-commit: 270f0a8620a2d8fac3bcab3779df782d85b3b4bf
> 

Re: [PATCH 0/3] PCI: Locking related improvements

[Bjorn Helgaas]

On Fri, Jan 16, 2026 at 02:57:39PM +0200, Ilpo Järvinen wrote:
> Hi all,
> 
> Here are a few locking related coding quality improvements, none of
> them aims to introduce any function changes. First two convert "must be
> asserted" comments into lockdep asserts for easier detection of
> violations. The last patch consolidates almost duplicated code in the
> bus/slot locking function.
> 
> This series based is based on top of the fix (the last change would
> obviously conflict with it):
> 
> https://patchwork.kernel.org/project/linux-pci/patch/20251212145528.2555-1-guojinhui.liam@bytedance.com/
> 
> 
> Ilpo Järvinen (3):
>   PCI: Use lockdep_assert_held(pci_bus_sem) to verify lock is held
>   PCI: Use device_lock_assert() to verify device lock is held
>   PCI: Consolidate pci_bus/slot_lock/unlock/trylock()
> 
>  drivers/pci/pci.c | 120 ++++++++++++++++++++++++----------------------
>  1 file changed, 63 insertions(+), 57 deletions(-)

Applied the first two to pci/virtualization along with the other reset
changes for v6.20, thanks!

The last is a great idea but will need some rebase/rework on top of
Keith's changes.