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

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

[Maciej W. Rozycki]

Hi,

 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.

  Maciej

[PATCH 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>
---
 drivers/pci/pci.c   |   15 ++++++++++++++-
 include/linux/pci.h |    1 +
 2 files changed, 15 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
@@ -5149,7 +5149,19 @@ 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 0 if successful or
+ * -ENOTTY 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.
+ */
+int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
 {
 	struct pci_dev *pdev;
 
@@ -5166,6 +5178,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
@@ -1447,6 +1447,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 1/2] PCI: Export pci_parent_bus_reset() for drivers to use

[Ilpo Järvinen]

On Tue, 20 Jun 2023, 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>
> ---
>  drivers/pci/pci.c   |   15 ++++++++++++++-
>  include/linux/pci.h |    1 +
>  2 files changed, 15 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
> @@ -5149,7 +5149,19 @@ 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 0 if successful or

Kernel doc has Return: section for return values.

> + * -ENOTTY 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.

I guess most of the in-depth explanation about reasons why reset 
might not me issuable could go into the longer description block.

-- 
 i.

> + */
> +int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
>  {
>  	struct pci_dev *pdev;
>  
> @@ -5166,6 +5178,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
> @@ -1447,6 +1447,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;					\
> 

[PATCH 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>
---
 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
@@ -796,35 +796,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);
 }
 
 /*

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

[Ilpo Järvinen]

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

On Tue, 20 Jun 2023, Maciej W. Rozycki wrote:

> 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>
> ---
>  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
> @@ -796,35 +796,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);
>  }

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.