Skip to contentSkip to conversation

PCI: Expose reset_type to users of __pci_reset_function_locked()

PATCH Superseded Sinan Kaya 3 participants 19 messages Oct 11, 2018 – Oct 12, 2018

Subsystems (15)
Network drivers PCI InfiniBand HID +11 more subsystems

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Sinan Kaya

author
#0 lore raw

PATCH v5 1/11 PCI: Expose reset_type to users of __pci_reset_function_locked()

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 <[email protected]>
Signed-off-by Sinan Kaya <[email protected]>
---
 .../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                           | 56 +++++++++++++++++-
 4 files changed, 98 insertions(+), 25 deletions(-)
drivers/net/ethernet/cavium/liquidio/lio_main.c +1 -1
index 6fb13fa73b27..0ff76722734d 100644
@@ -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",
drivers/pci/pci.c +39 -20

99 lines not on this page · raw message

drivers/xen/xen-pciback/pci_stub.c +3 -3
index 59661db144e5..6dfb805bcb19 100644
@@ -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
include/linux/pci.h +55 -1

68 lines not on this page · raw message

Signature · 1 line
2.19.0
author
#1 lore raw

PATCH v5 2/11 PCI: Expose reset_type to users of pci_reset_function()

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 <[email protected]>
Acked-by Srinivas Pandruvada <[email protected]> (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(-)
drivers/hid/intel-ish-hid/ipc/ipc.c +1 -1
index bfbca7ec54ce..18312969f1b3 100644
@@ -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;
drivers/misc/genwqe/card_base.c +1 -1
index c7cd3675bcd1..cc78ef28ee38 100644
@@ -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);
drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c +1 -1
index d344e9d43832..bb737725f175 100644
@@ -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;
 	}
drivers/net/ethernet/sfc/mcdi.c +1 -1
index dfad93fca0a6..7f95e17b8a48 100644
@@ -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 */
drivers/pci/pci-sysfs.c +1 -1
index 9ecfe13157c0..9569664ec4b2 100644
@@ -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;
drivers/pci/pci.c +3 -2
index e292ea589d3e..66f102b7ed4e 100644
@@ -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);
include/linux/pci.h +1 -1
index 0967e6bad421..b72689f869db 100644
@@ -1166,7 +1166,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);
Signature · 1 line
2.19.0
author
#2 lore raw

PATCH v5 3/11 PCI: Expose reset_type to users of pci_reset_function_locked()

In reply to 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 <[email protected]>
---
 drivers/pci/pci.c   | 5 +++--
 include/linux/pci.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)
drivers/pci/pci.c +3 -2
index 66f102b7ed4e..9a649d1adb13 100644
@@ -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);
include/linux/pci.h +1 -1
index b72689f869db..e1d1328172fa 100644
@@ -1167,7 +1167,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);
Signature · 1 line
2.19.0
author
#3 lore raw

PATCH v5 4/11 PCI: Expose reset type to users of pci_try_reset_function()

In reply to 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 <[email protected]>
---
 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(-)
drivers/pci/pci.c +3 -2
index 9a649d1adb13..7739f28988ae 100644
@@ -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);
drivers/vfio/pci/vfio_pci.c +4 -3
index cddb453a1ba5..fe7ada997c51 100644
@@ -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;
drivers/vfio/pci/vfio_pci_config.c +2 -2
index 115a36f6f403..0d66bac66211 100644
@@ -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;
include/linux/pci.h +1 -1
index e1d1328172fa..1c120cf00dd8 100644
@@ -1168,7 +1168,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);
Signature · 1 line
2.19.0
author
#4 lore raw

PATCH v5 5/11 PCI: Expose reset type to users of pci_probe_reset_function()

In reply to 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 <[email protected]>
---
 drivers/pci/pci.c   | 52 ++++++++++++++++++++++++++++++---------------
 drivers/pci/pci.h   |  2 +-
 drivers/pci/probe.c |  2 +-
 3 files changed, 37 insertions(+), 19 deletions(-)
drivers/pci/pci.c +35 -17
index 7739f28988ae..a1a7dd6988be 100644
@@ -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;
 }
 
 /**
drivers/pci/pci.h +1 -1
index 6e0d1528d471..0444bfa51b52 100644
@@ -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);
 
 /**
drivers/pci/probe.c +1 -1
index 201f9e5ff55c..6c8f6af2d4aa 100644
@@ -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;
 }
Signature · 1 line
2.19.0
author
#5 lore raw

PATCH v5 6/11 PCI: Expose reset type to users of pci_reset_bus()

In reply to 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 <[email protected]>
---
 drivers/pci/pci.c           | 17 ++++++++++++++---
 drivers/vfio/pci/vfio_pci.c |  6 ++++--
 include/linux/pci.h         |  2 +-
 3 files changed, 19 insertions(+), 6 deletions(-)
drivers/pci/pci.c +14 -3
index a1a7dd6988be..a3a1c2a5e0cf 100644
@@ -5236,13 +5236,24 @@ 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) == 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) == PCI_RESET_BUS)
+		return __pci_reset_bus(pdev->bus);
+
+	if ((reset_type & PCI_RESET_SLOT) == PCI_RESET_SLOT)
+		return __pci_reset_slot(pdev->slot);
+
+	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(pci_reset_bus);
drivers/vfio/pci/vfio_pci.c +4 -2
index fe7ada997c51..0e80c72b1eaa 100644
@@ -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++) {
include/linux/pci.h +1 -1
index 1c120cf00dd8..cf1e847ea02e 100644
@@ -1171,7 +1171,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);
Signature · 1 line
2.19.0
#6 lore raw

Re: [PATCH v5 06/11] PCI: Expose reset type to users of pci_reset_bus()

Quoted 30 lines · On Thu, 11 Oct 2018 04:49:58 +0000 Sinan Kaya <[email protected]> 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 <[email protected]>
---
 drivers/pci/pci.c           | 17 ++++++++++++++---
 drivers/vfio/pci/vfio_pci.c |  6 ++++--
 include/linux/pci.h         |  2 +-
 3 files changed, 19 insertions(+), 6 deletions(-)
drivers/pci/pci.c +4 -3
index a1a7dd6988be..a3a1c2a5e0cf 100644
@@ -5236,13 +5236,24 @@ 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) == PCI_RESET_LINK)
+		return (!pci_probe_reset_slot(pdev->slot)) ?

drivers/pci/pci.c static int __pci_reset_bus(struct pci_bus *bus) · line 5256

Quoted 10 lines
drivers/pci/pci.c +10 -0
+				__pci_reset_slot(pdev->slot) :
+				__pci_reset_bus(pdev->bus);
+
+	if ((reset_type & PCI_RESET_BUS) == PCI_RESET_BUS)
+		return __pci_reset_bus(pdev->bus);
+
+	if ((reset_type & PCI_RESET_SLOT) == PCI_RESET_SLOT)
+		return __pci_reset_slot(pdev->slot);
+
+	return -EINVAL;

Having three cases here still seems strange. The below still has some duplicate probing but doesn't it do the same thing with less redundancy?

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->slot))

	return __pci_reset_bus(pdev->bus);

return -EINVAL;

Thanks,
Alex

Quoted 40 lines
drivers/pci/pci.c +0 -0
 }
 EXPORT_SYMBOL_GPL(pci_reset_bus);
drivers/vfio/pci/vfio_pci.c +4 -2
index fe7ada997c51..0e80c72b1eaa 100644
@@ -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++) {
include/linux/pci.h +1 -1
index 1c120cf00dd8..cf1e847ea02e 100644
@@ -1171,7 +1171,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);
author
#7 lore raw

Re: [PATCH v5 06/11] PCI: Expose reset type to users of pci_reset_bus()

Quoted 8 lines · Alex Williamson

Having three cases here still seems strange. The below still has some duplicate probing but doesn't it do the same thing with less redundancy?

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->slot))

	return __pci_reset_bus(pdev->bus);

I agree this looks cleaner. I'll fold it in on the next rev.

author
#8 lore raw

PATCH v5 7/11 IB/hfi1,PCI: switch to __pci_function_locked() for reset request

In reply to Sinan Kaya

Start using the new reset API rather than the workaround.

Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/infiniband/hw/hfi1/pcie.c | 2 +-
 include/linux/pci.h               | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)
drivers/infiniband/hw/hfi1/pcie.c +1 -1
index 6c967dde58e7..38f96192e5f0 100644
@@ -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);
 }
 
 /*
include/linux/pci.h +0 -3
index cf1e847ea02e..d4acdc400ef2 100644
@@ -1289,9 +1289,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; \
Signature · 1 line
2.19.0
author
#9 lore raw

PATCH v5 8/11 PCI: Unify pci_reset_function_locked() and __pci_reset_function_locked()

In reply to Sinan Kaya

The difference between pci_reset_function_locked() and __pci_reset_function_locked() is the saving and restoring of the registers. Unify these API by adding saverestore argument that caller passes.

Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/infiniband/hw/hfi1/pcie.c               |  2 +-
 drivers/net/ethernet/cavium/liquidio/lio_main.c |  2 +-
 drivers/pci/pci.c                               | 10 +++++++---
 drivers/pci/pci.h                               |  1 +
 drivers/xen/xen-pciback/pci_stub.c              |  6 +++---
 include/linux/pci.h                             |  4 ++--
 6 files changed, 15 insertions(+), 10 deletions(-)
drivers/infiniband/hw/hfi1/pcie.c +1 -1
index 38f96192e5f0..73eced9c5523 100644
@@ -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_reset_function_locked(dev, PCI_RESET_LINK);
+	return pci_reset_function_locked(dev, PCI_RESET_LINK, false);
 }
 
 /*
drivers/net/ethernet/cavium/liquidio/lio_main.c +1 -1
index 0ff76722734d..7e001382ad93 100644
@@ -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, PCI_RESET_ANY);
+	rc = pci_reset_function_locked(oct->pci_dev, PCI_RESET_ANY, false);
 
 	if (rc != 0)
 		dev_err(&oct->pci_dev->dev, "Error %d resetting PCI function %d\n",
drivers/pci/pci.c +7 -3
index a3a1c2a5e0cf..97328dc8e476 100644
@@ -4835,6 +4835,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
+ * @saverestore: Flag indicating whether device context should be saved.
  *
  * Some devices allow an individual function to be reset without affecting
  * other functions in the same device.  The PCI device must be responsive

@@ -4849,18 +4850,21 @@ 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, u32 reset_type)
+int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type,
+			      bool saverestore)
 {
 	int rc;
 
 	if (!dev->reset_fn)
 		return -ENOTTY;
 
-	pci_dev_save_and_disable(dev);
+	if (saverestore)
+		pci_dev_save_and_disable(dev);
 
 	rc = __pci_reset_function_locked(dev, reset_type);
 
-	pci_dev_restore(dev);
+	if (saverestore)
+		pci_dev_restore(dev);
 
 	return rc;
 }
drivers/pci/pci.h +1 -0
index 0444bfa51b52..fbfb44fb32b7 100644
@@ -35,6 +35,7 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
 
 int pci_probe_reset_function(struct pci_dev *dev, u32 reset_type);
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
+int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
drivers/xen/xen-pciback/pci_stub.c +3 -3
index 6dfb805bcb19..b68e811ab27f 100644
@@ -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_ANY);
+	pci_reset_function_locked(dev, PCI_RESET_ANY, false);
 	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_ANY);
+	pci_reset_function_locked(dev, PCI_RESET_ANY, false);
 
 	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_ANY);
+		pci_reset_function_locked(dev, PCI_RESET_ANY, false);
 		pci_restore_state(dev);
 	}
 	/* Now disable the device (this also ensures some private device
include/linux/pci.h +2 -2
index d4acdc400ef2..84d08a9a01e3 100644
@@ -1165,9 +1165,9 @@ 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, 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_reset_function_locked(struct pci_dev *dev, u32 reset_type,
+			      bool saverestore);
 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);
Signature · 1 line
2.19.0
#10 lore raw

Re: [PATCH v5 08/11] PCI: Unify pci_reset_function_locked() and __pci_reset_function_locked()

Quoted 5 lines · On Thu, 11 Oct 2018 04:50:00 +0000 Sinan Kaya <[email protected]> wrote:

The difference between pci_reset_function_locked() and __pci_reset_function_locked() is the saving and restoring of the registers. Unify these API by adding saverestore argument that caller passes.

Signed-off-by Sinan Kaya <[email protected]>
Quoted 8 lines
---
 drivers/infiniband/hw/hfi1/pcie.c               |  2 +-
 drivers/net/ethernet/cavium/liquidio/lio_main.c |  2 +-
 drivers/pci/pci.c                               | 10 +++++++---
 drivers/pci/pci.h                               |  1 +
 drivers/xen/xen-pciback/pci_stub.c              |  6 +++---
 include/linux/pci.h                             |  4 ++--
 6 files changed, 15 insertions(+), 10 deletions(-)

TBH, I'm not a fan of this patch or the remainder in this series. Having a function prefixed with underscored or specifically indicate locked tells callers that these are special cases and should be understood before using. Adding bool parameters to the common functions ensures that every caller now needs to understand those special cases and potentially get them wrong. Also, a string of random bool options to a function means that any time we want to use it we need to go reexamine the function definition. It's not intuitive to use or review. Thanks,

Alex

author
#11 lore raw

Re: [PATCH v5 08/11] PCI: Unify pci_reset_function_locked() and __pci_reset_function_locked()

Quoted 13 lines · Alex Williamson
Quoted 5 lines

The difference between pci_reset_function_locked() and __pci_reset_function_locked() is the saving and restoring of the registers. Unify these API by adding saverestore argument that caller passes.

Signed-off-by Sinan Kaya<[email protected]>
Quoted 8 lines
---
  drivers/infiniband/hw/hfi1/pcie.c               |  2 +-
  drivers/net/ethernet/cavium/liquidio/lio_main.c |  2 +-
  drivers/pci/pci.c                               | 10 +++++++---
  drivers/pci/pci.h                               |  1 +
  drivers/xen/xen-pciback/pci_stub.c              |  6 +++---
  include/linux/pci.h                             |  4 ++--
  6 files changed, 15 insertions(+), 10 deletions(-)
Quoted 9 lines

TBH, I'm not a fan of this patch or the remainder in this series. Having a function prefixed with underscored or specifically indicate locked tells callers that these are special cases and should be understood before using. Adding bool parameters to the common functions ensures that every caller now needs to understand those special cases and potentially get them wrong. Also, a string of random bool options to a function means that any time we want to use it we need to go reexamine the function definition. It's not intuitive to use or review. Thanks,

Thanks for the feedback. I wanted to get it out to gather feedback.

I think it is quite straightforward for people familiar with the existing API to know what they are dealing with. I didn't know the difference between

__pci_reset_function_locked() and pci_reset_function_locked()

for a long time. This was an attempt to minimize reset function flavors and ask explicitly what user wants rather than having them use the wrong function and suffer the consequence with random failures.

#12 lore raw

Re: [PATCH v5 08/11] PCI: Unify pci_reset_function_locked() and __pci_reset_function_locked()

In reply to Sinan Kaya
Quoted 3 lines · Sinan Kaya

The difference between pci_reset_function_locked() and __pci_reset_function_locked() is the saving and restoring of the registers. Unify these API by adding saverestore argument that caller passes.

Adding random boolean arguments doesn't really help the API. Either make this another flag for reset_type or if there is a clear benefit add an additional flags parameter with well described flags.

author
#13 lore raw

Re: [PATCH v5 08/11] PCI: Unify pci_reset_function_locked() and __pci_reset_function_locked()

Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Sinan Kaya

The difference between pci_reset_function_locked() and __pci_reset_function_locked() is the saving and restoring of the registers. Unify these API by adding saverestore argument that caller passes.

Adding random boolean arguments doesn't really help the API. Either make this another flag for reset_type or if there is a clear benefit add an additional flags parameter with well described flags.

Good idea. This is the kind of feedback I was looking for. Here is what I collected so far.

Alex doesn't like the API changes as it should be obvious that there are differences between pci_reset_function_locked() and __pci_reset_function_locked(). Sinan thinks that the difference is not that obvious as some drivers implemented their own save/restore mechanism using __pci_reset_function_locked(). Sinan would rather reduce the reset API flavors and give a unified interface where user specifies what they are interested in.

Christoph asked for additional flags to be rolled into the reset_type argument. Sinan thinks that this is a good idea.

Anybody else?

author
#14 lore raw

PATCH v5 9/11 PCI: Add options to pci_reset_function

In reply to Sinan Kaya

Getting ready to deprecate pci_reset_function_locked(). Add saverestore and locked parameters to pci_reset_function() function and add saverestore = true and locked = false to all existing callers.

Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c           |  2 +-
 drivers/misc/genwqe/card_base.c               |  2 +-
 .../net/ethernet/qlogic/qlcnic/qlcnic_ctx.c   |  2 +-
 drivers/net/ethernet/sfc/mcdi.c               |  3 ++-
 drivers/pci/pci-sysfs.c                       |  2 +-
 drivers/pci/pci.c                             | 20 ++++++++++++++-----
 include/linux/pci.h                           |  3 ++-
 7 files changed, 23 insertions(+), 11 deletions(-)
drivers/hid/intel-ish-hid/ipc/ipc.c +1 -1
index 18312969f1b3..5fa7cdd136fe 100644
@@ -754,7 +754,7 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 	if (!pdev)
 		return	-ENODEV;
 
-	rv = pci_reset_function(pdev, PCI_RESET_ANY);
+	rv = pci_reset_function(pdev, PCI_RESET_ANY, true, false);
 	if (!rv)
 		dev->dev_state = ISHTP_DEV_RESETTING;
drivers/misc/genwqe/card_base.c +1 -1
index cc78ef28ee38..85eb3ca55ccb 100644
@@ -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, PCI_RESET_ANY);
+	rc = pci_reset_function(pci_dev, PCI_RESET_ANY, true, false);
 	if (rc) {
 		dev_err(&pci_dev->dev,
 			"[%s] err: failed reset func (rc %d)\n", __func__, rc);
drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c +1 -1
index bb737725f175..4fb3ea301af8 100644
@@ -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_ANY);
+		pci_reset_function(dev->pdev, PCI_RESET_ANY, true, false);
 		dev->flags &= ~QLCNIC_NEED_FLR;
 	}
drivers/net/ethernet/sfc/mcdi.c +2 -1
index 7f95e17b8a48..2406cd38f258 100644
@@ -1862,7 +1862,8 @@ 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, PCI_RESET_ANY);
+		rc = pci_reset_function(efx->pci_dev, PCI_RESET_ANY, true,
+					false);
 		if (rc)
 			return rc;
 		/* Re-enable polled MCDI completion */
drivers/pci/pci-sysfs.c +1 -1
index 9569664ec4b2..939de1d323ad 100644
@@ -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, PCI_RESET_ANY);
+	result = pci_reset_function(pdev, PCI_RESET_ANY, true, false);
 	pm_runtime_put(dev);
 	if (result < 0)
 		return result;
drivers/pci/pci.c +15 -5
index 97328dc8e476..b34909376221 100644
@@ -4799,6 +4799,9 @@ int pci_probe_reset_function(struct pci_dev *dev, u32 reset_type)
  * pci_reset_function - quiesce and reset a PCI device function
  * @dev: PCI device to reset
  * @reset_type: reset type to apply
+ * @saverestore: flag for save/restore the device state before and after reset
+ * @locked: flag for obtaining device lock. true if caller is already holding
+ *          the lock.
  *
  * Some devices allow an individual function to be reset without affecting
  * other functions in the same device.  The PCI device must be responsive

@@ -4812,20 +4815,27 @@ int pci_probe_reset_function(struct pci_dev *dev, u32 reset_type)
  * 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, u32 reset_type)
+int pci_reset_function(struct pci_dev *dev, u32 reset_type, bool saverestore,
+		       bool locked)
 {
 	int rc;
 
 	if (!dev->reset_fn)
 		return -ENOTTY;
 
-	pci_dev_lock(dev);
-	pci_dev_save_and_disable(dev);
+	if (!locked)
+		pci_dev_lock(dev);
+
+	if (saverestore)
+		pci_dev_save_and_disable(dev);
 
 	rc = __pci_reset_function_locked(dev, reset_type);
 
-	pci_dev_restore(dev);
-	pci_dev_unlock(dev);
+	if (saverestore)
+		pci_dev_restore(dev);
+
+	if (!locked)
+		pci_dev_unlock(dev);
 
 	return rc;
 }
include/linux/pci.h +2 -1
index 84d08a9a01e3..82b326eb624b 100644
@@ -1165,7 +1165,8 @@ 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(struct pci_dev *dev, u32 reset_type);
+int pci_reset_function(struct pci_dev *dev, u32 reset_type, bool saverestore,
+		       bool locked);
 int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type,
 			      bool saverestore);
 int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
Signature · 1 line
2.19.0
NAK
#15 lore raw

Re: [PATCH v5 09/11] PCI: Add options to pci_reset_function

Quoted 4 lines · Sinan Kaya

Getting ready to deprecate pci_reset_function_locked(). Add saverestore and locked parameters to pci_reset_function() function and add saverestore = true and locked = false to all existing callers.

NAK - no conditional locking based on a parameter, please.

author
#16 lore raw

PATCH v5 10/11 PCI: Hide pci_reset_function_locked()

In reply to Sinan Kaya

It is time to hide pci_reset_function_locked() since pci_reset_function() provides the same functionality. One less API to expose to the users.

Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/infiniband/hw/hfi1/pcie.c               | 2 +-
 drivers/net/ethernet/cavium/liquidio/lio_main.c | 2 +-
 drivers/pci/pci.h                               | 2 ++
 drivers/xen/xen-pciback/pci_stub.c              | 6 +++---
 include/linux/pci.h                             | 2 --
 5 files changed, 7 insertions(+), 7 deletions(-)
drivers/infiniband/hw/hfi1/pcie.c +1 -1
index 73eced9c5523..8fbcde36353d 100644
@@ -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_reset_function_locked(dev, PCI_RESET_LINK, false);
+	return pci_reset_function(dev, PCI_RESET_LINK, false, true);
 }
 
 /*
drivers/net/ethernet/cavium/liquidio/lio_main.c +1 -1
index 7e001382ad93..e1ff9818ed6b 100644
@@ -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, PCI_RESET_ANY, false);
+	rc = pci_reset_function(oct->pci_dev, PCI_RESET_ANY, false, true);
 
 	if (rc != 0)
 		dev_err(&oct->pci_dev->dev, "Error %d resetting PCI function %d\n",
drivers/pci/pci.h +2 -0
index fbfb44fb32b7..737fdc36fe36 100644
@@ -36,6 +36,8 @@ int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
 int pci_probe_reset_function(struct pci_dev *dev, u32 reset_type);
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
+int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type,
+			      bool saverestore);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
drivers/xen/xen-pciback/pci_stub.c +3 -3
index b68e811ab27f..6806a58c30d5 100644
@@ -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_ANY, false);
+	pci_reset_function(dev, PCI_RESET_ANY, false, true);
 	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_ANY, false);
+	pci_reset_function(dev, PCI_RESET_ANY, false, true);
 
 	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_ANY, false);
+		pci_reset_function(dev, PCI_RESET_ANY, false, true);
 		pci_restore_state(dev);
 	}
 	/* Now disable the device (this also ensures some private device
include/linux/pci.h +0 -2
index 82b326eb624b..95efffd09690 100644
@@ -1167,8 +1167,6 @@ bool pcie_has_flr(struct pci_dev *dev);
 int pcie_flr(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev, u32 reset_type, bool saverestore,
 		       bool locked);
-int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type,
-			      bool saverestore);
 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);
Signature · 1 line
2.19.0
author
#17 lore raw

PATCH v5 11/11 PCI: Hide pcie_flr() in favor of pci_reset_function()

In reply to Sinan Kaya

Now that we have a unified API for device reset, let's eliminate one duplication by hiding pcie_flr().

Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/crypto/qat/qat_common/adf_aer.c            | 3 ++-
 drivers/infiniband/hw/hfi1/chip.c                  | 5 +++--
 drivers/net/ethernet/cavium/liquidio/lio_vf_main.c | 3 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      | 6 ++++--
 drivers/pci/pci.h                                  | 1 +
 include/linux/pci.h                                | 1 -
 6 files changed, 12 insertions(+), 7 deletions(-)
drivers/crypto/qat/qat_common/adf_aer.c +2 -1
index 9225d060e18f..9326685dd89f 100644
@@ -109,7 +109,8 @@ EXPORT_SYMBOL_GPL(adf_reset_sbr);
 
 void adf_reset_flr(struct adf_accel_dev *accel_dev)
 {
-	pcie_flr(accel_to_pci_dev(accel_dev));
+	pci_reset_function(accel_to_pci_dev(accel_dev), PCI_RESET_FLR,
+			   false, true);
 }
 EXPORT_SYMBOL_GPL(adf_reset_flr);
drivers/infiniband/hw/hfi1/chip.c +3 -2
index e1668bcc2d13..12bc5ef81e12 100644
@@ -14077,7 +14077,7 @@ static int init_chip(struct hfi1_devdata *dd)
 		dd_dev_info(dd, "Resetting CSRs with FLR\n");
 
 		/* do the FLR, the DC reset will remain */
-		pcie_flr(dd->pcidev);
+		pci_reset_function(dd->pcidev, PCI_RESET_FLR, false, true);
 
 		/* restore command and BARs */
 		ret = restore_pci_variables(dd);

@@ -14089,7 +14089,8 @@ static int init_chip(struct hfi1_devdata *dd)
 
 		if (is_ax(dd)) {
 			dd_dev_info(dd, "Resetting CSRs with FLR\n");
-			pcie_flr(dd->pcidev);
+			pci_reset_function(dd->pcidev, PCI_RESET_FLR, false,
+					   true);
 			ret = restore_pci_variables(dd);
 			if (ret) {
 				dd_dev_err(dd, "%s: Could not restore PCI variables\n",
drivers/net/ethernet/cavium/liquidio/lio_vf_main.c +2 -1
index b77835724dc8..6af086337359 100644
@@ -438,7 +438,8 @@ static void octeon_pci_flr(struct octeon_device *oct)
 	pci_write_config_word(oct->pci_dev, PCI_COMMAND,
 			      PCI_COMMAND_INTX_DISABLE);
 
-	pcie_flr(oct->pci_dev);
+	pci_reset_function(oct->pci_dev, PCI_RESET_FUNCTION,
+			   false, true);
 
 	pci_cfg_access_unlock(oct->pci_dev);
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +4 -2
index f27d73a7bf16..6345c2c11c62 100644
@@ -7539,7 +7539,8 @@ static void ixgbe_check_for_bad_vf(struct ixgbe_adapter *adapter)
 		pci_read_config_word(vfdev, PCI_STATUS, &status_reg);
 		if (status_reg != IXGBE_FAILED_READ_CFG_WORD &&
 		    status_reg & PCI_STATUS_REC_MASTER_ABORT)
-			pcie_flr(vfdev);
+			pci_reset_function(vfdev, PCI_RESET_FLR, false
+					   true);
 	}
 }
 

@@ -11046,7 +11047,8 @@ static pci_ers_result_t ixgbe_io_error_detected(struct pci_dev *pdev,
 		 * VFLR.  Just clean up the AER in that case.
 		 */
 		if (vfdev) {
-			pcie_flr(vfdev);
+			pci_reset_function(vfdev, PCI_RESET_FUNCTION, false,
+					   true);
 			/* Free device reference count */
 			pci_dev_put(vfdev);
 		}
drivers/pci/pci.h +1 -0
index 737fdc36fe36..c1dc92b1cc1e 100644
@@ -38,6 +38,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev, u32 reset_type);
 int pci_reset_function_locked(struct pci_dev *dev, u32 reset_type,
 			      bool saverestore);
+int pcie_flr(struct pci_dev *dev);
 
 /**
  * struct pci_platform_pm_ops - Firmware PM callbacks
include/linux/pci.h +0 -1
index 95efffd09690..acaf115841cc 100644
@@ -1164,7 +1164,6 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
 			     enum pcie_link_width *width);
 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(struct pci_dev *dev, u32 reset_type, bool saverestore,
 		       bool locked);
 int pci_try_reset_function(struct pci_dev *dev, u32 reset_type);
Signature · 1 line
2.19.0
author
#18 lore raw

Re: [PATCH v5 11/11] PCI: Hide pcie_flr() in favor of pci_reset_function()

Quoted 4 lines · Sinan Kaya

Now that we have a unified API for device reset, let's eliminate one duplication by hiding pcie_flr().

Signed-off-by Sinan Kaya<[email protected]>

This patch might have gone too far. I'm willing to drop it.