[PATCH v4 0/6] PCI: Add reset type parameter to PCI reset functions

[PATCH v4 0/6] PCI: Add reset type parameter to PCI reset functions

[Sinan Kaya]

We want more control over what kind of reset PCI core can apply. Have the
application explicitly requested allowed reset types.

differences from v3:
- set default return value to -EINVAL in __pci_reset_function_locked()

Sinan Kaya (6):
  PCI: Expose reset_type to users of __pci_reset_function_locked()
  PCI: Expose reset_type to users of pci_reset_function()
  PCI: Expose reset_type to users of pci_reset_function_locked()
  PCI: Expose reset type to users of pci_try_reset_function()
  PCI: Expose reset type to users of pci_probe_reset_function()
  PCI: Expose reset type to users of pci_reset_bus()

 drivers/hid/intel-ish-hid/ipc/ipc.c           |   2 +-
 drivers/misc/genwqe/card_base.c               |   2 +-
 .../net/ethernet/cavium/liquidio/lio_main.c   |   2 +-
 .../net/ethernet/qlogic/qlcnic/qlcnic_ctx.c   |   2 +-
 drivers/net/ethernet/sfc/mcdi.c               |   2 +-
 drivers/pci/pci-sysfs.c                       |   2 +-
 drivers/pci/pci.c                             | 134 ++++++++++++------
 drivers/pci/pci.h                             |   2 +-
 drivers/pci/probe.c                           |   2 +-
 drivers/vfio/pci/vfio_pci.c                   |  13 +-
 drivers/vfio/pci/vfio_pci_config.c            |   4 +-
 drivers/xen/xen-pciback/pci_stub.c            |   6 +-
 include/linux/pci.h                           |  21 ++-
 13 files changed, 128 insertions(+), 66 deletions(-)

-- 
2.18.0

[PATCH v4 1/6] PCI: Expose reset_type to users of __pci_reset_function_locked()

[Sinan Kaya]

We need a contract between the reset API users and the PCI core about the
types of reset that a user needs vs. what PCI core can do internally.
If a platform supports hotplug, we need to do hotplug reset as an example.

Expose the reset types to the drivers and try different reset types based
on the new reset_type parameter.

Most users are expected to use PCI_RESET_ANY, PCI_RESET_FUNC or
PCI_RESET_LINK parameters.

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 .../net/ethernet/cavium/liquidio/lio_main.c   |  2 +-
 drivers/pci/pci.c                             | 59 ++++++++++++-------
 drivers/xen/xen-pciback/pci_stub.c            |  6 +-
 include/linux/pci.h                           | 13 +++-
 4 files changed, 55 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 6fb13fa73b27..0ff76722734d 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -989,7 +989,7 @@ static void octeon_pci_flr(struct octeon_device *oct)
 	pci_write_config_word(oct->pci_dev, PCI_COMMAND,
 			      PCI_COMMAND_INTX_DISABLE);
 
-	rc = __pci_reset_function_locked(oct->pci_dev);
+	rc = __pci_reset_function_locked(oct->pci_dev, PCI_RESET_ANY);
 
 	if (rc != 0)
 		dev_err(&oct->pci_dev->dev, "Error %d resetting PCI function %d\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1835f3a7aa8d..e292ea589d3e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4673,6 +4673,7 @@ static void pci_dev_restore(struct pci_dev *dev)
  * __pci_reset_function_locked - reset a PCI device function while holding
  * the @dev mutex lock.
  * @dev: PCI device to reset
+ * @reset_type: reset mask to try
  *
  * Some devices allow an individual function to be reset without affecting
  * other functions in the same device.  The PCI device must be responsive
@@ -4688,9 +4689,9 @@ static void pci_dev_restore(struct pci_dev *dev)
  * 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)
+int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type)
 {
-	int rc;
+	int rc = -EINVAL;
 
 	might_sleep();
 
@@ -4702,24 +4703,42 @@ int __pci_reset_function_locked(struct pci_dev *dev)
 	 * other error, we're also finished: this indicates that further
 	 * reset mechanisms might be broken on the device.
 	 */
-	rc = pci_dev_specific_reset(dev, 0);
-	if (rc != -ENOTTY)
-		return rc;
-	if (pcie_has_flr(dev)) {
-		rc = pcie_flr(dev);
+	if (reset_type & PCI_RESET_DEV_SPECIFIC) {
+		rc = pci_dev_specific_reset(dev, 0);
 		if (rc != -ENOTTY)
 			return rc;
 	}
-	rc = pci_af_flr(dev, 0);
-	if (rc != -ENOTTY)
-		return rc;
-	rc = pci_pm_reset(dev, 0);
-	if (rc != -ENOTTY)
-		return rc;
-	rc = pci_dev_reset_slot_function(dev, 0);
-	if (rc != -ENOTTY)
-		return rc;
-	return pci_parent_bus_reset(dev, 0);
+
+	if (reset_type & PCI_RESET_FLR) {
+		if (pcie_has_flr(dev)) {
+			rc = pcie_flr(dev);
+			if (rc != -ENOTTY)
+				return rc;
+		}
+		rc = pci_af_flr(dev, 0);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	if (reset_type & PCI_RESET_PM) {
+		rc = pci_pm_reset(dev, 0);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	if (reset_type & PCI_RESET_SLOT) {
+		rc = pci_dev_reset_slot_function(dev, 0);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	if (reset_type & PCI_RESET_BUS) {
+		rc = pci_parent_bus_reset(dev, 0);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	return rc;
 }
 EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
 
@@ -4784,7 +4803,7 @@ int pci_reset_function(struct pci_dev *dev)
 	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
 
-	rc = __pci_reset_function_locked(dev);
+	rc = __pci_reset_function_locked(dev, PCI_RESET_ANY);
 
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
@@ -4819,7 +4838,7 @@ int pci_reset_function_locked(struct pci_dev *dev)
 
 	pci_dev_save_and_disable(dev);
 
-	rc = __pci_reset_function_locked(dev);
+	rc = __pci_reset_function_locked(dev, PCI_RESET_ANY);
 
 	pci_dev_restore(dev);
 
@@ -4844,7 +4863,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(dev, PCI_RESET_ANY);
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
 
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 59661db144e5..6dfb805bcb19 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -105,7 +105,7 @@ static void pcistub_device_release(struct kref *kref)
 	/* Call the reset function which does not take lock as this
 	 * is called from "unbind" which takes a device_lock mutex.
 	 */
-	__pci_reset_function_locked(dev);
+	__pci_reset_function_locked(dev, PCI_RESET_ANY);
 	if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state))
 		dev_info(&dev->dev, "Could not reload PCI state\n");
 	else
@@ -283,7 +283,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
 	 * (so it's ready for the next domain)
 	 */
 	device_lock_assert(&dev->dev);
-	__pci_reset_function_locked(dev);
+	__pci_reset_function_locked(dev, PCI_RESET_ANY);
 
 	dev_data = pci_get_drvdata(dev);
 	ret = pci_load_saved_state(dev, dev_data->pci_saved_state);
@@ -417,7 +417,7 @@ static int pcistub_init_device(struct pci_dev *dev)
 		dev_err(&dev->dev, "Could not store PCI conf saved state!\n");
 	else {
 		dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n");
-		__pci_reset_function_locked(dev);
+		__pci_reset_function_locked(dev, PCI_RESET_ANY);
 		pci_restore_state(dev);
 	}
 	/* Now disable the device (this also ensures some private device
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6925828f9f25..d2e0a8c8177b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -849,6 +849,17 @@ enum {
 	PCI_SCAN_ALL_PCIE_DEVS	= 0x00000040,	/* Scan all, not just dev 0 */
 };
 
+#define PCI_RESET_DEV_SPECIFIC	(1 << 0)
+#define PCI_RESET_FLR		(1 << 1)
+#define PCI_RESET_PM		(1 << 2)
+#define PCI_RESET_SLOT		(1 << 3)
+#define PCI_RESET_BUS		(1 << 4)
+
+#define PCI_RESET_ANY		(~0)
+#define PCI_RESET_FUNC		(PCI_RESET_DEV_SPECIFIC | \
+				 PCI_RESET_FLR | PCI_RESET_PM)
+#define PCI_RESET_LINK		(PCI_RESET_SLOT | PCI_RESET_BUS)
+
 /* These external functions are only available when PCI support is enabled */
 #ifdef CONFIG_PCI
 
@@ -1111,7 +1122,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
 void pcie_print_link_status(struct pci_dev *dev);
 bool pcie_has_flr(struct pci_dev *dev);
 int pcie_flr(struct pci_dev *dev);
-int __pci_reset_function_locked(struct pci_dev *dev);
+int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
 int pci_reset_function(struct pci_dev *dev);
 int pci_reset_function_locked(struct pci_dev *dev);
 int pci_try_reset_function(struct pci_dev *dev);
-- 
2.18.0

[PATCH v4 2/6] PCI: Expose reset_type to users of pci_reset_function()

[Sinan Kaya]

Looking to have more control between the users of the API vs. what the API
can do internally. The new reset_type tells the PCI core about the bounds
of the request.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> (drivers/hid/intel-ish-hid/ipc/ipc.c)
---
 drivers/hid/intel-ish-hid/ipc/ipc.c             | 2 +-
 drivers/misc/genwqe/card_base.c                 | 2 +-
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 2 +-
 drivers/net/ethernet/sfc/mcdi.c                 | 2 +-
 drivers/pci/pci-sysfs.c                         | 2 +-
 drivers/pci/pci.c                               | 4 ++--
 include/linux/pci.h                             | 2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index bfbca7ec54ce..18312969f1b3 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -754,7 +754,7 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 	if (!pdev)
 		return	-ENODEV;
 
-	rv = pci_reset_function(pdev);
+	rv = pci_reset_function(pdev, PCI_RESET_ANY);
 	if (!rv)
 		dev->dev_state = ISHTP_DEV_RESETTING;
 
diff --git a/drivers/misc/genwqe/card_base.c b/drivers/misc/genwqe/card_base.c
index c7cd3675bcd1..cc78ef28ee38 100644
--- a/drivers/misc/genwqe/card_base.c
+++ b/drivers/misc/genwqe/card_base.c
@@ -201,7 +201,7 @@ static int genwqe_bus_reset(struct genwqe_dev *cd)
 	 * restored by the pci_reset_function().
 	 */
 	dev_dbg(&pci_dev->dev, "[%s] pci_reset function ...\n", __func__);
-	rc = pci_reset_function(pci_dev);
+	rc = pci_reset_function(pci_dev, PCI_RESET_ANY);
 	if (rc) {
 		dev_err(&pci_dev->dev,
 			"[%s] err: failed reset func (rc %d)\n", __func__, rc);
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
index d344e9d43832..bb737725f175 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
@@ -629,7 +629,7 @@ int qlcnic_fw_create_ctx(struct qlcnic_adapter *dev)
 	int i, err, ring;
 
 	if (dev->flags & QLCNIC_NEED_FLR) {
-		pci_reset_function(dev->pdev);
+		pci_reset_function(dev->pdev, PCI_RESET_ANY);
 		dev->flags &= ~QLCNIC_NEED_FLR;
 	}
 
diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c
index dfad93fca0a6..7f95e17b8a48 100644
--- a/drivers/net/ethernet/sfc/mcdi.c
+++ b/drivers/net/ethernet/sfc/mcdi.c
@@ -1862,7 +1862,7 @@ int efx_mcdi_reset(struct efx_nic *efx, enum reset_type method)
 
 	/* If MCDI is down, we can't handle_assertion */
 	if (method == RESET_TYPE_MCDI_TIMEOUT) {
-		rc = pci_reset_function(efx->pci_dev);
+		rc = pci_reset_function(efx->pci_dev, PCI_RESET_ANY);
 		if (rc)
 			return rc;
 		/* Re-enable polled MCDI completion */
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9ecfe13157c0..9569664ec4b2 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1449,7 +1449,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
 		return -EINVAL;
 
 	pm_runtime_get_sync(dev);
-	result = pci_reset_function(pdev);
+	result = pci_reset_function(pdev, PCI_RESET_ANY);
 	pm_runtime_put(dev);
 	if (result < 0)
 		return result;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e292ea589d3e..04fd40472a93 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4793,7 +4793,7 @@ int pci_probe_reset_function(struct pci_dev *dev)
  * 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(struct pci_dev *dev)
+int pci_reset_function(struct pci_dev *dev, u32 reset_type)
 {
 	int rc;
 
@@ -4803,7 +4803,7 @@ int pci_reset_function(struct pci_dev *dev)
 	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
 
-	rc = __pci_reset_function_locked(dev, PCI_RESET_ANY);
+	rc = __pci_reset_function_locked(dev, reset_type);
 
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d2e0a8c8177b..1a1a76130e91 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1123,7 +1123,7 @@ void pcie_print_link_status(struct pci_dev *dev);
 bool pcie_has_flr(struct pci_dev *dev);
 int pcie_flr(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
-int pci_reset_function(struct pci_dev *dev);
+int pci_reset_function(struct pci_dev *dev, u32 reset_type);
 int pci_reset_function_locked(struct pci_dev *dev);
 int pci_try_reset_function(struct pci_dev *dev);
 int pci_probe_reset_slot(struct pci_slot *slot);
-- 
2.18.0

[PATCH v4 3/6] PCI: Expose reset_type to users of pci_reset_function_locked()

[Sinan Kaya]

Looking to have more control between the users of the API vs. what the API
can do internally. The new reset_type tells the PCI core about the bounds
of the request.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 drivers/pci/pci.c   | 4 ++--
 include/linux/pci.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 04fd40472a93..63b5930d8fb1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4829,7 +4829,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function);
  * 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)
+int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type)
 {
 	int rc;
 
@@ -4838,7 +4838,7 @@ int pci_reset_function_locked(struct pci_dev *dev)
 
 	pci_dev_save_and_disable(dev);
 
-	rc = __pci_reset_function_locked(dev, PCI_RESET_ANY);
+	rc = __pci_reset_function_locked(dev, reset_type);
 
 	pci_dev_restore(dev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1a1a76130e91..09aa99dd1c1b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1124,7 +1124,7 @@ bool pcie_has_flr(struct pci_dev *dev);
 int pcie_flr(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
 int pci_reset_function(struct pci_dev *dev, u32 reset_type);
-int pci_reset_function_locked(struct pci_dev *dev);
+int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
 int pci_try_reset_function(struct pci_dev *dev);
 int pci_probe_reset_slot(struct pci_slot *slot);
 int pci_probe_reset_bus(struct pci_bus *bus);
-- 
2.18.0

[PATCH v4 4/6] PCI: Expose reset type to users of pci_try_reset_function()

[Sinan Kaya]

Looking to have more control between the users of the API vs. what the API
can do internally. The new reset_type tells the PCI core about the bounds
of the request.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 drivers/pci/pci.c                  | 4 ++--
 drivers/vfio/pci/vfio_pci.c        | 7 ++++---
 drivers/vfio/pci/vfio_pci_config.c | 4 ++--
 include/linux/pci.h                | 2 +-
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 63b5930d8fb1..4edf70a4d528 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4852,7 +4852,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);
  *
  * Same as above, except return -EAGAIN if unable to lock device.
  */
-int pci_try_reset_function(struct pci_dev *dev)
+int pci_try_reset_function(struct pci_dev *dev, u32 reset_type)
 {
 	int rc;
 
@@ -4863,7 +4863,7 @@ int pci_try_reset_function(struct pci_dev *dev)
 		return -EAGAIN;
 
 	pci_dev_save_and_disable(dev);
-	rc = __pci_reset_function_locked(dev, PCI_RESET_ANY);
+	rc = __pci_reset_function_locked(dev, reset_type);
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
 
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index cddb453a1ba5..fe7ada997c51 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -228,7 +228,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
 		return ret;
 
 	/* If reset fails because of the device lock, fail this path entirely */
-	ret = pci_try_reset_function(pdev);
+	ret = pci_try_reset_function(pdev, PCI_RESET_ANY);
 	if (ret == -EAGAIN) {
 		pci_disable_device(pdev);
 		return ret;
@@ -376,7 +376,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
 	 * Try to reset the device.  The success of this is dependent on
 	 * being able to lock the device, which is not always possible.
 	 */
-	if (vdev->reset_works && !pci_try_reset_function(pdev))
+	if (vdev->reset_works && !pci_try_reset_function(pdev, PCI_RESET_ANY))
 		vdev->needs_reset = false;
 
 	pci_restore_state(pdev);
@@ -844,7 +844,8 @@ static long vfio_pci_ioctl(void *device_data,
 
 	} else if (cmd == VFIO_DEVICE_RESET) {
 		return vdev->reset_works ?
-			pci_try_reset_function(vdev->pdev) : -EINVAL;
+			pci_try_reset_function(vdev->pdev, PCI_RESET_ANY) :
+			 -EINVAL;
 
 	} else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
 		struct vfio_pci_hot_reset_info hdr;
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 115a36f6f403..0d66bac66211 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -831,7 +831,7 @@ static int vfio_exp_config_write(struct vfio_pci_device *vdev, int pos,
 						 &cap);
 
 		if (!ret && (cap & PCI_EXP_DEVCAP_FLR))
-			pci_try_reset_function(vdev->pdev);
+			pci_try_reset_function(vdev->pdev, PCI_RESET_ANY);
 	}
 
 	/*
@@ -910,7 +910,7 @@ static int vfio_af_config_write(struct vfio_pci_device *vdev, int pos,
 						&cap);
 
 		if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP))
-			pci_try_reset_function(vdev->pdev);
+			pci_try_reset_function(vdev->pdev, PCI_RESET_ANY);
 	}
 
 	return count;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 09aa99dd1c1b..4fdddcb85066 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1125,7 +1125,7 @@ int pcie_flr(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
 int pci_reset_function(struct pci_dev *dev, u32 reset_type);
 int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
-int pci_try_reset_function(struct pci_dev *dev);
+int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
 int pci_probe_reset_slot(struct pci_slot *slot);
 int pci_probe_reset_bus(struct pci_bus *bus);
 int pci_reset_bus(struct pci_dev *dev);
-- 
2.18.0

[PATCH v4 5/6] PCI: Expose reset type to users of pci_probe_reset_function()

[Sinan Kaya]

Looking to have more control between the users of the API vs. what the API
can do internally. The new reset_type tells the PCI core about the bounds
of the request.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 drivers/pci/pci.c   | 51 ++++++++++++++++++++++++++++++---------------
 drivers/pci/pci.h   |  2 +-
 drivers/pci/probe.c |  2 +-
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4edf70a4d528..b4f14419bd51 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4753,28 +4753,45 @@ EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
  * Returns 0 if the device function can be reset or negative if the
  * device doesn't support resetting a single function.
  */
-int pci_probe_reset_function(struct pci_dev *dev)
+int pci_probe_reset_function(struct pci_dev *dev, u32 reset_type)
 {
-	int rc;
+	int rc = 0;
 
 	might_sleep();
 
-	rc = pci_dev_specific_reset(dev, 1);
-	if (rc != -ENOTTY)
-		return rc;
-	if (pcie_has_flr(dev))
-		return 0;
-	rc = pci_af_flr(dev, 1);
-	if (rc != -ENOTTY)
-		return rc;
-	rc = pci_pm_reset(dev, 1);
-	if (rc != -ENOTTY)
-		return rc;
-	rc = pci_dev_reset_slot_function(dev, 1);
-	if (rc != -ENOTTY)
-		return rc;
+	if (reset_type & PCI_RESET_DEV_SPECIFIC) {
+		rc = pci_dev_specific_reset(dev, 1);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	if (reset_type & PCI_RESET_FLR) {
+		if (pcie_has_flr(dev))
+			return 0;
+		rc = pci_af_flr(dev, 1);
+		if (rc != -ENOTTY)
+			return rc;
+	}
 
-	return pci_parent_bus_reset(dev, 1);
+	if (reset_type & PCI_RESET_PM) {
+		rc = pci_pm_reset(dev, 1);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	if (reset_type & PCI_RESET_SLOT) {
+		rc = pci_dev_reset_slot_function(dev, 1);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	if (reset_type & PCI_RESET_BUS) {
+		rc = pci_parent_bus_reset(dev, 1);
+		if (rc != -ENOTTY)
+			return rc;
+	}
+
+	return rc;
 }
 
 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6e0d1528d471..0444bfa51b52 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -33,7 +33,7 @@ enum pci_mmap_api {
 int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
 		  enum pci_mmap_api mmap_api);
 
-int pci_probe_reset_function(struct pci_dev *dev);
+int pci_probe_reset_function(struct pci_dev *dev, u32 reset_type);
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 
 /**
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ec784009a36b..cbce5420431b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2332,7 +2332,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 
 	pcie_report_downtraining(dev);
 
-	if (pci_probe_reset_function(dev) == 0)
+	if (pci_probe_reset_function(dev, PCI_RESET_ANY) == 0)
 		dev->reset_fn = 1;
 }
 
-- 
2.18.0

[PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()

[Sinan Kaya]

Looking to have more control between the users of the API vs. what the API
can do internally. The new reset_type tells the PCI core about the bounds
of the request.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 drivers/pci/pci.c           | 18 +++++++++++++++---
 drivers/vfio/pci/vfio_pci.c |  6 ++++--
 include/linux/pci.h         |  2 +-
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b4f14419bd51..f11d29f32119 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)
 /**
  * pci_reset_bus - Try to reset a PCI bus
  * @pdev: top level PCI device to reset via slot/bus
+ * @reset_type: resets to try
  *
  * Same as above except return -EAGAIN if the bus cannot be locked
  */
-int pci_reset_bus(struct pci_dev *pdev)
+int pci_reset_bus(struct pci_dev *pdev, u32 reset_type)
 {
-	return (!pci_probe_reset_slot(pdev->slot)) ?
-	    __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
+	if (reset_type & PCI_RESET_LINK) {
+		return (!pci_probe_reset_slot(pdev->slot)) ?
+				__pci_reset_slot(pdev->slot) :
+				__pci_reset_bus(pdev->bus);
+	}
+
+	if (reset_type & PCI_RESET_BUS)
+		return __pci_reset_bus(pdev->bus);
+
+	if (reset_type & PCI_RESET_SLOT)
+		return __pci_reset_slot(pdev->slot);
+
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(pci_reset_bus);
 
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index fe7ada997c51..0e80c72b1eaa 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data,
 						    &info, slot);
 		if (!ret)
 			/* User has access, do the reset */
-			ret = pci_reset_bus(vdev->pdev);
+			ret = pci_reset_bus(vdev->pdev,
+				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
 
 hot_reset_release:
 		for (i--; i >= 0; i--)
@@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 	}
 
 	if (needs_reset)
-		ret = pci_reset_bus(vdev->pdev);
+		ret = pci_reset_bus(vdev->pdev,
+				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
 
 put_devs:
 	for (i = 0; i < devs.cur_index; i++) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4fdddcb85066..85f48e156753 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
 int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
 int pci_probe_reset_slot(struct pci_slot *slot);
 int pci_probe_reset_bus(struct pci_bus *bus);
-int pci_reset_bus(struct pci_dev *dev);
+int pci_reset_bus(struct pci_dev *dev, u32 reset_type);
 void pci_reset_secondary_bus(struct pci_dev *dev);
 void pcibios_reset_secondary_bus(struct pci_dev *dev);
 void pci_update_resource(struct pci_dev *dev, int resno);
-- 
2.18.0

Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()

[Alex Williamson]

On Wed, 19 Sep 2018 16:30:37 +0000
Sinan Kaya <okaya@kernel.org> wrote:

> Looking to have more control between the users of the API vs. what the API
> can do internally. The new reset_type tells the PCI core about the bounds
> of the request.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> ---
>  drivers/pci/pci.c           | 18 +++++++++++++++---
>  drivers/vfio/pci/vfio_pci.c |  6 ++++--
>  include/linux/pci.h         |  2 +-
>  3 files changed, 20 insertions(+), 6 deletions(-)

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b4f14419bd51..f11d29f32119 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)
>  /**
>   * pci_reset_bus - Try to reset a PCI bus
>   * @pdev: top level PCI device to reset via slot/bus
> + * @reset_type: resets to try
>   *
>   * Same as above except return -EAGAIN if the bus cannot be locked
>   */
> -int pci_reset_bus(struct pci_dev *pdev)
> +int pci_reset_bus(struct pci_dev *pdev, u32 reset_type)
>  {
> -	return (!pci_probe_reset_slot(pdev->slot)) ?
> -	    __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
> +	if (reset_type & PCI_RESET_LINK) {
> +		return (!pci_probe_reset_slot(pdev->slot)) ?
> +				__pci_reset_slot(pdev->slot) :
> +				__pci_reset_bus(pdev->bus);
> +	}
> +
> +	if (reset_type & PCI_RESET_BUS)
> +		return __pci_reset_bus(pdev->bus);
> +
> +	if (reset_type & PCI_RESET_SLOT)
> +		return __pci_reset_slot(pdev->slot);
> +
> +	return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(pci_reset_bus);
>  
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index fe7ada997c51..0e80c72b1eaa 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data,
>  						    &info, slot);
>  		if (!ret)
>  			/* User has access, do the reset */
> -			ret = pci_reset_bus(vdev->pdev);
> +			ret = pci_reset_bus(vdev->pdev,
> +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
>  
>  hot_reset_release:
>  		for (i--; i >= 0; i--)
> @@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
>  	}
>  
>  	if (needs_reset)
> -		ret = pci_reset_bus(vdev->pdev);
> +		ret = pci_reset_bus(vdev->pdev,
> +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
>  
>  put_devs:
>  	for (i = 0; i < devs.cur_index; i++) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4fdddcb85066..85f48e156753 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
>  int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
>  int pci_probe_reset_slot(struct pci_slot *slot);
>  int pci_probe_reset_bus(struct pci_bus *bus);
> -int pci_reset_bus(struct pci_dev *dev);
> +int pci_reset_bus(struct pci_dev *dev, u32 reset_type);
>  void pci_reset_secondary_bus(struct pci_dev *dev);
>  void pcibios_reset_secondary_bus(struct pci_dev *dev);
>  void pci_update_resource(struct pci_dev *dev, int resno);

Re: [PATCH v4 4/6] PCI: Expose reset type to users of pci_try_reset_function()

[Alex Williamson]

On Wed, 19 Sep 2018 16:30:35 +0000
Sinan Kaya <okaya@kernel.org> wrote:

> Looking to have more control between the users of the API vs. what the API
> can do internally. The new reset_type tells the PCI core about the bounds
> of the request.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> ---
>  drivers/pci/pci.c                  | 4 ++--
>  drivers/vfio/pci/vfio_pci.c        | 7 ++++---
>  drivers/vfio/pci/vfio_pci_config.c | 4 ++--
>  include/linux/pci.h                | 2 +-
>  4 files changed, 9 insertions(+), 8 deletions(-)

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 63b5930d8fb1..4edf70a4d528 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4852,7 +4852,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);
>   *
>   * Same as above, except return -EAGAIN if unable to lock device.
>   */
> -int pci_try_reset_function(struct pci_dev *dev)
> +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type)
>  {
>  	int rc;
>  
> @@ -4863,7 +4863,7 @@ int pci_try_reset_function(struct pci_dev *dev)
>  		return -EAGAIN;
>  
>  	pci_dev_save_and_disable(dev);
> -	rc = __pci_reset_function_locked(dev, PCI_RESET_ANY);
> +	rc = __pci_reset_function_locked(dev, reset_type);
>  	pci_dev_restore(dev);
>  	pci_dev_unlock(dev);
>  
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index cddb453a1ba5..fe7ada997c51 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -228,7 +228,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>  		return ret;
>  
>  	/* If reset fails because of the device lock, fail this path entirely */
> -	ret = pci_try_reset_function(pdev);
> +	ret = pci_try_reset_function(pdev, PCI_RESET_ANY);
>  	if (ret == -EAGAIN) {
>  		pci_disable_device(pdev);
>  		return ret;
> @@ -376,7 +376,7 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>  	 * Try to reset the device.  The success of this is dependent on
>  	 * being able to lock the device, which is not always possible.
>  	 */
> -	if (vdev->reset_works && !pci_try_reset_function(pdev))
> +	if (vdev->reset_works && !pci_try_reset_function(pdev, PCI_RESET_ANY))
>  		vdev->needs_reset = false;
>  
>  	pci_restore_state(pdev);
> @@ -844,7 +844,8 @@ static long vfio_pci_ioctl(void *device_data,
>  
>  	} else if (cmd == VFIO_DEVICE_RESET) {
>  		return vdev->reset_works ?
> -			pci_try_reset_function(vdev->pdev) : -EINVAL;
> +			pci_try_reset_function(vdev->pdev, PCI_RESET_ANY) :
> +			 -EINVAL;
>  
>  	} else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
>  		struct vfio_pci_hot_reset_info hdr;
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 115a36f6f403..0d66bac66211 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -831,7 +831,7 @@ static int vfio_exp_config_write(struct vfio_pci_device *vdev, int pos,
>  						 &cap);
>  
>  		if (!ret && (cap & PCI_EXP_DEVCAP_FLR))
> -			pci_try_reset_function(vdev->pdev);
> +			pci_try_reset_function(vdev->pdev, PCI_RESET_ANY);
>  	}
>  
>  	/*
> @@ -910,7 +910,7 @@ static int vfio_af_config_write(struct vfio_pci_device *vdev, int pos,
>  						&cap);
>  
>  		if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP))
> -			pci_try_reset_function(vdev->pdev);
> +			pci_try_reset_function(vdev->pdev, PCI_RESET_ANY);
>  	}
>  
>  	return count;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 09aa99dd1c1b..4fdddcb85066 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1125,7 +1125,7 @@ int pcie_flr(struct pci_dev *dev);
>  int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
>  int pci_reset_function(struct pci_dev *dev, u32 reset_type);
>  int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
> -int pci_try_reset_function(struct pci_dev *dev);
> +int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
>  int pci_probe_reset_slot(struct pci_slot *slot);
>  int pci_probe_reset_bus(struct pci_bus *bus);
>  int pci_reset_bus(struct pci_dev *dev);

Re: [PATCH v4 0/6] PCI: Add reset type parameter to PCI reset functions

[Alex Williamson]

On Wed, 19 Sep 2018 16:30:31 +0000
Sinan Kaya <okaya@kernel.org> wrote:

> We want more control over what kind of reset PCI core can apply. Have the
> application explicitly requested allowed reset types.
> 
> differences from v3:
> - set default return value to -EINVAL in __pci_reset_function_locked()
> 
> Sinan Kaya (6):
>   PCI: Expose reset_type to users of __pci_reset_function_locked()
>   PCI: Expose reset_type to users of pci_reset_function()
>   PCI: Expose reset_type to users of pci_reset_function_locked()
>   PCI: Expose reset type to users of pci_try_reset_function()
>   PCI: Expose reset type to users of pci_probe_reset_function()
>   PCI: Expose reset type to users of pci_reset_bus()
> 
>  drivers/hid/intel-ish-hid/ipc/ipc.c           |   2 +-
>  drivers/misc/genwqe/card_base.c               |   2 +-
>  .../net/ethernet/cavium/liquidio/lio_main.c   |   2 +-
>  .../net/ethernet/qlogic/qlcnic/qlcnic_ctx.c   |   2 +-
>  drivers/net/ethernet/sfc/mcdi.c               |   2 +-
>  drivers/pci/pci-sysfs.c                       |   2 +-
>  drivers/pci/pci.c                             | 134 ++++++++++++------
>  drivers/pci/pci.h                             |   2 +-
>  drivers/pci/probe.c                           |   2 +-
>  drivers/vfio/pci/vfio_pci.c                   |  13 +-
>  drivers/vfio/pci/vfio_pci_config.c            |   4 +-
>  drivers/xen/xen-pciback/pci_stub.c            |   6 +-
>  include/linux/pci.h                           |  21 ++-
>  13 files changed, 128 insertions(+), 66 deletions(-)
> 

Ack'd 4 & 6, for the remainder,

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Re: [PATCH v4 1/6] PCI: Expose reset_type to users of __pci_reset_function_locked()

[Bjorn Helgaas]

On Wed, Sep 19, 2018 at 04:30:32PM +0000, Sinan Kaya wrote:
> We need a contract between the reset API users and the PCI core about the
> types of reset that a user needs vs. what PCI core can do internally.
> If a platform supports hotplug, we need to do hotplug reset as an example.
> 
> Expose the reset types to the drivers and try different reset types based
> on the new reset_type parameter.
> 
> Most users are expected to use PCI_RESET_ANY, PCI_RESET_FUNC or
> PCI_RESET_LINK parameters.

I understand the words above, but I don't really understand the
motivation behind this.  Can you help flesh that out a bit?

I guess there's a problem being solved by these patches, and it would
help a lot to have some details about what it is.

It would be ideal if we could offer some guidance, probably as code
comments, to help driver writers choose the correct reset type.

> +	if (reset_type & PCI_RESET_FLR) {
> +		if (pcie_has_flr(dev)) {
> +			rc = pcie_flr(dev);
> +			if (rc != -ENOTTY)
> +				return rc;
> +		}
> +		rc = pci_af_flr(dev, 0);

This is a tangent and not material for the current series, so just
file it away so as not to delay this series, but is there any way we
can get rid of the "probe" arguments?  Maybe do all the probes at
enumeration-time and remember the results?

I know there are one or two cases where you need to look at more than
one device, e.g., a secondary bus reset might be possible if there's
only one device and not otherwise, but it seems like in most cases
this is a property of the individual device.

Bjorn

Re: [PATCH v4 2/6] PCI: Expose reset_type to users of pci_reset_function()

[Bjorn Helgaas]

On Wed, Sep 19, 2018 at 04:30:33PM +0000, Sinan Kaya wrote:
> Looking to have more control between the users of the API vs. what the API
> can do internally. The new reset_type tells the PCI core about the bounds
> of the request.

I'd like to know why you chose PCI_RESET_ANY for these drivers.

> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> (drivers/hid/intel-ish-hid/ipc/ipc.c)
> ---
>  drivers/hid/intel-ish-hid/ipc/ipc.c             | 2 +-
>  drivers/misc/genwqe/card_base.c                 | 2 +-
>  drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 2 +-
>  drivers/net/ethernet/sfc/mcdi.c                 | 2 +-
>  drivers/pci/pci-sysfs.c                         | 2 +-
>  drivers/pci/pci.c                               | 4 ++--
>  include/linux/pci.h                             | 2 +-
>  7 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
> index bfbca7ec54ce..18312969f1b3 100644
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -754,7 +754,7 @@ static int _ish_hw_reset(struct ishtp_device *dev)
>  	if (!pdev)
>  		return	-ENODEV;
>  
> -	rv = pci_reset_function(pdev);
> +	rv = pci_reset_function(pdev, PCI_RESET_ANY);
>  	if (!rv)
>  		dev->dev_state = ISHTP_DEV_RESETTING;
>  
> diff --git a/drivers/misc/genwqe/card_base.c b/drivers/misc/genwqe/card_base.c
> index c7cd3675bcd1..cc78ef28ee38 100644
> --- a/drivers/misc/genwqe/card_base.c
> +++ b/drivers/misc/genwqe/card_base.c
> @@ -201,7 +201,7 @@ static int genwqe_bus_reset(struct genwqe_dev *cd)
>  	 * restored by the pci_reset_function().
>  	 */
>  	dev_dbg(&pci_dev->dev, "[%s] pci_reset function ...\n", __func__);
> -	rc = pci_reset_function(pci_dev);
> +	rc = pci_reset_function(pci_dev, PCI_RESET_ANY);
>  	if (rc) {
>  		dev_err(&pci_dev->dev,
>  			"[%s] err: failed reset func (rc %d)\n", __func__, rc);
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
> index d344e9d43832..bb737725f175 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c
> @@ -629,7 +629,7 @@ int qlcnic_fw_create_ctx(struct qlcnic_adapter *dev)
>  	int i, err, ring;
>  
>  	if (dev->flags & QLCNIC_NEED_FLR) {
> -		pci_reset_function(dev->pdev);
> +		pci_reset_function(dev->pdev, PCI_RESET_ANY);
>  		dev->flags &= ~QLCNIC_NEED_FLR;
>  	}
>  
> diff --git a/drivers/net/ethernet/sfc/mcdi.c b/drivers/net/ethernet/sfc/mcdi.c
> index dfad93fca0a6..7f95e17b8a48 100644
> --- a/drivers/net/ethernet/sfc/mcdi.c
> +++ b/drivers/net/ethernet/sfc/mcdi.c
> @@ -1862,7 +1862,7 @@ int efx_mcdi_reset(struct efx_nic *efx, enum reset_type method)
>  
>  	/* If MCDI is down, we can't handle_assertion */
>  	if (method == RESET_TYPE_MCDI_TIMEOUT) {
> -		rc = pci_reset_function(efx->pci_dev);
> +		rc = pci_reset_function(efx->pci_dev, PCI_RESET_ANY);
>  		if (rc)
>  			return rc;
>  		/* Re-enable polled MCDI completion */
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 9ecfe13157c0..9569664ec4b2 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1449,7 +1449,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>  		return -EINVAL;
>  
>  	pm_runtime_get_sync(dev);
> -	result = pci_reset_function(pdev);
> +	result = pci_reset_function(pdev, PCI_RESET_ANY);
>  	pm_runtime_put(dev);
>  	if (result < 0)
>  		return result;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e292ea589d3e..04fd40472a93 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4793,7 +4793,7 @@ int pci_probe_reset_function(struct pci_dev *dev)
>   * 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(struct pci_dev *dev)
> +int pci_reset_function(struct pci_dev *dev, u32 reset_type)
>  {
>  	int rc;
>  
> @@ -4803,7 +4803,7 @@ int pci_reset_function(struct pci_dev *dev)
>  	pci_dev_lock(dev);
>  	pci_dev_save_and_disable(dev);
>  
> -	rc = __pci_reset_function_locked(dev, PCI_RESET_ANY);
> +	rc = __pci_reset_function_locked(dev, reset_type);
>  
>  	pci_dev_restore(dev);
>  	pci_dev_unlock(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d2e0a8c8177b..1a1a76130e91 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1123,7 +1123,7 @@ void pcie_print_link_status(struct pci_dev *dev);
>  bool pcie_has_flr(struct pci_dev *dev);
>  int pcie_flr(struct pci_dev *dev);
>  int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
> -int pci_reset_function(struct pci_dev *dev);
> +int pci_reset_function(struct pci_dev *dev, u32 reset_type);
>  int pci_reset_function_locked(struct pci_dev *dev);
>  int pci_try_reset_function(struct pci_dev *dev);
>  int pci_probe_reset_slot(struct pci_slot *slot);
> -- 
> 2.18.0
> 

Re: [PATCH v4 2/6] PCI: Expose reset_type to users of pci_reset_function()

[Sinan Kaya]

On 9/25/2018 4:46 PM, Bjorn Helgaas wrote:
>> Looking to have more control between the users of the API vs. what the API
>> can do internally. The new reset_type tells the PCI core about the bounds
>> of the request.
> I'd like to know why you chose PCI_RESET_ANY for these drivers.
> 

PCI_RESET_ANY was chosen to match the existing behavior in the code.

Current pci_reset_function() tries all reset types one by one until
it returns 0. By specifying PCI_RESET_ANY, we get the same thing.

Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()

[Bjorn Helgaas]

On Wed, Sep 19, 2018 at 04:30:37PM +0000, Sinan Kaya wrote:
> Looking to have more control between the users of the API vs. what the API
> can do internally. The new reset_type tells the PCI core about the bounds
> of the request.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> ---
>  drivers/pci/pci.c           | 18 +++++++++++++++---
>  drivers/vfio/pci/vfio_pci.c |  6 ++++--
>  include/linux/pci.h         |  2 +-
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b4f14419bd51..f11d29f32119 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)
>  /**
>   * pci_reset_bus - Try to reset a PCI bus
>   * @pdev: top level PCI device to reset via slot/bus
> + * @reset_type: resets to try
>   *
>   * Same as above except return -EAGAIN if the bus cannot be locked
>   */
> -int pci_reset_bus(struct pci_dev *pdev)
> +int pci_reset_bus(struct pci_dev *pdev, u32 reset_type)
>  {
> -	return (!pci_probe_reset_slot(pdev->slot)) ?
> -	    __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
> +	if (reset_type & PCI_RESET_LINK) {
> +		return (!pci_probe_reset_slot(pdev->slot)) ?
> +				__pci_reset_slot(pdev->slot) :
> +				__pci_reset_bus(pdev->bus);
> +	}
> +
> +	if (reset_type & PCI_RESET_BUS)
> +		return __pci_reset_bus(pdev->bus);
> +
> +	if (reset_type & PCI_RESET_SLOT)
> +		return __pci_reset_slot(pdev->slot);

I don't understand this code.  We have

  #define PCI_RESET_LINK         (PCI_RESET_SLOT | PCI_RESET_BUS)

so if either PCI_RESET_BUS or PCI_RESET_SLOT is set, we should take
the first "if" above, which always returns, and the last two should
never be reached.  What am I missing?

> +
> +	return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(pci_reset_bus);
>  
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index fe7ada997c51..0e80c72b1eaa 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data,
>  						    &info, slot);
>  		if (!ret)
>  			/* User has access, do the reset */
> -			ret = pci_reset_bus(vdev->pdev);
> +			ret = pci_reset_bus(vdev->pdev,
> +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);

This is the only place in the whole series where a caller uses
something other than PCI_RESET_ANY.  Inquiring minds want to know why.
I'm sure it's the right thing to do, it's just that we'll need to know
how to choose in future cases.

>  
>  hot_reset_release:
>  		for (i--; i >= 0; i--)
> @@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
>  	}
>  
>  	if (needs_reset)
> -		ret = pci_reset_bus(vdev->pdev);
> +		ret = pci_reset_bus(vdev->pdev,
> +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
>  
>  put_devs:
>  	for (i = 0; i < devs.cur_index; i++) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4fdddcb85066..85f48e156753 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
>  int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
>  int pci_probe_reset_slot(struct pci_slot *slot);
>  int pci_probe_reset_bus(struct pci_bus *bus);
> -int pci_reset_bus(struct pci_dev *dev);
> +int pci_reset_bus(struct pci_dev *dev, u32 reset_type);
>  void pci_reset_secondary_bus(struct pci_dev *dev);
>  void pcibios_reset_secondary_bus(struct pci_dev *dev);
>  void pci_update_resource(struct pci_dev *dev, int resno);
> -- 
> 2.18.0
> 

Re: [PATCH v4 1/6] PCI: Expose reset_type to users of __pci_reset_function_locked()

[Sinan Kaya]

On 9/25/2018 4:45 PM, Bjorn Helgaas wrote:
>> We need a contract between the reset API users and the PCI core about the
>> types of reset that a user needs vs. what PCI core can do internally.
>> If a platform supports hotplug, we need to do hotplug reset as an example.
>>
>> Expose the reset types to the drivers and try different reset types based
>> on the new reset_type parameter.
>>
>> Most users are expected to use PCI_RESET_ANY, PCI_RESET_FUNC or
>> PCI_RESET_LINK parameters.
> I understand the words above, but I don't really understand the
> motivation behind this.  Can you help flesh that out a bit?
> 
> I guess there's a problem being solved by these patches, and it would
> help a lot to have some details about what it is.
> 
> It would be ideal if we could offer some guidance, probably as code
> comments, to help driver writers choose the correct reset type.
> 

Alex and I discussed this here:

https://www.spinics.net/lists/linux-pci/msg75828.html

Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()

[Sinan Kaya]

On 9/25/2018 4:54 PM, Bjorn Helgaas wrote:
> On Wed, Sep 19, 2018 at 04:30:37PM +0000, Sinan Kaya wrote:
>> Looking to have more control between the users of the API vs. what the API
>> can do internally. The new reset_type tells the PCI core about the bounds
>> of the request.
>>
>> Signed-off-by: Sinan Kaya <okaya@kernel.org>
>> ---
>>   drivers/pci/pci.c           | 18 +++++++++++++++---
>>   drivers/vfio/pci/vfio_pci.c |  6 ++++--
>>   include/linux/pci.h         |  2 +-
>>   3 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b4f14419bd51..f11d29f32119 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)
>>   /**
>>    * pci_reset_bus - Try to reset a PCI bus
>>    * @pdev: top level PCI device to reset via slot/bus
>> + * @reset_type: resets to try
>>    *
>>    * Same as above except return -EAGAIN if the bus cannot be locked
>>    */
>> -int pci_reset_bus(struct pci_dev *pdev)
>> +int pci_reset_bus(struct pci_dev *pdev, u32 reset_type)
>>   {
>> -	return (!pci_probe_reset_slot(pdev->slot)) ?
>> -	    __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
>> +	if (reset_type & PCI_RESET_LINK) {
>> +		return (!pci_probe_reset_slot(pdev->slot)) ?
>> +				__pci_reset_slot(pdev->slot) :
>> +				__pci_reset_bus(pdev->bus);
>> +	}
>> +
>> +	if (reset_type & PCI_RESET_BUS)
>> +		return __pci_reset_bus(pdev->bus);
>> +
>> +	if (reset_type & PCI_RESET_SLOT)
>> +		return __pci_reset_slot(pdev->slot);
> 
> I don't understand this code.  We have
> 
>    #define PCI_RESET_LINK         (PCI_RESET_SLOT | PCI_RESET_BUS)
> 
> so if either PCI_RESET_BUS or PCI_RESET_SLOT is set, we should take
> the first "if" above, which always returns, and the last two should
> never be reached.  What am I missing?

Yup, this is broken. I need to follow up with another patchset.

I was trying to cover the case where user said,

"I need a link reset. I don't care whether it is a slot reset or bus reset
as long as we achieve a reset."

That's when PCI_RESET_LINK is used. We expect most drivers to PCI_RESET_LINK
if they are interested in a link reset and doesn't need to know about the
hotplug capability of a particular slot.

> 
>> +
>> +	return -EINVAL;
>>   }
>>   EXPORT_SYMBOL_GPL(pci_reset_bus);
>>   
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index fe7ada997c51..0e80c72b1eaa 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data,
>>   						    &info, slot);
>>   		if (!ret)
>>   			/* User has access, do the reset */
>> -			ret = pci_reset_bus(vdev->pdev);
>> +			ret = pci_reset_bus(vdev->pdev,
>> +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
> 
> This is the only place in the whole series where a caller uses
> something other than PCI_RESET_ANY.  Inquiring minds want to know why.
> I'm sure it's the right thing to do, it's just that we'll need to know
> how to choose in future cases.

Use PCI_RESET_ANY for the existing behavior where you want one type of
reset and don't care if it is PM/function/slot/bus/specific.

Use PCI_RESET_LINK when you need link to retrain.

Use PCI_RESET_SLOT when you know that this system is hotplug capable
by calling probe functions.

Use PCI_RESET_BUS when you know that this system is not hotplug capable
and this endpoint will never be used in such a system.

I can capture this into the commit message.

> 
>>   
>>   hot_reset_release:
>>   		for (i--; i >= 0; i--)
>> @@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
>>   	}
>>   
>>   	if (needs_reset)
>> -		ret = pci_reset_bus(vdev->pdev);
>> +		ret = pci_reset_bus(vdev->pdev,
>> +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
>>   
>>   put_devs:
>>   	for (i = 0; i < devs.cur_index; i++) {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 4fdddcb85066..85f48e156753 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
>>   int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
>>   int pci_probe_reset_slot(struct pci_slot *slot);
>>   int pci_probe_reset_bus(struct pci_bus *bus);
>> -int pci_reset_bus(struct pci_dev *dev);
>> +int pci_reset_bus(struct pci_dev *dev, u32 reset_type);
>>   void pci_reset_secondary_bus(struct pci_dev *dev);
>>   void pcibios_reset_secondary_bus(struct pci_dev *dev);
>>   void pci_update_resource(struct pci_dev *dev, int resno);
>> -- 
>> 2.18.0
>>
> 

Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()

[Bjorn Helgaas]

On Tue, Sep 25, 2018 at 05:15:52PM -0400, Sinan Kaya wrote:
> On 9/25/2018 4:54 PM, Bjorn Helgaas wrote:
> > On Wed, Sep 19, 2018 at 04:30:37PM +0000, Sinan Kaya wrote:
> > > Looking to have more control between the users of the API vs. what the API
> > > can do internally. The new reset_type tells the PCI core about the bounds
> > > of the request.
> > > 
> > > Signed-off-by: Sinan Kaya <okaya@kernel.org>
> > > ---
> > >   drivers/pci/pci.c           | 18 +++++++++++++++---
> > >   drivers/vfio/pci/vfio_pci.c |  6 ++++--
> > >   include/linux/pci.h         |  2 +-
> > >   3 files changed, 20 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index b4f14419bd51..f11d29f32119 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5232,13 +5232,25 @@ static int __pci_reset_bus(struct pci_bus *bus)
> > >   /**
> > >    * pci_reset_bus - Try to reset a PCI bus
> > >    * @pdev: top level PCI device to reset via slot/bus
> > > + * @reset_type: resets to try
> > >    *
> > >    * Same as above except return -EAGAIN if the bus cannot be locked
> > >    */
> > > -int pci_reset_bus(struct pci_dev *pdev)
> > > +int pci_reset_bus(struct pci_dev *pdev, u32 reset_type)
> > >   {
> > > -	return (!pci_probe_reset_slot(pdev->slot)) ?
> > > -	    __pci_reset_slot(pdev->slot) : __pci_reset_bus(pdev->bus);
> > > +	if (reset_type & PCI_RESET_LINK) {
> > > +		return (!pci_probe_reset_slot(pdev->slot)) ?
> > > +				__pci_reset_slot(pdev->slot) :
> > > +				__pci_reset_bus(pdev->bus);
> > > +	}
> > > +
> > > +	if (reset_type & PCI_RESET_BUS)
> > > +		return __pci_reset_bus(pdev->bus);
> > > +
> > > +	if (reset_type & PCI_RESET_SLOT)
> > > +		return __pci_reset_slot(pdev->slot);
> > 
> > I don't understand this code.  We have
> > 
> >    #define PCI_RESET_LINK         (PCI_RESET_SLOT | PCI_RESET_BUS)
> > 
> > so if either PCI_RESET_BUS or PCI_RESET_SLOT is set, we should take
> > the first "if" above, which always returns, and the last two should
> > never be reached.  What am I missing?
> 
> Yup, this is broken. I need to follow up with another patchset.
> 
> I was trying to cover the case where user said,
> 
> "I need a link reset. I don't care whether it is a slot reset or bus reset
> as long as we achieve a reset."
> 
> That's when PCI_RESET_LINK is used. We expect most drivers to PCI_RESET_LINK
> if they are interested in a link reset and doesn't need to know about the
> hotplug capability of a particular slot.
> 
> > 
> > > +
> > > +	return -EINVAL;
> > >   }
> > >   EXPORT_SYMBOL_GPL(pci_reset_bus);
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index fe7ada997c51..0e80c72b1eaa 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -1015,7 +1015,8 @@ static long vfio_pci_ioctl(void *device_data,
> > >   						    &info, slot);
> > >   		if (!ret)
> > >   			/* User has access, do the reset */
> > > -			ret = pci_reset_bus(vdev->pdev);
> > > +			ret = pci_reset_bus(vdev->pdev,
> > > +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
> > 
> > This is the only place in the whole series where a caller uses
> > something other than PCI_RESET_ANY.  Inquiring minds want to know why.
> > I'm sure it's the right thing to do, it's just that we'll need to know
> > how to choose in future cases.
> 
> Use PCI_RESET_ANY for the existing behavior where you want one type of
> reset and don't care if it is PM/function/slot/bus/specific.
> 
> Use PCI_RESET_LINK when you need link to retrain.

There are no callers using this.  Is this intended specifically for
the case of "the hardware wasn't smart enough to train at the correct
speed, so we fiddled things and want to retrain"?

Maybe it doesn't need to be exposed in include/linux/pci.h and could
be defined internally in drivers/pci/pci.c if it's needed there?

> Use PCI_RESET_SLOT when you know that this system is hotplug capable
> by calling probe functions.

I raise my eyebrows at this because (a) a driver generally can't tell
whether hotplug is supported and (b) even if the driver does know,
what is the benefit to the driver to specify this?  What probe
functions does this refer to?  If I'm a driver writer, I really can't
tell what I'm supposed to do with this guidance.  If I'm supposed to
call a probe function, what is it, when should I call it, and what
should I do with the result?  Am I supposed to know whether hotplug is
supported?  Why would I care?

I'm sure you have good answers for all these questions; I just don't
know what they are :)

> Use PCI_RESET_BUS when you know that this system is not hotplug capable
> and this endpoint will never be used in such a system.

How can a driver know this?  And what's the benefit of being specific?

> I can capture this into the commit message.

I think it needs to be more accessible, e.g., comments near the constants
and/or the function declaration.  We shouldn't expect users of the
interface to dig through the changelogs for it.

> > >   hot_reset_release:
> > >   		for (i--; i >= 0; i--)
> > > @@ -1390,7 +1391,8 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
> > >   	}
> > >   	if (needs_reset)
> > > -		ret = pci_reset_bus(vdev->pdev);
> > > +		ret = pci_reset_bus(vdev->pdev,
> > > +				slot ? PCI_RESET_SLOT :	PCI_RESET_BUS);
> > >   put_devs:
> > >   	for (i = 0; i < devs.cur_index; i++) {
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 4fdddcb85066..85f48e156753 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -1128,7 +1128,7 @@ int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
> > >   int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
> > >   int pci_probe_reset_slot(struct pci_slot *slot);
> > >   int pci_probe_reset_bus(struct pci_bus *bus);
> > > -int pci_reset_bus(struct pci_dev *dev);
> > > +int pci_reset_bus(struct pci_dev *dev, u32 reset_type);
> > >   void pci_reset_secondary_bus(struct pci_dev *dev);
> > >   void pcibios_reset_secondary_bus(struct pci_dev *dev);
> > >   void pci_update_resource(struct pci_dev *dev, int resno);
> > > -- 
> > > 2.18.0
> > > 
> > 
> 

Re: [PATCH v4 1/6] PCI: Expose reset_type to users of __pci_reset_function_locked()

[Bjorn Helgaas]

On Tue, Sep 25, 2018 at 05:07:17PM -0400, Sinan Kaya wrote:
> On 9/25/2018 4:45 PM, Bjorn Helgaas wrote:
> > > We need a contract between the reset API users and the PCI core about the
> > > types of reset that a user needs vs. what PCI core can do internally.
> > > If a platform supports hotplug, we need to do hotplug reset as an example.
> > > 
> > > Expose the reset types to the drivers and try different reset types based
> > > on the new reset_type parameter.
> > > 
> > > Most users are expected to use PCI_RESET_ANY, PCI_RESET_FUNC or
> > > PCI_RESET_LINK parameters.
> > I understand the words above, but I don't really understand the
> > motivation behind this.  Can you help flesh that out a bit?
> > 
> > I guess there's a problem being solved by these patches, and it would
> > help a lot to have some details about what it is.
> > 
> > It would be ideal if we could offer some guidance, probably as code
> > comments, to help driver writers choose the correct reset type.
> 
> Alex and I discussed this here:
> 
> https://www.spinics.net/lists/linux-pci/msg75828.html

Thanks; needs to be distilled and included in the code as comments.

Re: [PATCH v4 2/6] PCI: Expose reset_type to users of pci_reset_function()

[Bjorn Helgaas]

On Tue, Sep 25, 2018 at 04:52:38PM -0400, Sinan Kaya wrote:
> On 9/25/2018 4:46 PM, Bjorn Helgaas wrote:
> > > Looking to have more control between the users of the API vs. what the API
> > > can do internally. The new reset_type tells the PCI core about the bounds
> > > of the request.
> > I'd like to know why you chose PCI_RESET_ANY for these drivers.
> > 
> 
> PCI_RESET_ANY was chosen to match the existing behavior in the code.
> 
> Current pci_reset_function() tries all reset types one by one until
> it returns 0. By specifying PCI_RESET_ANY, we get the same thing.

That would be useful information for the changelog.

Re: [PATCH v4 6/6] PCI: Expose reset type to users of pci_reset_bus()

[Sinan Kaya]

On 9/25/2018 5:56 PM, Bjorn Helgaas wrote:
>> Use PCI_RESET_LINK when you need link to retrain.
> There are no callers using this.  Is this intended specifically for
> the case of "the hardware wasn't smart enough to train at the correct
> speed, so we fiddled things and want to retrain"?
> 

Yup, I'll go ahead and change the hfi1 driver to use PCI_RESET_LINK
as originally planned so that there is an actual user.

These constants are intended to be used by the user of pci_reset
APIs. I think it makes sense to keep them there as well. I see
your point that PCI_RESET_LINK was not used. We should also make the hfi1
follow the rules.

> Maybe it doesn't need to be exposed in include/linux/pci.h and could
> be defined internally in drivers/pci/pci.c if it's needed there?
> 

see above.

>> Use PCI_RESET_SLOT when you know that this system is hotplug capable
>> by calling probe functions.
> I raise my eyebrows at this because (a) a driver generally can't tell
> whether hotplug is supported and (b) even if the driver does know,
> what is the benefit to the driver to specify this?  What probe
> functions does this refer to?  If I'm a driver writer, I really can't
> tell what I'm supposed to do with this guidance.  If I'm supposed to
> call a probe function, what is it, when should I call it, and what
> should I do with the result?  Am I supposed to know whether hotplug is
> supported?  Why would I care?

That was the whole reason why I hid the secondary bus reset API from the
user and folded into pci_reset_bus() so that PCI core can deal with hotplug
internally and can go to hotplug reset or bus reset internally.

User just calls pci_reset_bus().

I fully agree with your assessment but there are also exceptions like VFIO
where the code finds out which particular devices are part of a slot hierarchy
and for each one it checks that VFIO ownership has been claimed.
(Alex, please correct me if I'm not getting this right. I just read
200 lines of code in VFIO)

		/* Can we do a slot or bus reset or neither? */
		if (!pci_probe_reset_slot(vdev->pdev->slot))
			slot = true;
		else if (pci_probe_reset_bus(vdev->pdev->bus))
			return -ENODEV;

		ret = vfio_pci_for_each_slot_or_bus(vdev->pdev,
						    vfio_pci_fill_devs,
						    &fill, slot);



> 
> I'm sure you have good answers for all these questions; I just don't
> know what they are:)
> 
>> Use PCI_RESET_BUS when you know that this system is not hotplug capable
>> and this endpoint will never be used in such a system.
> How can a driver know this?  And what's the benefit of being specific?

There is really no benefit but there are drivers making assumptions about
the type of platform they want to run like supporting single segment only
etc.

PCI_RESET_BUS gives you direct access to secondary bus reset. If you are
calling this directly, you are responsible for saving and restoring state
like the hfi1 driver and need to ensure that this card will never work
on a hotplug capable system. If not, you are ...ed.

That's why, I'm suggesting that most users will use PCI_RESET_LINK so
that code works on all platforms.

> 
>> I can capture this into the commit message.
> I think it needs to be more accessible, e.g., comments near the constants
> and/or the function declaration.  We shouldn't expect users of the
> interface to dig through the changelogs for it.
> 

I can work on this once we agree if this is the way to go. I was responding
to Alex's request to have a contact between the PCI core and the drivers
because there are some driver that are more intelligent about the PCI tree
than a simple endpoint driver.