pci: add function reset call that can be used inside of probe

From: Alexander Duyck <[email protected]>

This change makes it so that both igb and ixgbe can trigger a full pcie function reset if the reset_devices kernel parameter is defined. The main reason for adding this is that kdump can cause serious issues when the kdump kernel resets the IOMMU while DMA transactions are still occurring.

Signed-off-by Alexander Duyck <[email protected]>

Signed-off-by Jeff Kirsher <[email protected]>

From: Jeff Kirsher <[email protected]>
Date: Fri, 30 Jul 2010 17:59:12 -0700

Quoted 9 lines

From: Alexander Duyck <[email protected]>

This change makes it so that both igb and ixgbe can trigger a full pcie function reset if the reset_devices kernel parameter is defined. The main reason for adding this is that kdump can cause serious issues when the kdump kernel resets the IOMMU while DMA transactions are still occurring.

Signed-off-by Alexander Duyck <[email protected]>

Signed-off-by Jeff Kirsher <[email protected]>

I tend to disagree with the essence of this change.

Which is that we should add workaround after workaround for things that aren't functioning properly in kdump and kexec.

They should have a pass that shuts devices down properly, so that this kind of stuff doesn't need to happen in the kernel we then boot into.

What happens on non-PCIE systems then? Do they just lose when this happens?

Quoted 13 lines · David Miller

From: Jeff Kirsher<[email protected]>
Date: Fri, 30 Jul 2010 17:59:12 -0700

Quoted 9 lines

From: Alexander Duyck<[email protected]>

This change makes it so that both igb and ixgbe can trigger a full pcie function reset if the reset_devices kernel parameter is defined. The main reason for adding this is that kdump can cause serious issues when the kdump kernel resets the IOMMU while DMA transactions are still occurring.

Signed-off-by Alexander Duyck<[email protected]>

Signed-off-by Jeff Kirsher<[email protected]>

Quoted 7 lines

I tend to disagree with the essence of this change.

Which is that we should add workaround after workaround for things that aren't functioning properly in kdump and kexec.

They should have a pass that shuts devices down properly, so that this kind of stuff doesn't need to happen in the kernel we then boot into.

For a normal kexec, arguably true.

But in the kdump case, the original kernel has *crashed* and we really don't have that option -- we need to jump *straight* to the new kernel and have it reset the hardware.

The device driver really *ought* to be able to reset the hardware from whatever state it's in when the new kernel starts up. Anything less is broken, and reminds me of those crappy drivers that only work after a soft-reboot from Windows.

Most drivers *do* quite happily initialise their device and reliably get it into a known state; it's just that this particular hardware goes into a *particularly* stroppy fit when it gets a DMA master abort (which is what happens when the IOMMU stops it from scribbling into memory after the new kernel has taken over).

Quoted 2 lines

What happens on non-PCIE systems then? Do they just lose when this happens?

If they have a device that's this broken, and the driver can't get it into a working state any other way, then yes -- I don't see any way to *avoid* them losing.

From: Alexander Duyck<[email protected]>

This change makes it so that there are several new calls available.

The first is __pci_reset_dev which works similar to pci_reset_dev, however it does not obtain the device lock. This is important as I found several cases such as __pci_reset_function in which the call was obtaining the device lock even though the lock had yet to be initialized. In addition if one wishes to do such a reset during probe it will hang since the device lock is already being held.

The second change that was added was a function named pci_reset_device_function. This function is similar to pci_reset_function however it does not hold the device lock and so it as well can be called during the driver probe routine.

Signed-off-by Alexander Duyck<[email protected]>

Signed-off-by Jeff Kirsher<[email protected]>

---

  drivers/pci/pci.c   |   74 +++++++++++++++++++++++++++++++++++++++++++--------
  include/linux/pci.h |    1 +
  2 files changed, 64 insertions(+), 11 deletions(-)

drivers/pci/pci.c +45 -11

index 60f30e7..1421bc7 100644
@@ -2445,17 +2445,14 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
  	return 0;
  }
 
-static int pci_dev_reset(struct pci_dev *dev, int probe)
+static int __pci_dev_reset(struct pci_dev *dev, int probe)
  {
  	int rc;
 
  	might_sleep();
 
-	if (!probe) {
+	if (!probe)
  		pci_block_user_cfg_access(dev);
-		/* block PM suspend, driver probe, etc. */
-		device_lock(&dev->dev);
-	}
 
  	rc = pci_dev_specific_reset(dev, probe);
  	if (rc != -ENOTTY)

@@ -2474,11 +2471,26 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
  		goto done;
 
  	rc = pci_parent_bus_reset(dev, probe);
+
  done:
-	if (!probe) {
-		device_unlock(&dev->dev);
+	if (!probe)
  		pci_unblock_user_cfg_access(dev);
-	}
+
+	return rc;
+}
+
+static int pci_dev_reset(struct pci_dev *dev, int probe)
+{
+	int rc;
+
+	/* block PM suspend, driver probe, etc. */
+	if (!probe)
+		device_lock(&dev->dev);
+
+	rc = __pci_dev_reset(dev, probe);
+
+	if (!probe)
+		device_unlock(&dev->dev);
 
  	return rc;
  }

@@ -2502,7 +2514,7 @@ done:
   */
  int __pci_reset_function(struct pci_dev *dev)
  {
-	return pci_dev_reset(dev, 0);
+	return __pci_dev_reset(dev, 0);
  }
  EXPORT_SYMBOL_GPL(__pci_reset_function);
 

@@ -2519,7 +2531,7 @@ EXPORT_SYMBOL_GPL(__pci_reset_function);
   */
  int pci_probe_reset_function(struct pci_dev *dev)
  {
-	return pci_dev_reset(dev, 1);
+	return __pci_dev_reset(dev, 1);
  }
 
  /**

@@ -2542,7 +2554,7 @@ int pci_reset_function(struct pci_dev *dev)
  {
  	int rc;
 
-	rc = pci_dev_reset(dev, 1);
+	rc = __pci_dev_reset(dev, 1);
  	if (rc)
  		return rc;
 

@@ -2563,6 +2575,46 @@ int pci_reset_function(struct pci_dev *dev)
  EXPORT_SYMBOL_GPL(pci_reset_function);
 
  /**
+ * pci_reset_device_function - quiesce and reinitialize a PCI device function
+ * @dev: PCI device to reset
+ *
+ * Some devices allow an individual function to be reset without affecting
+ * other functions in the same device.  The PCI device must be responsive
+ * to PCI config space in order to use this function.
+ *
+ * This function is very similar to pci_reset_function, however this function
+ * does not obtain the device lock during the reset.  This is due to the fact
+ * that the call is meant to be used during probe if the reset_devices
+ * kernel parameter is set.
+ *
+ * Returns 0 if the device function was successfully reset or negative if the
+ * device doesn't support resetting a single function.
+ */
+int pci_reset_device_function(struct pci_dev *dev)
+{
+	int rc;
+
+	rc = __pci_dev_reset(dev, 1);
+	if (rc)
+		return rc;

drivers/pci/pci.c int pci_reset_function(struct pci_dev *dev) · line 2609

Quoted 8 lines · Jeff Kirsher

From: Alexander Duyck<[email protected]>

drivers/pci/pci.c +7 -0

+	/*
+	 * both INTx and MSI are disabled after the Interrupt Disable bit
+	 * is set and the Bus Master bit is cleared.
+	 */
+	pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
+
+	rc = __pci_dev_reset(dev, 0);

Could you tell me why you need to program command register before reset?

"MSI enable" and "Bus Master" bits are cleared by the reset. Furthermore, resetting the device clears the "Interrupt Disable bit", even though it was set just before the rest. So I'm a little confused.

Thanks,
Kenji Kaneshige

Quoted 10 lines · Kenji Kaneshige

Quoted 8 lines · Jeff Kirsher

From: Alexander Duyck<[email protected]>
+ /*
+ * both INTx and MSI are disabled after the Interrupt Disable bit
+ * is set and the Bus Master bit is cleared.
+ */
+ pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE); +
+ rc = __pci_dev_reset(dev, 0);

Quoted 9 lines

Could you tell me why you need to program command register before reset?

"MSI enable" and "Bus Master" bits are cleared by the reset. Furthermore, resetting the device clears the "Interrupt Disable bit", even though it was set just before the rest. So I'm a little confused.

Thanks,
Kenji Kaneshige

The point is to prevent any pending transactions from being on the bus while we are doing the reset. By writing only the INTX disable bit we are disabling all interrupts from being generated, and also disabling all DMA and MSI interrupts since the bus master enable bit is not set.

Without this change the device might be in the middle of a transaction or sending an interrupt while we are doing the reset which may lead to other issues after the reset.