[PATCH v2] PCI: defer bridge cfg lock in pci_try_reset_function()

Runyu Xiao <[email protected]>
Newsgroups org.kernel.vger.linux-pci,org.kernel.vger.linux-kernel,org.kernel.vger.stable
Message-ID <[email protected]>
pci_try_reset_function() may need upstream bridge config serialization when
__pci_reset_function_locked() falls back to the "bus" or "cxl_bus" reset
methods.  Unlike pci_reset_function(), it currently only locks the target
device, so those fallback paths can still issue an unlocked secondary-bus
reset.

A naive fix is to trylock the shared upstream bridge before locking the
target device, mirroring pci_reset_function().  But
pci_try_reset_function() is used in vfio_pci_core_enable(), which treats
-EAGAIN as a fatal open failure.  Taking the bridge trylock up front
therefore broadens -EAGAIN to all try-reset callers, including sibling
devices that would otherwise reset independently via FLR.

Keep the existing device trylock semantics, but defer bridge serialization
until __pci_reset_function_locked() is actually about to run a bus-level
reset method.  For pci_try_reset_function(), trylock only the bridge config
access needed by the bus/cxl_bus paths and return -EAGAIN only if that
serialization is really required and contended.

This preserves bridge config serialization for the secondary-bus reset
fallback without introducing spurious -EAGAIN failures for concurrent
resets of sibling devices.

Link: https://lore.kernel.org/r/[email protected]
Fixes: 61cf16d8bd38 ("PCI: Add pci_try_reset_function(), pci_try_reset_slot(), pci_try_reset_bus()")
Cc: [email protected]
Signed-off-by: Runyu Xiao <[email protected]>
---
v2:
- Rework v1 to defer bridge cfg serialization until the `bus` or `cxl_bus`
  fallback is actually selected.
- Avoid broadening `-EAGAIN` to sibling devices that can reset
  independently via FLR or other function-level methods.
- Keep the upstream bridge serialization limited to the path that actually
  needs secondary-bus reset protection.

 drivers/pci/pci.c | 75 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e128696..0aa770c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5166,28 +5166,17 @@ const struct pci_reset_fn_method pci_reset_fn_methods[] = {
 	{ cxl_reset_bus_function, .name = "cxl_bus" },
 };
 
-/**
- * __pci_reset_function_locked - reset a PCI device function while holding
- * the @dev mutex lock.
- * @dev: PCI device to reset
- *
- * Some devices allow an individual function to be reset without affecting
- * other functions in the same device.  The PCI device must be responsive
- * to PCI config space in order to use this function.
- *
- * The device function is presumed to be unused and the caller is holding
- * the device mutex lock when this function is called.
- *
- * Resetting the device will make the contents of PCI configuration space
- * random, so any caller of this must be prepared to reinitialise the
- * 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
- * device doesn't support resetting a single function.
- */
-int __pci_reset_function_locked(struct pci_dev *dev)
+static bool
+pci_reset_fn_needs_bridge_cfg_access(const struct pci_reset_fn_method *method)
 {
+	return method->reset_fn == pci_reset_bus_function ||
+	       method->reset_fn == cxl_reset_bus_function;
+}
+
+static int pci_reset_function_locked_internal(struct pci_dev *dev,
+					      bool trylock_bridge_cfg)
+{
+	struct pci_dev *bridge;
 	int i, m, rc;
 	const struct pci_reset_fn_method *method;
 
@@ -5208,7 +5197,21 @@ int __pci_reset_function_locked(struct pci_dev *dev)
 
 		method = &pci_reset_fn_methods[m];
 		pci_dbg(dev, "reset via %s\n", method->name);
+
+		bridge = NULL;
+		if (trylock_bridge_cfg &&
+		    pci_reset_fn_needs_bridge_cfg_access(method)) {
+			bridge = pci_upstream_bridge(dev);
+			if (bridge && !pci_cfg_access_trylock(bridge)) {
+				rc = -EAGAIN;
+				goto done;
+			}
+		}
+
 		rc = method->reset_fn(dev, PCI_RESET_DO_RESET);
+		if (bridge)
+			pci_cfg_access_unlock(bridge);
+done:
 		if (!rc)
 			return 0;
 
@@ -5219,6 +5222,31 @@ int __pci_reset_function_locked(struct pci_dev *dev)
 
 	return -ENOTTY;
 }
+
+/**
+ * __pci_reset_function_locked - reset a PCI device function while holding
+ * the @dev mutex lock.
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device.  The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * The device function is presumed to be unused and the caller is holding
+ * the device mutex lock when this function is called.
+ *
+ * Resetting the device will make the contents of PCI configuration space
+ * random, so any caller of this must be prepared to reinitialise the
+ * 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
+ * device doesn't support resetting a single function.
+ */
+int __pci_reset_function_locked(struct pci_dev *dev)
+{
+	return pci_reset_function_locked_internal(dev, false);
+}
 EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
 
 /**
@@ -5338,7 +5366,8 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);
  * pci_try_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
  *
- * Same as above, except return -EAGAIN if unable to lock device.
+ * Same as above, except return -EAGAIN if unable to lock device or the
+ * upstream bridge config access needed for a bus-level reset fallback.
  */
 int pci_try_reset_function(struct pci_dev *dev)
 {
@@ -5351,7 +5380,7 @@ int pci_try_reset_function(struct pci_dev *dev)
 		return -EAGAIN;
 
 	pci_dev_save_and_disable(dev);
-	rc = __pci_reset_function_locked(dev);
+	rc = pci_reset_function_locked_internal(dev, true);
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
 
-- 
2.34.1
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.