PCI: Expose reset_type to users of __pci_reset_function_locked()

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 +0 -0

index 927e60660b96..9103ac1b3c31 100644

include/linux/pci.h +0 -0

 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

Quoted 1 line

+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?

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 9103ac1b3c31..cde63e0a85ea 100644
@@ -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);

Signature · 1 line

2.19.0

[+cc mwifiex maintainers, Brian]

Quoted 10 lines · 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 +-

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

Quoted 98 lines

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

index 9103ac1b3c31..cde63e0a85ea 100644
@@ -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

Start using the new reset API rather than the workaround.

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

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

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

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 1 line

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

Quoted 8 lines · Bjorn Helgaas

Quoted 3 lines · 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.

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

Quoted 6 lines

Quoted 5 lines

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.

Quoted 4 lines

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.

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.

Quoted 2 lines

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.

Quoted 10 lines · Sinan Kaya

Quoted 8 lines · Bjorn Helgaas

Quoted 3 lines · 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.

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 2 lines · On Sat, 20 Oct 2018 10:03:53 -0500 Bjorn Helgaas <[email protected]> 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,