[RFC PATCH 1/2] pci: add function reset call that can be used inside of probe

[RFC PATCH 1/2] pci: add function reset call that can be used inside of probe

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60f30e7..1421bc7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
+
+	pci_save_state(dev);
+
+	/*
+	 * 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);
+
+	pci_restore_state(dev);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(pci_reset_device_function);
+
+/**
  * pcix_get_max_mmrbc - get PCI-X maximum designed memory read byte count
  * @dev: PCI device to query
  *
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6a471ab..c7708d3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -789,6 +789,7 @@ int pcie_get_readrq(struct pci_dev *dev);
 int pcie_set_readrq(struct pci_dev *dev, int rq);
 int __pci_reset_function(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
+int pci_reset_device_function(struct pci_dev *dev);
 void pci_update_resource(struct pci_dev *dev, int resno);
 int __must_check pci_assign_resource(struct pci_dev *dev, int i);
 int pci_select_bars(struct pci_dev *dev, unsigned long flags);

[RFC PATCH 2/2] igb/ixgbe: add code to trigger function reset if reset_devices is set

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_main.c     |    3 +++
 drivers/net/ixgbe/ixgbe_main.c |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 667b527..b924443 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1731,6 +1731,9 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 		return -EINVAL;
 	}
 
+	if (reset_devices && pci_reset_device_function(pdev))
+		return -ENODEV;
+
 	err = pci_enable_device_mem(pdev);
 	if (err)
 		return err;
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 7d6a415..f459f24 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -6548,6 +6548,9 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 		return -EINVAL;
 	}
 
+	if (reset_devices && pci_reset_device_function(pdev))
+		return -ENODEV;
+
 	err = pci_enable_device_mem(pdev);
 	if (err)
 		return err;

Re: [RFC PATCH 2/2] igb/ixgbe: add code to trigger function reset if reset_devices is set

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 30 Jul 2010 17:59:12 -0700

> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> 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 <alexander.h.duyck@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

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?

No, you dun goof'd.  :-) Find another way to fix this please.

Thanks.

Re: [RFC PATCH 2/2] igb/ixgbe: add code to trigger function reset if reset_devices is set

[David Woodhouse]

On Sun, 2010-08-01 at 01:15 -0700, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Fri, 30 Jul 2010 17:59:12 -0700
> 
> > From: Alexander Duyck <alexander.h.duyck@intel.com>
> > 
> > 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 <alexander.h.duyck@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> 
> 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).

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

I don't like the reset_devices thing though -- the device driver ought
to cope (and reset the device with a full PCIe reset if that's the only
way to make it stop sulking) *regardless* of that option, if it's
necessary.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC PATCH 2/2] igb/ixgbe: add code to trigger function reset if reset_devices is set

[Kenji Kaneshige]

(2010/08/06 1:27), David Woodhouse wrote:
> On Sun, 2010-08-01 at 01:15 -0700, David Miller wrote:
>> From: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>> Date: Fri, 30 Jul 2010 17:59:12 -0700
>>
>>> From: Alexander Duyck<alexander.h.duyck@intel.com>
>>>
>>> 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<alexander.h.duyck@intel.com>
>>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>
>> 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).
>
>> 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.

What about asserting secondary RST# on the bridge?
It would not work for devices on the root bus though.

Thanks,
Kenji Kaneshige

Re: [RFC PATCH 1/2] pci: add function reset call that can be used inside of probe

[Kenji Kaneshige]

(2010/07/31 9:58), Jeff Kirsher wrote:
> From: Alexander Duyck<alexander.h.duyck@intel.com>
>
> 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<alexander.h.duyck@intel.com>
> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> ---
>
>   drivers/pci/pci.c   |   74 +++++++++++++++++++++++++++++++++++++++++++--------
>   include/linux/pci.h |    1 +
>   2 files changed, 64 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60f30e7..1421bc7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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;
> +
> +	pci_save_state(dev);
> +
> +	/*
> +	 * 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

Re: [RFC PATCH 1/2] pci: add function reset call that can be used inside of probe

[Alexander Duyck]

Kenji Kaneshige wrote:
> (2010/07/31 9:58), Jeff Kirsher wrote:
>> From: Alexander Duyck<alexander.h.duyck@intel.com>
>> +	/*
>> +	 * 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
> 

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.

Thanks,

Alex

Re: [RFC PATCH 1/2] pci: add function reset call that can be used inside of probe

[Kenji Kaneshige]

(2010/08/11 8:14), Alexander Duyck wrote:
> Kenji Kaneshige wrote:
>> (2010/07/31 9:58), Jeff Kirsher wrote:
>>> From: Alexander Duyck<alexander.h.duyck@intel.com>
>>> + /*
>>> + * 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
>>
>
> 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.
>

Thank you for clarification. I understood.

Thanks,
Kenji Kaneshige