[PATCH v2 0/2] PCI+IB/hfi1: Fold duplicate secondary bus reset code

[PATCH v2 0/2] PCI+IB/hfi1: Fold duplicate secondary bus reset code

[Maciej W. Rozycki]

Hi,

 This v2 of the patch series addresses a documentation issue raised in v1.

 In the course of verifying my PCIe link training failure workaround (cf. 
<https://lore.kernel.org/r/alpine.DEB.2.21.2305310024400.59226@angie.orcam.me.uk/>) 
in the context of secondary bus reset handling I found a piece of code in 
the InfiniBand HFI1 driver that duplicates what we already have as private 
code in PCI core.  This patch series removes this duplication by exporting 
said private code and than making use of it in the HFI1 driver.

 As I have no means to run-time verify InfiniBand code I have only build 
these patches, for x86-64, with the HFI1 driver both built in and modular.
Please see individual change descriptions for further details.

 Please consider.

 Previous iterations:

- v1 at: <https://lore.kernel.org/r/alpine.DEB.2.21.2306200153110.14084@angie.orcam.me.uk/>.

  Maciej

[PATCH v2 1/2] PCI: Export pci_parent_bus_reset() for drivers to use

[Maciej W. Rozycki]

Export pci_parent_bus_reset() so that drivers do not duplicate it.  
Document the interface.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
Changes from v1, 
<https://lore.kernel.org/r/alpine.DEB.2.21.2306200231020.14084@angie.orcam.me.uk/>:

- Reword function description so as to list the return values separately.
---
 drivers/pci/pci.c   |   17 ++++++++++++++++-
 include/linux/pci.h |    1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

linux-pci-parent-bus-reset-export.diff
Index: linux-macro/drivers/pci/pci.c
===================================================================
--- linux-macro.orig/drivers/pci/pci.c
+++ linux-macro/drivers/pci/pci.c
@@ -4833,7 +4833,21 @@ int pci_bridge_secondary_bus_reset(struc
 }
 EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
 
-static int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
+/**
+ * pci_parent_bus_reset - Reset a device via its upstream PCI bridge
+ * @dev: Device to reset.
+ * @probe: Only check if reset is possible if TRUE, actually reset if FALSE.
+ *
+ * Perform a device reset by requesting a secondary bus reset via the
+ * device's immediate upstream PCI bridge.  Return failure if the reset
+ * failed or it could not have been issued in the first place because
+ * the device is not on a secondary bus of any PCI bridge or it wouldn't
+ * be the only device reset.  If probing, then only verify whether it
+ * would be possible to issue a reset.
+ *
+ * Returns: 0 if successful or -ENOTTY otherwise.
+ */
+int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
 {
 	struct pci_dev *pdev;
 
@@ -4850,6 +4864,7 @@ static int pci_parent_bus_reset(struct p
 
 	return pci_bridge_secondary_bus_reset(dev->bus->self);
 }
+EXPORT_SYMBOL_GPL(pci_parent_bus_reset);
 
 static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, bool probe)
 {
Index: linux-macro/include/linux/pci.h
===================================================================
--- linux-macro.orig/include/linux/pci.h
+++ linux-macro/include/linux/pci.h
@@ -1595,6 +1595,7 @@ int devm_request_pci_bus_resources(struc
 
 /* Temporary until new and working PCI SBR API in place */
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
+int pci_parent_bus_reset(struct pci_dev *dev, bool probe);
 
 #define __pci_bus_for_each_res0(bus, res, ...)				\
 	for (unsigned int __b = 0;					\

Re: [PATCH v2 1/2] PCI: Export pci_parent_bus_reset() for drivers to use

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unprotected list traversal and data race in `pci_parent_bus_reset()` exposed to external drivers.
- [High] Exporting `pci_parent_bus_reset()` creates an unsafe API that bypasses mandatory PCI core state management and synchronization.
--

commit 73a6ae560868e4a44decb13db21d60108f4c09f1
Author: Maciej W. Rozycki <macro@orcam.me.uk>

PCI: Export pci_parent_bus_reset() for drivers to use

This commit removes the static modifier from pci_parent_bus_reset(),
adds kernel doc comments, and exports it using EXPORT_SYMBOL_GPL()
so that external drivers can trigger a secondary bus reset via the
device's immediate upstream bridge.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4820,7 +4820,21 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
[ ... ]
> +int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
>  {
>  	struct pci_dev *pdev;
>  

Internally, pci_parent_bus_reset() traverses dev->bus->devices using
list_for_each_entry() locklessly.

Reading this list typically requires holding pci_bus_sem to protect against
concurrent modifications from hotplug events.

Since pci_bus_sem is an internal lock and not exported to modules, drivers
utilizing this newly exported API will not be able to acquire the required
locks prior to calling it.

Does exporting this function introduce a regression regarding list
corruption or use-after-free?

> @@ -4837,6 +4851,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
>  
>  	return pci_bridge_secondary_bus_reset(dev->bus->self);
>  }
> +EXPORT_SYMBOL_GPL(pci_parent_bus_reset);

Unlike higher-level APIs like pci_reset_function(), this newly exported
function does not save and restore the PCI configuration space or prepare
the IOMMU for DMA reset.

Additionally, pci_bridge_secondary_bus_reset() expects the bridge
configuration access to be locked, and emits a warning if it is not:

pci_bridge_secondary_bus_reset() {
    if (!dev->block_cfg_access)
        pci_warn_once(dev, "unlocked secondary bus reset via...");
}

Could drivers utilizing this export trigger system instability or
uncorrectable errors while DMA is active, since it bypasses state
management and locking steps like acquiring pci_dev_lock() on the upstream
bridge?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/alpine.DEB.2.21.2605050042390.46195@angie.orcam.me.uk?part=1

Re: [PATCH v2 1/2] PCI: Export pci_parent_bus_reset() for drivers to use

[Leon Romanovsky]

On Tue, May 05, 2026 at 01:03:39AM +0100, Maciej W. Rozycki wrote:
> Export pci_parent_bus_reset() so that drivers do not duplicate it.  
> Document the interface.
> 
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> ---
> Changes from v1, 
> <https://lore.kernel.org/r/alpine.DEB.2.21.2306200231020.14084@angie.orcam.me.uk/>:
> 
> - Reword function description so as to list the return values separately.
> ---
>  drivers/pci/pci.c   |   17 ++++++++++++++++-
>  include/linux/pci.h |    1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> linux-pci-parent-bus-reset-export.diff
> Index: linux-macro/drivers/pci/pci.c
> ===================================================================
> --- linux-macro.orig/drivers/pci/pci.c
> +++ linux-macro/drivers/pci/pci.c
> @@ -4833,7 +4833,21 @@ int pci_bridge_secondary_bus_reset(struc
>  }
>  EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
>  
> -static int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
> +/**
> + * pci_parent_bus_reset - Reset a device via its upstream PCI bridge
> + * @dev: Device to reset.
> + * @probe: Only check if reset is possible if TRUE, actually reset if FALSE.
> + *
> + * Perform a device reset by requesting a secondary bus reset via the
> + * device's immediate upstream PCI bridge.  Return failure if the reset
> + * failed or it could not have been issued in the first place because
> + * the device is not on a secondary bus of any PCI bridge or it wouldn't
> + * be the only device reset.  If probing, then only verify whether it
> + * would be possible to issue a reset.
> + *
> + * Returns: 0 if successful or -ENOTTY otherwise.
> + */
> +int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
>  {
>  	struct pci_dev *pdev;
>  
> @@ -4850,6 +4864,7 @@ static int pci_parent_bus_reset(struct p
>  
>  	return pci_bridge_secondary_bus_reset(dev->bus->self);
>  }
> +EXPORT_SYMBOL_GPL(pci_parent_bus_reset);

I wouldn't recommend doing this solely for hfi1. The driver is likely to be
removed or significantly changed soon.

https://lore.kernel.org/linux-rdma/177516078937.637585.1447184858924347033.stgit@awdrv-04.cornelisnetworks.com/

Thanks

Re: [PATCH v2 1/2] PCI: Export pci_parent_bus_reset() for drivers to use

[Maciej W. Rozycki]

On Tue, 12 May 2026, Leon Romanovsky wrote:

> > Export pci_parent_bus_reset() so that drivers do not duplicate it.  
> > Document the interface.
[...]
> 
> I wouldn't recommend doing this solely for hfi1. The driver is likely to be
> removed or significantly changed soon.
> 
> https://lore.kernel.org/linux-rdma/177516078937.637585.1447184858924347033.stgit@awdrv-04.cornelisnetworks.com/

 Thank you for the pointer.  FWIW as per the cover letter I have no own 
interest in the driver and offered this cleanup as a general code quality 
improvement.

 Dennis, please clarify your position, and if you'd rather this change 
wasn't made, then I'll drop the patches from my local verification setup 
and won't offer them again.

  Maciej

[PATCH v2 2/2] IB/hfi1: Remove pci_parent_bus_reset() duplication

[Maciej W. Rozycki]

Call pci_parent_bus_reset() rather than duplicating it in trigger_sbr().
There are extra preparatory checks made by the former function, but they 
are supposed to be neutral to the HFI1 device.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
No change from v1, 
<https://lore.kernel.org/r/alpine.DEB.2.21.2306200235510.14084@angie.orcam.me.uk/>.
---
 drivers/infiniband/hw/hfi1/pcie.c |   30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

linux-ib-hfi1-pcie-sbr-parent-bus-reset.diff
Index: linux-macro/drivers/infiniband/hw/hfi1/pcie.c
===================================================================
--- linux-macro.orig/drivers/infiniband/hw/hfi1/pcie.c
+++ linux-macro/drivers/infiniband/hw/hfi1/pcie.c
@@ -791,35 +791,13 @@ static void pcie_post_steps(struct hfi1_
 /*
  * Trigger a secondary bus reset (SBR) on ourselves using our parent.
  *
- * Based on pci_parent_bus_reset() which is not exported by the
- * kernel core.
+ * This is an end around to do an SBR during probe time.  A new API
+ * needs to be implemented to have cleaner interface but this fixes
+ * the current brokenness.
  */
 static int trigger_sbr(struct hfi1_devdata *dd)
 {
-	struct pci_dev *dev = dd->pcidev;
-	struct pci_dev *pdev;
-
-	/* need a parent */
-	if (!dev->bus->self) {
-		dd_dev_err(dd, "%s: no parent device\n", __func__);
-		return -ENOTTY;
-	}
-
-	/* should not be anyone else on the bus */
-	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
-		if (pdev != dev) {
-			dd_dev_err(dd,
-				   "%s: another device is on the same bus\n",
-				   __func__);
-			return -ENOTTY;
-		}
-
-	/*
-	 * This is an end around to do an SBR during probe time. A new API needs
-	 * to be implemented to have cleaner interface but this fixes the
-	 * current brokenness
-	 */
-	return pci_bridge_secondary_bus_reset(dev->bus->self);
+	return pci_parent_bus_reset(dd->pcidev, PCI_RESET_DO_RESET);
 }
 
 /*