[PATCH v6 1/7] PCI: Expose reset_type to users of __pci_reset_function_locked()

[PATCH v6 1/7] 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.

Link: https://www.spinics.net/lists/linux-pci/msg75828.html
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                           | 58 +++++++++++++++++-
 4 files changed, 100 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..7ace46b3e479 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -849,6 +849,62 @@ enum {
 	PCI_SCAN_ALL_PCIE_DEVS	= 0x00000040,	/* Scan all, not just dev 0 */
 };
 
+/*
+ * A common theme here is that the driver wants some degree of
+ * control of the type of reset used, vfio specifically wants to specify a
+ * bus or slot reset while hfi1 just wants to specify that the link is
+ * reset and doesn't care if it's a bus or slot reset that accomplishes
+ * that.  The right "unified" interface is one that takes a parameter
+ * allowing the caller to specify the scope or type of reset
+ * with aliases so drivers can ignore the hotplug interface if they wish
+ * for special cases.
+ *
+ * PCI_RESET_ANY tries all reset type one by one until device is successfully
+ * reset. Under normal circumstances, most drivers are expected to use
+ * PCI_RESET_ANY as they don't usually care about the type of reset as long
+ * as device is reset.
+ *
+ * PCI_RESET_FUNC is useful when you want to reset one particular PCI device
+ * but you don't want to impact other devices or cause a temporary service
+ * outage.
+ *
+ * PCI_RESET_LINK can be used to cause a link retraining. This operation will
+ * cause service outage for the PCI bus if there are other devices on the same
+ * bus. PCI_RESET_LINK determines if a platform supports hotplug or not and
+ * suppresses hotplug interrupts during secondary bus reset.
+ *
+ * PCI_RESET_DEV_SPECIFIC can be used to reset a device by help from the
+ * device driver. Not all device drivers support this option.
+ *
+ * PCI_RESET_FLR can be used to issue a Function Level Reset to a device. This
+ * option will fail if FLR is not supported.
+ *
+ * PCI_RESET_PM can be used to reset the device via D3->D0 and D0->D3 sleep
+ * transition. This assumes that device supports PM based reset.
+ *
+ * PCI_RESET_SLOT forces a slot/hotplug reset. This will not work on platforms
+ * without hotplug capability. This option should only be used for advanced
+ * use-cases where driver developer absolutely knows that the device will never
+ * be used on non-hotplug environments.
+ * Not recommended for scalability. Please refer to PCI_RESET_LINK and let the
+ * PCI core do the hotplug detection.
+ *
+ * PCI_RESET_BUS performs a secondary bus reset on the link and causes a link
+ * recovery. Using this option directly and bypassing hotplug driver may
+ * cause a deadlock if platform supports hotplug. Please refer to
+ * PCI_RESET_LINK and let the PCI core do the hotplug detection.
+ */
+#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 +1167,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.19.0

[PATCH v6 2/7] 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.

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 behavior like
before.

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                               | 5 +++--
 include/linux/pci.h                             | 2 +-
 7 files changed, 9 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..66f102b7ed4e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4780,6 +4780,7 @@ int pci_probe_reset_function(struct pci_dev *dev)
 /**
  * pci_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
+ * @reset_type: reset type to apply
  *
  * Some devices allow an individual function to be reset without affecting
  * other functions in the same device.  The PCI device must be responsive
@@ -4793,7 +4794,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 +4804,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 7ace46b3e479..927e60660b96 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1168,7 +1168,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.19.0

[PATCH v6 3/7] 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   | 5 +++--
 include/linux/pci.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 66f102b7ed4e..9a649d1adb13 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4816,6 +4816,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function);
 /**
  * pci_reset_function_locked - quiesce and reset a PCI device function
  * @dev: PCI device to reset
+ * @reset_type: reset type to apply
  *
  * Some devices allow an individual function to be reset without affecting
  * other functions in the same device.  The PCI device must be responsive
@@ -4830,7 +4831,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;
 
@@ -4839,7 +4840,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 927e60660b96..9103ac1b3c31 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1169,7 +1169,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.19.0

[PATCH v6 4/7] 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                  | 5 +++--
 drivers/vfio/pci/vfio_pci.c        | 7 ++++---
 drivers/vfio/pci/vfio_pci_config.c | 4 ++--
 include/linux/pci.h                | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9a649d1adb13..7739f28988ae 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4851,10 +4851,11 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);
 /**
  * pci_try_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
+ * @reset_type: reset type to apply
  *
  * 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;
 
@@ -4865,7 +4866,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 9103ac1b3c31..cde63e0a85ea 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1170,7 +1170,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.19.0

[PATCH v6 5/7] 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   | 52 ++++++++++++++++++++++++++++++---------------
 drivers/pci/pci.h   |  2 +-
 drivers/pci/probe.c |  2 +-
 3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7739f28988ae..a1a7dd6988be 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4745,6 +4745,7 @@ EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
 /**
  * pci_probe_reset_function - check whether the device can be safely reset
  * @dev: PCI device to reset
+ * @reset_type: reset type to apply
  *
  * Some devices allow an individual function to be reset without affecting
  * other functions in the same device.  The PCI device must be responsive
@@ -4753,28 +4754,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 201f9e5ff55c..6c8f6af2d4aa 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2335,7 +2335,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.19.0

[PATCH v6 6/7] 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.

pci_reset_bus() calls different slot reset mechanisms based on given mask.
Slot based reset is prioritized over bus reset.

Users are expected to use PCI_RESET_SLOT mask by default.

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a1a7dd6988be..1feecbb7f85d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5236,13 +5236,19 @@ 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_SLOT) && !pci_probe_reset_slot(pdev->slot))
+		return __pci_reset_slot(pdev->slot);
+
+	if ((reset_type & PCI_RESET_BUS) && !pci_probe_reset_bus(pdev->bus))
+		return __pci_reset_bus(pdev->bus);
+
+	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 cde63e0a85ea..8ee4e3c177fe 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1173,7 +1173,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.19.0

[PATCH v6 7/7] IB/hfi1,PCI: switch to __pci_function_locked() for reset request

[Sinan Kaya]

Start using the new reset API rather than the workaround.

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

diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
index 6c967dde58e7..38f96192e5f0 100644
--- a/drivers/infiniband/hw/hfi1/pcie.c
+++ b/drivers/infiniband/hw/hfi1/pcie.c
@@ -897,7 +897,7 @@ static int trigger_sbr(struct hfi1_devdata *dd)
 	 * to be implemented to have cleaner interface but this fixes the
 	 * current brokenness
 	 */
-	return pci_bridge_secondary_bus_reset(dev->bus->self);
+	return __pci_reset_function_locked(dev, PCI_RESET_LINK);
 }
 
 /*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8ee4e3c177fe..cfb1018d774e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1291,9 +1291,6 @@ void pci_bus_remove_resources(struct pci_bus *bus);
 int devm_request_pci_bus_resources(struct device *dev,
 				   struct list_head *resources);
 
-/* Temporary until new and working PCI SBR API in place */
-int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
-
 #define pci_bus_for_each_resource(bus, res, i)				\
 	for (i = 0;							\
 	    (res = pci_bus_resource_n(bus, i)) || i < PCI_BRIDGE_RESOURCE_NUM; \
-- 
2.19.0

Re: [PATCH v6 7/7] IB/hfi1,PCI: switch to __pci_function_locked() for reset request

[Doug Ledford]

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

On Fri, 2018-10-19 at 02:11 +0000, Sinan Kaya wrote:
> Start using the new reset API rather than the workaround.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>

Acked-by: Doug Ledford <dledford@redhat.com>

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bjorn Helgaas]

[+cc mwifiex maintainers, Brian]

On Fri, Oct 19, 2018 at 02:11:24AM +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                  | 5 +++--
>  drivers/vfio/pci/vfio_pci.c        | 7 ++++---
>  drivers/vfio/pci/vfio_pci_config.c | 4 ++--
>  include/linux/pci.h                | 2 +-

Doesn't mwifiex_pcie_card_reset_work() also use
pci_try_reset_function() and need an update?

vfio is special in several ways, but I'm a little dubious about
mwifiex since it's the only other driver in the whole tree that uses
pci_try_reset_function().

The mwifiex use was added by a64e7a79dd60 ("mwifiex: resolve reset vs.
remove()/shutdown() deadlocks").

>  4 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9a649d1adb13..7739f28988ae 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4851,10 +4851,11 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);
>  /**
>   * pci_try_reset_function - quiesce and reset a PCI device function
>   * @dev: PCI device to reset
> + * @reset_type: reset type to apply
>   *
>   * 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;
>  
> @@ -4865,7 +4866,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 9103ac1b3c31..cde63e0a85ea 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1170,7 +1170,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.19.0
> 

Re: [PATCH v6 3/7] PCI: Expose reset_type to users of pci_reset_function_locked()

[Bjorn Helgaas]

On Fri, Oct 19, 2018 at 02:11:23AM +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   | 5 +++--
>  include/linux/pci.h | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 66f102b7ed4e..9a649d1adb13 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4816,6 +4816,7 @@ EXPORT_SYMBOL_GPL(pci_reset_function);
>  /**
>   * pci_reset_function_locked - quiesce and reset a PCI device function
>   * @dev: PCI device to reset
> + * @reset_type: reset type to apply
>   *
>   * Some devices allow an individual function to be reset without affecting
>   * other functions in the same device.  The PCI device must be responsive
> @@ -4830,7 +4831,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;
>  
> @@ -4839,7 +4840,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 927e60660b96..9103ac1b3c31 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1169,7 +1169,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);

There are no callers of this.  Why are we updating it rather than
removing it?

>  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.19.0
> 

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

[Brian Norris]

On Fri, Oct 19, 2018 at 1:14 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc mwifiex maintainers, Brian]
>
> On Fri, Oct 19, 2018 at 02:11:24AM +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                  | 5 +++--
> >  drivers/vfio/pci/vfio_pci.c        | 7 ++++---
> >  drivers/vfio/pci/vfio_pci_config.c | 4 ++--
> >  include/linux/pci.h                | 2 +-
>
> Doesn't mwifiex_pcie_card_reset_work() also use
> pci_try_reset_function() and need an update?

Yes :)

> vfio is special in several ways, but I'm a little dubious about
> mwifiex since it's the only other driver in the whole tree that uses
> pci_try_reset_function().
>
> The mwifiex use was added by a64e7a79dd60 ("mwifiex: resolve reset vs.
> remove()/shutdown() deadlocks").

I won't say that patch is perfect (there are cases where we'll end up
failing to try to reset, and the device will just stay dead instead),
but it definitely resolved deadlocks in a way that looked reasonable
to me. Feel free to poke holes in my original commit description, or
else ask questions, if you'd like mwifiex to do something differently.

Brian

> >  4 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 9a649d1adb13..7739f28988ae 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4851,10 +4851,11 @@ EXPORT_SYMBOL_GPL(pci_reset_function_locked);
> >  /**
> >   * pci_try_reset_function - quiesce and reset a PCI device function
> >   * @dev: PCI device to reset
> > + * @reset_type: reset type to apply
> >   *
> >   * 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;
> >
> > @@ -4865,7 +4866,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 9103ac1b3c31..cde63e0a85ea 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1170,7 +1170,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.19.0
> >

Re: [PATCH v6 3/7] PCI: Expose reset_type to users of pci_reset_function_locked()

[Sinan Kaya]

On 10/19/2018 4:20 PM, Bjorn Helgaas wrote:
>> +int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
> There are no callers of this.  Why are we updating it rather than
> removing it?
> 

I'll remove this function on another patch.

I transition all __pci_reset_function_locked users
to pci_reset_function_locked()  first.

Then all pci_reset_function_locked() users to pci_reset_function().

Then, git rid of pci_reset_function_locked() at last step.

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

[Bjorn Helgaas]

This series (and v5, it looks like) lost the cover letter, which had a
nice diffstat overview :)

On Fri, Oct 19, 2018 at 02:11:21AM +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.

Somewhere this needs a description of a bug that's being fixed or some
other justification, e.g., code simplification.  The above is a little
too abstract for me to grasp it.

> 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.

There are fewer than a dozen callers of all these functions and the
complication of these interfaces doesn't seem commensurate with the
problem.  With six different interfaces and five independent bit
flags, the possibilities are way more than necessary.

It seems like there are only three main cases (possibly extended by
locked/unlocked versions):

  - a generic reset, used by ipc, genwqe, qlcnic, sfc, liquidio,
    mwifiex, xen

  - something special for VFIO that is affected by the set of devices
    owned by the user

  - a link reset for places like these where a device needs help to
    train the link correctly:

      cik_pcie_gen3_enable()       # amdgpu, radeon
      si_pcie_gen3_enable()        # amdgpu, radeon
      do_pcie_gen3_transition()    # infiniband/hfi1

The interface for the generic case should be simple and made to be the
obvious choice for drivers, i.e., requires only the pci_dev and no
magic flags.  The link reset case is different enough that it might
deserve its own special interface.

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

Can you switch to using https://lore.kernel.org/linux-pci/<Message-ID>
URLs so we don't depend on 3rd-party archives like spinics?  See
https://www.kernel.org/lore.html .  I usually silently convert to the
lore URLs, but I guess doing it silently only makes more work for
myself.

Bjorn

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

[Sinan Kaya]

On 10/19/2018 10:09 PM, Bjorn Helgaas wrote:
> This series (and v5, it looks like) lost the cover letter, which had a
> nice diffstat overview :)

Yeah, I was too lazy. I was rushing to get it out of my laptop before I go
back to my daily work.

> 
> On Fri, Oct 19, 2018 at 02:11:21AM +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.
> 
> Somewhere this needs a description of a bug that's being fixed or some
> other justification, e.g., code simplification.  The above is a little
> too abstract for me to grasp it.
> 

This series was in response to a request from Alex to have external users
some level of control on what PCI core can do internally.

On the other hand, I have been posting patches to remove direct access from
external users to PCI core internals and hide all reset semantics like
save/restore from the users. I like giving less power to the users :)

>> 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.
> 
> There are fewer than a dozen callers of all these functions and the
> complication of these interfaces doesn't seem commensurate with the
> problem.  With six different interfaces and five independent bit
> flags, the possibilities are way more than necessary.
> 

True, I posted an RFC to reduce 5 function reset API flavors to 1 and
rely on the flags.

In the end, there would be two reset APIs only.

> It seems like there are only three main cases (possibly extended by
> locked/unlocked versions):
> 
>    - a generic reset, used by ipc, genwqe, qlcnic, sfc, liquidio,
>      mwifiex, xen
> 
>    - something special for VFIO that is affected by the set of devices
>      owned by the user
> 
>    - a link reset for places like these where a device needs help to
>      train the link correctly:
> 
>        cik_pcie_gen3_enable()       # amdgpu, radeon
>        si_pcie_gen3_enable()        # amdgpu, radeon
>        do_pcie_gen3_transition()    # infiniband/hfi1
> 
> The interface for the generic case should be simple and made to be the
> obvious choice for drivers, i.e., requires only the pci_dev and no
> magic flags.  

Even these generic reset examples are not really generic today. Some of them
call reset from probe path and use the locked reset API 
(pci_reset_function_locked()).

Others call the reset but prefer to not save/restore context.
(__pci_reset_function_locked()). Some drivers even implemented their own
context save/restore code themselves.

There are others that do obtain the lock as well as save/restore context.
(pci_reset_function())

I really don't like having these many reset API flavors and I thought
we could do something about that.

We can also drop the series if we think that current API are good enough
and nuances are well understood.

Alex wanted to have some more control into the reset APIs. Thus, this series
+ the API reduction in RFC from me (again Alex didn't like this much).

> The link reset case is different enough that it might
> deserve its own special interface.
> 
>> Link: https://www.spinics.net/lists/linux-pci/msg75828.html
> 
> Can you switch to using https://lore.kernel.org/linux-pci/<Message-ID>
> URLs so we don't depend on 3rd-party archives like spinics?  See
> https://www.kernel.org/lore.html .  I usually silently convert to the
> lore URLs, but I guess doing it silently only makes more work for
> myself.
> 
> Bjorn
> 

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

[Bjorn Helgaas]

On Fri, Oct 19, 2018 at 10:58:00PM -0400, Sinan Kaya wrote:
> On 10/19/2018 10:09 PM, Bjorn Helgaas wrote:
> > On Fri, Oct 19, 2018 at 02:11:21AM +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.
> > 
> > Somewhere this needs a description of a bug that's being fixed or some
> > other justification, e.g., code simplification.  The above is a little
> > too abstract for me to grasp it.
> 
> This series was in response to a request from Alex to have external users
> some level of control on what PCI core can do internally.
> 
> On the other hand, I have been posting patches to remove direct access from
> external users to PCI core internals and hide all reset semantics like
> save/restore from the users. I like giving less power to the users :)

That's a good goal.  Along the way we might need to tweak the users to
align their usage to some common cases.

> > > 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.
> > 
> > There are fewer than a dozen callers of all these functions and the
> > complication of these interfaces doesn't seem commensurate with the
> > problem.  With six different interfaces and five independent bit
> > flags, the possibilities are way more than necessary.
> 
> True, I posted an RFC to reduce 5 function reset API flavors to 1 and
> rely on the flags.

I don't like the flags.  Even one API with five bit flags has 32
possible combinations, most of which are meaningless.

> Even these generic reset examples are not really generic today. Some of them
> call reset from probe path and use the locked reset API
> (pci_reset_function_locked()).
> 
> Others call the reset but prefer to not save/restore context.
> (__pci_reset_function_locked()). Some drivers even implemented their own
> context save/restore code themselves.
> 
> There are others that do obtain the lock as well as save/restore context.
> (pci_reset_function())
> 
> I really don't like having these many reset API flavors and I thought
> we could do something about that.

I agree we have too many flavors.  I'm not convinced all the variation
in save/restore is necessary; I suspect most of that is accidental and
drivers could probably all live with a single solution.

> We can also drop the series if we think that current API are good enough
> and nuances are well understood.

I don't think the current API is good enough :)  It's just a small
matter of sorting out a better one.

Bjorn

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

[Sinan Kaya]

On 10/20/2018 11:03 AM, Bjorn Helgaas wrote:
>> We can also drop the series if we think that current API are good enough
>> and nuances are well understood.
> I don't think the current API is good enough:)   It's just a small
> matter of sorting out a better one.

I guess the next logical question is if you have any suggestion.

merge __pci_reset_function_locked() and pci_reset_function_locked().
Then save the context all the time?

What about locked vs. unlocked APIs? If there is a way to know if lock
is being held by the currently running task, we could skip the locking business
and unify these two APIs.

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

[Alex Williamson]

On Sat, 20 Oct 2018 10:03:53 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Fri, Oct 19, 2018 at 10:58:00PM -0400, Sinan Kaya wrote:
> > On 10/19/2018 10:09 PM, Bjorn Helgaas wrote:  
> > > On Fri, Oct 19, 2018 at 02:11:21AM +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.  
> > > 
> > > Somewhere this needs a description of a bug that's being fixed or some
> > > other justification, e.g., code simplification.  The above is a little
> > > too abstract for me to grasp it.  
> > 
> > This series was in response to a request from Alex to have external users
> > some level of control on what PCI core can do internally.
> > 
> > On the other hand, I have been posting patches to remove direct access from
> > external users to PCI core internals and hide all reset semantics like
> > save/restore from the users. I like giving less power to the users :)  
> 
> That's a good goal.  Along the way we might need to tweak the users to
> align their usage to some common cases.
> 
> > > > 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.  
> > > 
> > > There are fewer than a dozen callers of all these functions and the
> > > complication of these interfaces doesn't seem commensurate with the
> > > problem.  With six different interfaces and five independent bit
> > > flags, the possibilities are way more than necessary.  
> > 
> > True, I posted an RFC to reduce 5 function reset API flavors to 1 and
> > rely on the flags.  
> 
> I don't like the flags.  Even one API with five bit flags has 32
> possible combinations, most of which are meaningless.
> 
> > Even these generic reset examples are not really generic today. Some of them
> > call reset from probe path and use the locked reset API
> > (pci_reset_function_locked()).
> > 
> > Others call the reset but prefer to not save/restore context.
> > (__pci_reset_function_locked()). Some drivers even implemented their own
> > context save/restore code themselves.
> > 
> > There are others that do obtain the lock as well as save/restore context.
> > (pci_reset_function())
> > 
> > I really don't like having these many reset API flavors and I thought
> > we could do something about that.  
> 
> I agree we have too many flavors.  I'm not convinced all the variation
> in save/restore is necessary; I suspect most of that is accidental and
> drivers could probably all live with a single solution.
> 
> > We can also drop the series if we think that current API are good enough
> > and nuances are well understood.  
> 
> I don't think the current API is good enough :)  It's just a small
> matter of sorting out a better one.

The thing that I was really hoping to fix was what happened in
811c5cb37df4 where we used to have separate bus vs slot reset
interfaces that vfio-pci made use of, but these were abstracted away
such that even though vfio-pci is specifically trying to achieve one or
the other, not just whichever the pci-core chooses to use, we no longer
have an interface to do that.  It doesn't make sense that we have
pci_probe_reset_slot() and pci_probe_reset_bus() exported for drivers
yet only pci_reset_bus() to execute the reset, which actually does a
slot reset, not a bus reset, if it's available (and hopefully it picks
the one that aligns with what the caller was intending).

I agree that we should have the "I don't really know what I'm doing,
please make the correct choice obvious" interface, but at the same time
I don't see the value in penalizing drivers that do know what they're
doing and want to make a specific choice.  In that sense a desire to
give less power to the user (ie. GPL kernel drivers) seems misplaced, I
think we need to aim for a balance where the interface is hard to
misuse, but at the same time retains useful functionality.  It can't
just be one or the other.

I was also in a discussion today that seems like it could leverage
aspects of this series where there's a desire to allow system admin
level influence over the type of reset used for a device.  We can do
this in the kernel via device specific resets, but it's not always the
case that we can develop a universal recipe for blacklisting a reset
mechanism for a given device.  Consider for instance a device where FLR
works well only with some firmware revisions, but different system
vendors might report similar firmware revisions, some afflicted, others
not.

The bar is relatively high to not introduce device regressions and
incorrectly requiring a device to use SBR rather than FLR can have
functionality repercussions.  I know Bjorn has a desire to have things
work well by default and command line options and sysfs tweaking of
devices are obstacles to that, but there are cases where I don't see
that we have a clear path to impose a device specific quirk without
fear of regressions, and some mechanism of letting userspace policy
influence the operation seems a viable path.  IMO it would also be a
useful debugging tool if I could select a specific method of reset for a
device from userspace and perhaps the infrastructure for this might
make it easier to test and implement device specific quirks that simply
need to pick a reset mechanism different from the one the kernel
chooses by default.

The interface in this proposal maybe needs some work, but at some level
it seems we need to consider which resets the device/bus/slot is capable
of, resets blacklisted/selected/prioritized by the user/driver, and
allow the caller to identify the type of reset they want to achieve.
Can we agree that's a desirable goal?  Thanks,

Alex

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

[Bjorn Helgaas]

On Thu, Nov 08, 2018 at 01:31:47PM -0700, Alex Williamson wrote:
> On Sat, 20 Oct 2018 10:03:53 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> > I don't think the current API is good enough :)  It's just a small
> > matter of sorting out a better one.
> 
> The thing that I was really hoping to fix was what happened in
> 811c5cb37df4 where we used to have separate bus vs slot reset
> interfaces that vfio-pci made use of, but these were abstracted away
> such that even though vfio-pci is specifically trying to achieve one or
> the other, not just whichever the pci-core chooses to use, we no longer
> have an interface to do that.  It doesn't make sense that we have
> pci_probe_reset_slot() and pci_probe_reset_bus() exported for drivers
> yet only pci_reset_bus() to execute the reset, which actually does a
> slot reset, not a bus reset, if it's available (and hopefully it picks
> the one that aligns with what the caller was intending).
> 
> I agree that we should have the "I don't really know what I'm doing,
> please make the correct choice obvious" interface, but at the same time
> I don't see the value in penalizing drivers that do know what they're
> doing and want to make a specific choice.  In that sense a desire to
> give less power to the user (ie. GPL kernel drivers) seems misplaced, I
> think we need to aim for a balance where the interface is hard to
> misuse, but at the same time retains useful functionality.  It can't
> just be one or the other.
> 
> I was also in a discussion today that seems like it could leverage
> aspects of this series where there's a desire to allow system admin
> level influence over the type of reset used for a device.  We can do
> this in the kernel via device specific resets, but it's not always the
> case that we can develop a universal recipe for blacklisting a reset
> mechanism for a given device.  Consider for instance a device where FLR
> works well only with some firmware revisions, but different system
> vendors might report similar firmware revisions, some afflicted, others
> not.
> 
> The bar is relatively high to not introduce device regressions and
> incorrectly requiring a device to use SBR rather than FLR can have
> functionality repercussions.  I know Bjorn has a desire to have things
> work well by default and command line options and sysfs tweaking of
> devices are obstacles to that, but there are cases where I don't see
> that we have a clear path to impose a device specific quirk without
> fear of regressions, and some mechanism of letting userspace policy
> influence the operation seems a viable path.  IMO it would also be a
> useful debugging tool if I could select a specific method of reset for a
> device from userspace and perhaps the infrastructure for this might
> make it easier to test and implement device specific quirks that simply
> need to pick a reset mechanism different from the one the kernel
> chooses by default.
> 
> The interface in this proposal maybe needs some work, but at some level
> it seems we need to consider which resets the device/bus/slot is capable
> of, resets blacklisted/selected/prioritized by the user/driver, and
> allow the caller to identify the type of reset they want to achieve.
> Can we agree that's a desirable goal?  Thanks,

I can't sign up completely to that whole list because it basically
mandates much of the complexity I want to avoid.

I do certainly agree we need to figure out which resets are supported
by a device.  Along that line, I want to get rid of all the "probe"
arguments.  In almost all cases we can figure out what's supported at
enumeration-time, and I think we should cache that information in a
series of bits in the pci_dev.  We already do much of that in
pci_probe_reset_function(), but we only save overall "can this device
be reset by any method", not the results for each individual method.

Of course, the parent bus reset is slightly tricky because we can't
decide for each device individually; it depends on whether there are
peer devices.  But I think that's still doable.

If we cache each bit, then it's a small step to have quirks or some
sort of sysfs hook to disable specific resets for firmware issues or
sysadmin control.

I don't want to penalize drivers that need to do something specific.
I just don't believe there are very many of those cases.  I think we
should list them explicitly, combine the ones that are needlessly
different, and build something that's sufficient for those cases, but
not overly generalized.

Bjorn