[PATCH 1/5] PCI: protect restore with device lock to be consistent

[PATCH 1/5] PCI: protect restore with device lock to be consistent

[Sinan Kaya]

Commit b014e96d1abb ("PCI: Protect pci_error_handlers->reset_notify() usage
with device_lock()") added protection around pci_dev_restore() function so
that device specific remove callback does not cause a race condition
against hotplug.

pci_dev_lock() usage has been forgotten in two different places in the
code. Adding locks for pci_slot_restore() and moving pci_dev_restore()
inside the locks for pci_try_reset_function().

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6078dfc..23681f4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4344,9 +4344,9 @@ int pci_try_reset_function(struct pci_dev *dev)
 
 	pci_dev_save_and_disable(dev);
 	rc = __pci_reset_function_locked(dev);
+	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
 
-	pci_dev_restore(dev);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(pci_try_reset_function);
@@ -4546,7 +4546,9 @@ static void pci_slot_restore(struct pci_slot *slot)
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
 			continue;
+		pci_dev_lock(dev);
 		pci_dev_restore(dev);
+		pci_dev_unlock(dev);
 		if (dev->subordinate)
 			pci_bus_restore(dev->subordinate);
 	}
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] PCI: handle FLR failure and allow other reset types

[Sinan Kaya]

pci_flr_wait() and pci_af_flr() functions assume graceful return even
though the device is inaccessible under error conditions.

Return -ENOTTY in error cases so that __pci_reset_function_locked() can
try other reset types if AF_FLR/FLR reset fails.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c   | 18 ++++++++++--------
 include/linux/pci.h |  2 +-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 23681f4..27ec45d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3820,7 +3820,7 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_wait_for_pending_transaction);
 
-static void pci_flr_wait(struct pci_dev *dev)
+static int pci_flr_wait(struct pci_dev *dev)
 {
 	int delay = 1, timeout = 60000;
 	u32 id;
@@ -3849,7 +3849,7 @@ static void pci_flr_wait(struct pci_dev *dev)
 		if (delay > timeout) {
 			dev_warn(&dev->dev, "not ready %dms after FLR; giving up\n",
 				 100 + delay - 1);
-			return;
+			return -ENOTTY;
 		}
 
 		if (delay > 1000)
@@ -3863,6 +3863,8 @@ static void pci_flr_wait(struct pci_dev *dev)
 
 	if (delay > 1000)
 		dev_info(&dev->dev, "ready %dms after FLR\n", 100 + delay - 1);
+
+	return 0;
 }
 
 /**
@@ -3891,13 +3893,13 @@ static bool pcie_has_flr(struct pci_dev *dev)
  * device supports FLR before calling this function, e.g. by using the
  * pcie_has_flr() helper.
  */
-void pcie_flr(struct pci_dev *dev)
+int pcie_flr(struct pci_dev *dev)
 {
 	if (!pci_wait_for_pending_transaction(dev))
 		dev_err(&dev->dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
 
 	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
-	pci_flr_wait(dev);
+	return pci_flr_wait(dev);
 }
 EXPORT_SYMBOL_GPL(pcie_flr);
 
@@ -3930,8 +3932,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
 		dev_err(&dev->dev, "timed out waiting for pending transaction; performing AF function level reset anyway\n");
 
 	pci_write_config_byte(dev, pos + PCI_AF_CTRL, PCI_AF_CTRL_FLR);
-	pci_flr_wait(dev);
-	return 0;
+	return pci_flr_wait(dev);
 }
 
 /**
@@ -4203,8 +4204,9 @@ int __pci_reset_function_locked(struct pci_dev *dev)
 	if (rc != -ENOTTY)
 		return rc;
 	if (pcie_has_flr(dev)) {
-		pcie_flr(dev);
-		return 0;
+		rc = pcie_flr(dev);
+		if (rc != -ENOTTY)
+			return rc;
 	}
 	rc = pci_af_flr(dev, 0);
 	if (rc != -ENOTTY)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f68c58a..104224f7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1088,7 +1088,7 @@ static inline int pci_is_managed(struct pci_dev *pdev)
 int pcie_set_mps(struct pci_dev *dev, int mps);
 int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
 			  enum pcie_link_width *width);
-void pcie_flr(struct pci_dev *dev);
+int pcie_flr(struct pci_dev *dev);
 int __pci_reset_function(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] PCI: handle FLR failure and allow other reset types

[Bjorn Helgaas]

On Sat, Sep 23, 2017 at 08:16:55PM -0400, Sinan Kaya wrote:
> pci_flr_wait() and pci_af_flr() functions assume graceful return even
> though the device is inaccessible under error conditions.
> 
> Return -ENOTTY in error cases so that __pci_reset_function_locked() can
> try other reset types if AF_FLR/FLR reset fails.

This makes sense to me, but I think the error handling in
__pci_reset_function_locked() is confusing.  It currently is:

  rc = pci_dev_specific_reset(dev, 0);
  if (rc != -ENOTTY)
    return rc;
  if (pcie_has_flr(dev)) {
    pcie_flr(dev);
    return 0;
  }
  rc = pci_af_flr(dev, 0);
  if (rc != -ENOTTY)
    return rc;

Would it make sense to change this to the following?

  rc = pci_dev_specific_reset(dev, 0);
  if (rc == 0)
    return 0;

  if (pcie_has_flr(dev)) {
    pcie_flr(dev);
    return 0;
  }

  rc = pci_af_flr(dev, 0);
  if (rc == 0)
    return 0;

I found two cases where this would make a difference: reset_ivb_igd()
returns -ENOMEM if pci_iomap() fails, and pci_pm_reset() returns
-EINVAL if the device is not in D0.

In both cases we currently return the failure, but it would seem
reasonable to me to try another reset method.

That could be done in a new patch before this one.  Then *this* patch
could use -ETIMEDOUT instead of -ENOTTY, and I think the whole thing
would become a little more readable.

> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c   | 18 ++++++++++--------
>  include/linux/pci.h |  2 +-
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 23681f4..27ec45d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3820,7 +3820,7 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL(pci_wait_for_pending_transaction);
>  
> -static void pci_flr_wait(struct pci_dev *dev)
> +static int pci_flr_wait(struct pci_dev *dev)
>  {
>  	int delay = 1, timeout = 60000;
>  	u32 id;
> @@ -3849,7 +3849,7 @@ static void pci_flr_wait(struct pci_dev *dev)
>  		if (delay > timeout) {
>  			dev_warn(&dev->dev, "not ready %dms after FLR; giving up\n",
>  				 100 + delay - 1);
> -			return;
> +			return -ENOTTY;
>  		}
>  
>  		if (delay > 1000)
> @@ -3863,6 +3863,8 @@ static void pci_flr_wait(struct pci_dev *dev)
>  
>  	if (delay > 1000)
>  		dev_info(&dev->dev, "ready %dms after FLR\n", 100 + delay - 1);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -3891,13 +3893,13 @@ static bool pcie_has_flr(struct pci_dev *dev)
>   * device supports FLR before calling this function, e.g. by using the
>   * pcie_has_flr() helper.
>   */
> -void pcie_flr(struct pci_dev *dev)
> +int pcie_flr(struct pci_dev *dev)
>  {
>  	if (!pci_wait_for_pending_transaction(dev))
>  		dev_err(&dev->dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
>  
>  	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
> -	pci_flr_wait(dev);
> +	return pci_flr_wait(dev);
>  }
>  EXPORT_SYMBOL_GPL(pcie_flr);
>  
> @@ -3930,8 +3932,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>  		dev_err(&dev->dev, "timed out waiting for pending transaction; performing AF function level reset anyway\n");
>  
>  	pci_write_config_byte(dev, pos + PCI_AF_CTRL, PCI_AF_CTRL_FLR);
> -	pci_flr_wait(dev);
> -	return 0;
> +	return pci_flr_wait(dev);
>  }
>  
>  /**
> @@ -4203,8 +4204,9 @@ int __pci_reset_function_locked(struct pci_dev *dev)
>  	if (rc != -ENOTTY)
>  		return rc;
>  	if (pcie_has_flr(dev)) {
> -		pcie_flr(dev);
> -		return 0;
> +		rc = pcie_flr(dev);
> +		if (rc != -ENOTTY)
> +			return rc;
>  	}
>  	rc = pci_af_flr(dev, 0);
>  	if (rc != -ENOTTY)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f68c58a..104224f7 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1088,7 +1088,7 @@ static inline int pci_is_managed(struct pci_dev *pdev)
>  int pcie_set_mps(struct pci_dev *dev, int mps);
>  int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed,
>  			  enum pcie_link_width *width);
> -void pcie_flr(struct pci_dev *dev);
> +int pcie_flr(struct pci_dev *dev);
>  int __pci_reset_function(struct pci_dev *dev);
>  int __pci_reset_function_locked(struct pci_dev *dev);
>  int pci_reset_function(struct pci_dev *dev);
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] PCI: handle FLR failure and allow other reset types

[Sinan Kaya]

On 10/11/2017 5:00 PM, Bjorn Helgaas wrote:
> On Sat, Sep 23, 2017 at 08:16:55PM -0400, Sinan Kaya wrote:
>> pci_flr_wait() and pci_af_flr() functions assume graceful return even
>> though the device is inaccessible under error conditions.
>>
>> Return -ENOTTY in error cases so that __pci_reset_function_locked() can
>> try other reset types if AF_FLR/FLR reset fails.
> 
> This makes sense to me, but I think the error handling in
> __pci_reset_function_locked() is confusing.  It currently is:
> 
>   rc = pci_dev_specific_reset(dev, 0);
>   if (rc != -ENOTTY)
>     return rc;
>   if (pcie_has_flr(dev)) {
>     pcie_flr(dev);
>     return 0;
>   }
>   rc = pci_af_flr(dev, 0);
>   if (rc != -ENOTTY)
>     return rc;
> 
> Would it make sense to change this to the following?
> 
>   rc = pci_dev_specific_reset(dev, 0);
>   if (rc == 0)
>     return 0;
> 
>   if (pcie_has_flr(dev)) {
>     pcie_flr(dev);
>     return 0;
>   }
> 
>   rc = pci_af_flr(dev, 0);
>   if (rc == 0)
>     return 0;
> 

Yeah, this is cleaner. I'll create a separate patch for that. 

> I found two cases where this would make a difference: reset_ivb_igd()
> returns -ENOMEM if pci_iomap() fails, and pci_pm_reset() returns
> -EINVAL if the device is not in D0.
> 
> In both cases we currently return the failure, but it would seem
> reasonable to me to try another reset method.
> 
> That could be done in a new patch before this one.  Then *this* patch
> could use -ETIMEDOUT instead of -ENOTTY, and I think the whole thing
> would become a little more readable.
> 
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/5] PCI: make pci_flr_wait() generic and rename to pci_dev_wait()

[Sinan Kaya]

Rev 3.1 Sec 2.3.1 Request Handling Rules:
Valid reset conditions after which a device is permitted to return CRS
are:
* Cold, Warm, and Hot Resets,
* FLR
* A reset initiated in response to a D3hot to D0 uninitialized

Try to reuse FLR implementation towards other reset types.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 27ec45d..fd4a3b6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3820,20 +3820,14 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_wait_for_pending_transaction);
 
-static int pci_flr_wait(struct pci_dev *dev)
+static int pci_dev_wait(struct pci_dev *dev, char *reset_type,
+			int initial_wait, int timeout)
 {
-	int delay = 1, timeout = 60000;
+	int delay = 1;
 	u32 id;
 
 	/*
-	 * Per PCIe r3.1, sec 6.6.2, a device must complete an FLR within
-	 * 100ms, but may silently discard requests while the FLR is in
-	 * progress.  Wait 100ms before trying to access the device.
-	 */
-	msleep(100);
-
-	/*
-	 * After 100ms, the device should not silently discard config
+	 * After reset, the device should not silently discard config
 	 * requests, but it may still indicate that it needs more time by
 	 * responding to them with CRS completions.  The Root Port will
 	 * generally synthesize ~0 data to complete the read (except when
@@ -3847,14 +3841,14 @@ static int pci_flr_wait(struct pci_dev *dev)
 	pci_read_config_dword(dev, PCI_COMMAND, &id);
 	while (id == ~0) {
 		if (delay > timeout) {
-			dev_warn(&dev->dev, "not ready %dms after FLR; giving up\n",
-				 100 + delay - 1);
+			dev_warn(&dev->dev, "not ready %dms after %s; giving up\n",
+				 initial_wait + delay - 1, reset_type);
 			return -ENOTTY;
 		}
 
 		if (delay > 1000)
-			dev_info(&dev->dev, "not ready %dms after FLR; waiting\n",
-				 100 + delay - 1);
+			dev_info(&dev->dev, "not ready %dms after %s; waiting\n",
+				 initial_wait + delay - 1, reset_type);
 
 		msleep(delay);
 		delay *= 2;
@@ -3862,7 +3856,8 @@ static int pci_flr_wait(struct pci_dev *dev)
 	}
 
 	if (delay > 1000)
-		dev_info(&dev->dev, "ready %dms after FLR\n", 100 + delay - 1);
+		dev_info(&dev->dev, "ready %dms after %s\n",
+			 initial_wait + delay - 1, reset_type);
 
 	return 0;
 }
@@ -3899,7 +3894,15 @@ int pcie_flr(struct pci_dev *dev)
 		dev_err(&dev->dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
 
 	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
-	return pci_flr_wait(dev);
+
+	/*
+	 * Per PCIe r3.1, sec 6.6.2, a device must complete an FLR within
+	 * 100ms, but may silently discard requests while the FLR is in
+	 * progress.  Wait 100ms before trying to access the device.
+	 */
+	msleep(100);
+
+	return pci_dev_wait(dev, "FLR", 100, 60000);
 }
 EXPORT_SYMBOL_GPL(pcie_flr);
 
@@ -3932,7 +3935,15 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
 		dev_err(&dev->dev, "timed out waiting for pending transaction; performing AF function level reset anyway\n");
 
 	pci_write_config_byte(dev, pos + PCI_AF_CTRL, PCI_AF_CTRL_FLR);
-	return pci_flr_wait(dev);
+
+	/*
+	 * Per PCIe r3.1, sec 6.6.2, a device must complete an FLR within
+	 * 100ms, but may silently discard requests while the FLR is in
+	 * progress.  Wait 100ms before trying to access the device.
+	 */
+	msleep(100);
+
+	return pci_dev_wait(dev, "AF_FLR", 100, 60000);
 }
 
 /**
-- 
1.9.1

Re: [PATCH 3/5] PCI: make pci_flr_wait() generic and rename to pci_dev_wait()

[Christoph Hellwig]

On Sat, Sep 23, 2017 at 08:16:56PM -0400, Sinan Kaya wrote:
> Rev 3.1 Sec 2.3.1 Request Handling Rules:
> Valid reset conditions after which a device is permitted to return CRS
> are:
> * Cold, Warm, and Hot Resets,
> * FLR
> * A reset initiated in response to a D3hot to D0 uninitialized
> 
> Try to reuse FLR implementation towards other reset types.

Maybe call it pci_reset_wait instead of pci_dev_wait?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] PCI: make pci_flr_wait() generic and rename to pci_dev_wait()

[Sinan Kaya]

On 9/24/2017 9:08 AM, Christoph Hellwig wrote:
> On Sat, Sep 23, 2017 at 08:16:56PM -0400, Sinan Kaya wrote:
>> Rev 3.1 Sec 2.3.1 Request Handling Rules:
>> Valid reset conditions after which a device is permitted to return CRS
>> are:
>> * Cold, Warm, and Hot Resets,
>> * FLR
>> * A reset initiated in response to a D3hot to D0 uninitialized
>>
>> Try to reuse FLR implementation towards other reset types.
> 
> Maybe call it pci_reset_wait instead of pci_dev_wait?
> 

I can go for pci_dev_reset_wait(). I was using the PCI directory function
naming nomenclature where some functions using struct pci_dev start with
pci_dev and others using struct pci_bus start with pci_bus.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] PCI: wait device ready after pci_pm_reset()

[Sinan Kaya]

Rev 3.1 Sec 2.3.1 Request Handling Rules says a device can issue CRS
following a D3hot->D0 transition. Add pci_dev_wait() call with 1 second
timeout to see if device is available before returning.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index fd4a3b6..074adf9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3963,6 +3963,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
  */
 static int pci_pm_reset(struct pci_dev *dev, int probe)
 {
+	unsigned int delay = dev->d3_delay;
 	u16 csr;
 
 	if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET)
@@ -3988,7 +3989,10 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
 	pci_dev_d3_sleep(dev);
 
-	return 0;
+	if (delay < pci_pm_d3_delay)
+		delay = pci_pm_d3_delay;
+
+	return pci_dev_wait(dev, "PM D3->D0", delay, 1000);
 }
 
 void pci_reset_secondary_bus(struct pci_dev *dev)
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] PCI: wait device ready after pci_pm_reset()

[Bjorn Helgaas]

On Sat, Sep 23, 2017 at 08:16:57PM -0400, Sinan Kaya wrote:
> Rev 3.1 Sec 2.3.1 Request Handling Rules says a device can issue CRS
> following a D3hot->D0 transition. Add pci_dev_wait() call with 1 second
> timeout to see if device is available before returning.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index fd4a3b6..074adf9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3963,6 +3963,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>   */
>  static int pci_pm_reset(struct pci_dev *dev, int probe)
>  {
> +	unsigned int delay = dev->d3_delay;
>  	u16 csr;
>  
>  	if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET)
> @@ -3988,7 +3989,10 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
>  	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
>  	pci_dev_d3_sleep(dev);
>  
> -	return 0;
> +	if (delay < pci_pm_d3_delay)
> +		delay = pci_pm_d3_delay;
> +
> +	return pci_dev_wait(dev, "PM D3->D0", delay, 1000);

1) Why do we wait up to 1 second here, when we wait up to 60 seconds
for the other methods?  Can they all be the same?  Maybe a #define for
it?

2) I don't really like the fact that we do the initial sleep one place
and then pass the length of that sleep here.  It's hard to verify
they're the same and keep them in sync.  I think the only thing you
use initial_wait for is to include that time in the dmesg messages.
Maybe we should just omit that time from the message and drop the
parameter?

>  }
>  
>  void pci_reset_secondary_bus(struct pci_dev *dev)
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] PCI: wait device ready after pci_pm_reset()

[Sinan Kaya]

On 10/11/2017 6:06 PM, Bjorn Helgaas wrote:
>> static int pci_pm_reset(struct pci_dev *dev, int probe)
>>  {
>> +	unsigned int delay = dev->d3_delay;
>>  	u16 csr;
>>  
>>  	if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET)
>> @@ -3988,7 +3989,10 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
>>  	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
>>  	pci_dev_d3_sleep(dev);
>>  
>> -	return 0;
>> +	if (delay < pci_pm_d3_delay)
>> +		delay = pci_pm_d3_delay;
>> +
>> +	return pci_dev_wait(dev, "PM D3->D0", delay, 1000);
> 1) Why do we wait up to 1 second here, when we wait up to 60 seconds
> for the other methods?  Can they all be the same?  Maybe a #define for
> it?

I know you want to have similar behavior for systems that do and do not support
CRS. That was the reason why I converted flr wait function to into dev_wait function.

However, here is the problem:

For systems that do not support CRS, there is no way of knowing whether we
are reading 0xFFFFFFFF because the endpoint is not reachable due to an error
like "it doesn't support this reset type" or if it is actually emitting a CRS.

If one system has a problem with pm_reset, this code would add an unnecessary
1 second delay into the reset path. If I make it 60 it would be something like:

1. try reset method A
2. wait 60 seconds
3. try reset method B
4. wait 60 seconds. 
5. try reset method C
6. wait 60 seconds

This might end up being a regression on some system. 

I'm still leaning towards a wait only if we are observing a CRS. What's your
thought on this?

then the sequence would be.

1. try reset method A
2. if CRS pending, wait 60 seconds
3. try reset method B
4. if CRS pending, wait 60 seconds. 
5. try reset method C
6. if CRS pending, wait 60 seconds

> 
> 2) I don't really like the fact that we do the initial sleep one place
> and then pass the length of that sleep here.  It's hard to verify
> they're the same and keep them in sync.  I think the only thing you
> use initial_wait for is to include that time in the dmesg messages.
> Maybe we should just omit that time from the message and drop the
> parameter?
> 

This was for printing reasons like you spotted, I can certainly get rid of
the initial_wait.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 4/5] PCI: wait device ready after pci_pm_reset()

[Sinan Kaya]

On 10/12/2017 12:48 PM, Sinan Kaya wrote:
> On 10/11/2017 6:06 PM, Bjorn Helgaas wrote:
>>> static int pci_pm_reset(struct pci_dev *dev, int probe)
>>>  {
>>> +	unsigned int delay = dev->d3_delay;
>>>  	u16 csr;
>>>  
>>>  	if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET)
>>> @@ -3988,7 +3989,10 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
>>>  	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
>>>  	pci_dev_d3_sleep(dev);
>>>  
>>> -	return 0;
>>> +	if (delay < pci_pm_d3_delay)
>>> +		delay = pci_pm_d3_delay;
>>> +
>>> +	return pci_dev_wait(dev, "PM D3->D0", delay, 1000);
>> 1) Why do we wait up to 1 second here, when we wait up to 60 seconds
>> for the other methods?  Can they all be the same?  Maybe a #define for
>> it?
> 
> I know you want to have similar behavior for systems that do and do not support
> CRS. That was the reason why I converted flr wait function to into dev_wait function.
> 
> However, here is the problem:
> 
> For systems that do not support CRS, there is no way of knowing whether we
> are reading 0xFFFFFFFF because the endpoint is not reachable due to an error
> like "it doesn't support this reset type" or if it is actually emitting a CRS.
> 
> If one system has a problem with pm_reset, this code would add an unnecessary
> 1 second delay into the reset path. If I make it 60 it would be something like:
> 
> 1. try reset method A
> 2. wait 60 seconds
> 3. try reset method B
> 4. wait 60 seconds. 
> 5. try reset method C
> 6. wait 60 seconds
> 
> This might end up being a regression on some system. 
> 
> I'm still leaning towards a wait only if we are observing a CRS. What's your
> thought on this?
> 
> then the sequence would be.
> 
> 1. try reset method A
> 2. if CRS pending, wait 60 seconds
> 3. try reset method B
> 4. if CRS pending, wait 60 seconds. 
> 5. try reset method C
> 6. if CRS pending, wait 60 seconds
> 

Thinking more about this. Another possibility is to have an adjustable sleep time.
Start with 60 seconds for all reset types. If somebody doesn't like it,
have a kernel command line override.

>>
>> 2) I don't really like the fact that we do the initial sleep one place
>> and then pass the length of that sleep here.  It's hard to verify
>> they're the same and keep them in sync.  I think the only thing you
>> use initial_wait for is to include that time in the dmesg messages.
>> Maybe we should just omit that time from the message and drop the
>> parameter?
>>
> 
> This was for printing reasons like you spotted, I can certainly get rid of
> the initial_wait.
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 5/5] PCI: add device wait after slot and bus reset

[Sinan Kaya]

Rev 3.1 Sec 2.3.1 Request Handling Rules indicates that a device can issue
CRS following secondary bus reset. Handle device presence gracefully.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 074adf9..7131aab 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4056,7 +4056,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
 
 	pci_reset_bridge_secondary_bus(dev->bus->self);
 
-	return 0;
+	return pci_dev_wait(dev, "bus reset", 1000, 60000);
 }
 
 static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, int probe)
@@ -4077,6 +4077,7 @@ static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, int probe)
 static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
 {
 	struct pci_dev *pdev;
+	int rc;
 
 	if (dev->subordinate || !dev->slot ||
 	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
@@ -4086,7 +4087,11 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
 		if (pdev != dev && pdev->slot == dev->slot)
 			return -ENOTTY;
 
-	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
+	rc = pci_reset_hotplug_slot(dev->slot->hotplug, probe);
+	if (!rc && !probe)
+		rc = pci_dev_wait(dev, "slot reset", 1000, 60000);
+
+	return rc;
 }
 
 static void pci_dev_lock(struct pci_dev *dev)
-- 
1.9.1

Re: [PATCH 5/5] PCI: add device wait after slot and bus reset

[Sinan Kaya]

On 9/23/2017 8:16 PM, Sinan Kaya wrote:
> @@ -4056,7 +4056,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
>  
>  	pci_reset_bridge_secondary_bus(dev->bus->self);
>  
> -	return 0;
> +	return pci_dev_wait(dev, "bus reset", 1000, 60000);
>  }

This doesn't solve the warm reset message getting broadcasted. I wanted to get
feedback on the direction first with this version.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/5] PCI: protect restore with device lock to be consistent

[Bjorn Helgaas]

I think this series makes a lot of sense overall; thanks for doing
this work!  I had a few comments on the individual patches.

(This response would ordinarily be to the cover letter, but there
isn't one, so I'm just responding to the first patch.)

On Sat, Sep 23, 2017 at 08:16:54PM -0400, Sinan Kaya wrote:
> Commit b014e96d1abb ("PCI: Protect pci_error_handlers->reset_notify() usage
> with device_lock()") added protection around pci_dev_restore() function so
> that device specific remove callback does not cause a race condition
> against hotplug.
> 
> pci_dev_lock() usage has been forgotten in two different places in the
> code. Adding locks for pci_slot_restore() and moving pci_dev_restore()
> inside the locks for pci_try_reset_function().
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6078dfc..23681f4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4344,9 +4344,9 @@ int pci_try_reset_function(struct pci_dev *dev)
>  
>  	pci_dev_save_and_disable(dev);
>  	rc = __pci_reset_function_locked(dev);
> +	pci_dev_restore(dev);
>  	pci_dev_unlock(dev);
>  
> -	pci_dev_restore(dev);
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(pci_try_reset_function);
> @@ -4546,7 +4546,9 @@ static void pci_slot_restore(struct pci_slot *slot)
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
> +		pci_dev_lock(dev);
>  		pci_dev_restore(dev);
> +		pci_dev_unlock(dev);
>  		if (dev->subordinate)
>  			pci_bus_restore(dev->subordinate);
>  	}
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] PCI: protect restore with device lock to be consistent

[Sinan Kaya]

On 10/11/2017 6:08 PM, Bjorn Helgaas wrote:
> I think this series makes a lot of sense overall; thanks for doing
> this work!  I had a few comments on the individual patches.
> 
> (This response would ordinarily be to the cover letter, but there
> isn't one, so I'm just responding to the first patch.)

Thanks, I was too lazy to type it up. I'll do it on the next one.
Since this was an RFC even though the subject doesn't say so, I didn't
want to invest too much time on documentation.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.