[PATCH] PCI: Introduce pci_dev_wait() in pci_power_up() API

[PATCH] PCI: Introduce pci_dev_wait() in pci_power_up() API

[Vidya Sagar]

Add pci_dev_wait() in pci_power_up() before accessing the configuration
space of a device for the first time in the system resume sequence.
This is  to accommodate devices (Ex:- Intel 750 NVMe) that respond with CRS
status while they get ready for configuration space access and before they
finally start responding with proper values. It also refactors code to move
pci_dev_wait() ahead of pci_power_up() to avoid build error.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/pci.c | 89 +++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 599b2fc99234..7672b9a44bac 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1020,6 +1020,47 @@ void pci_wakeup_bus(struct pci_bus *bus)
 		pci_walk_bus(bus, pci_wakeup, NULL);
 }
 
+static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
+{
+	int delay = 1;
+	u32 id;
+
+	/*
+	 * 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
+	 * CRS SV is enabled and the read was for the Vendor ID; in that
+	 * case it synthesizes 0x0001 data).
+	 *
+	 * Wait for the device to return a non-CRS completion.  Read the
+	 * Command register instead of Vendor ID so we don't have to
+	 * contend with the CRS SV value.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &id);
+	while (id == ~0) {
+		if (delay > timeout) {
+			pci_warn(dev, "not ready %dms after %s; giving up\n",
+				 delay - 1, reset_type);
+			return -ENOTTY;
+		}
+
+		if (delay > 1000)
+			pci_info(dev, "not ready %dms after %s; waiting\n",
+				 delay - 1, reset_type);
+
+		msleep(delay);
+		delay *= 2;
+		pci_read_config_dword(dev, PCI_COMMAND, &id);
+	}
+
+	if (delay > 1000)
+		pci_info(dev, "ready %dms after %s\n", delay - 1,
+			 reset_type);
+
+	return 0;
+}
+
 /**
  * pci_power_up - Put the given device into D0
  * @dev: PCI device to power up
@@ -1045,6 +1086,13 @@ int pci_power_up(struct pci_dev *dev)
 		pci_wakeup_bus(dev->subordinate);
 	}
 
+	/*
+	 * Wait for those devices (Ex: Intel 750 NVMe) that are not ready yet
+	 * and responding with CRS statuses for the configuration space
+	 * requests.
+	 */
+	pci_dev_wait(dev, "Switch to D0", PCIE_RESET_READY_POLL_MS);
+
 	return pci_raw_set_power_state(dev, PCI_D0);
 }
 
@@ -4420,47 +4468,6 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_wait_for_pending_transaction);
 
-static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
-{
-	int delay = 1;
-	u32 id;
-
-	/*
-	 * 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
-	 * CRS SV is enabled and the read was for the Vendor ID; in that
-	 * case it synthesizes 0x0001 data).
-	 *
-	 * Wait for the device to return a non-CRS completion.  Read the
-	 * Command register instead of Vendor ID so we don't have to
-	 * contend with the CRS SV value.
-	 */
-	pci_read_config_dword(dev, PCI_COMMAND, &id);
-	while (id == ~0) {
-		if (delay > timeout) {
-			pci_warn(dev, "not ready %dms after %s; giving up\n",
-				 delay - 1, reset_type);
-			return -ENOTTY;
-		}
-
-		if (delay > 1000)
-			pci_info(dev, "not ready %dms after %s; waiting\n",
-				 delay - 1, reset_type);
-
-		msleep(delay);
-		delay *= 2;
-		pci_read_config_dword(dev, PCI_COMMAND, &id);
-	}
-
-	if (delay > 1000)
-		pci_info(dev, "ready %dms after %s\n", delay - 1,
-			 reset_type);
-
-	return 0;
-}
-
 /**
  * pcie_has_flr - check if a device supports function level resets
  * @dev: device to check
-- 
2.17.1

Re: [PATCH] PCI: Introduce pci_dev_wait() in pci_power_up() API

[Bjorn Helgaas]

On Mon, Nov 18, 2019 at 10:53:10PM +0530, Vidya Sagar wrote:
> Add pci_dev_wait() in pci_power_up() before accessing the configuration
> space of a device for the first time in the system resume sequence.
> This is  to accommodate devices (Ex:- Intel 750 NVMe) that respond with CRS
> status while they get ready for configuration space access and before they
> finally start responding with proper values. It also refactors code to move
> pci_dev_wait() ahead of pci_power_up() to avoid build error.

Would you mind splitting this into:

  1) Move the pci_dev_wait() definition (no functional change)
  2) Add the call to pci_dev_wait() from pci_power_up()

Then the important change will stand out instead of being buried in
the middle of a big diff that mostly doesn't do anything.

I'm getting uneasy with our use of PCI_COMMAND instead of
PCI_VENDOR_ID.  If we're going to enable CRS SV, it seems like we
ought to use it in the way the spec suggests, i.e., by reading the
Vendor ID.  But that's something for a future patch, not *this* patch.

> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/pci.c | 89 +++++++++++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 599b2fc99234..7672b9a44bac 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1020,6 +1020,47 @@ void pci_wakeup_bus(struct pci_bus *bus)
>  		pci_walk_bus(bus, pci_wakeup, NULL);
>  }
>  
> +static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
> +{
> +	int delay = 1;
> +	u32 id;
> +
> +	/*
> +	 * 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
> +	 * CRS SV is enabled and the read was for the Vendor ID; in that
> +	 * case it synthesizes 0x0001 data).
> +	 *
> +	 * Wait for the device to return a non-CRS completion.  Read the
> +	 * Command register instead of Vendor ID so we don't have to
> +	 * contend with the CRS SV value.
> +	 */
> +	pci_read_config_dword(dev, PCI_COMMAND, &id);
> +	while (id == ~0) {
> +		if (delay > timeout) {
> +			pci_warn(dev, "not ready %dms after %s; giving up\n",
> +				 delay - 1, reset_type);
> +			return -ENOTTY;
> +		}
> +
> +		if (delay > 1000)
> +			pci_info(dev, "not ready %dms after %s; waiting\n",
> +				 delay - 1, reset_type);
> +
> +		msleep(delay);
> +		delay *= 2;
> +		pci_read_config_dword(dev, PCI_COMMAND, &id);
> +	}
> +
> +	if (delay > 1000)
> +		pci_info(dev, "ready %dms after %s\n", delay - 1,
> +			 reset_type);
> +
> +	return 0;
> +}
> +
>  /**
>   * pci_power_up - Put the given device into D0
>   * @dev: PCI device to power up
> @@ -1045,6 +1086,13 @@ int pci_power_up(struct pci_dev *dev)
>  		pci_wakeup_bus(dev->subordinate);
>  	}
>  
> +	/*
> +	 * Wait for those devices (Ex: Intel 750 NVMe) that are not ready yet
> +	 * and responding with CRS statuses for the configuration space
> +	 * requests.
> +	 */
> +	pci_dev_wait(dev, "Switch to D0", PCIE_RESET_READY_POLL_MS);
> +
>  	return pci_raw_set_power_state(dev, PCI_D0);
>  }
>  
> @@ -4420,47 +4468,6 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL(pci_wait_for_pending_transaction);
>  
> -static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
> -{
> -	int delay = 1;
> -	u32 id;
> -
> -	/*
> -	 * 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
> -	 * CRS SV is enabled and the read was for the Vendor ID; in that
> -	 * case it synthesizes 0x0001 data).
> -	 *
> -	 * Wait for the device to return a non-CRS completion.  Read the
> -	 * Command register instead of Vendor ID so we don't have to
> -	 * contend with the CRS SV value.
> -	 */
> -	pci_read_config_dword(dev, PCI_COMMAND, &id);
> -	while (id == ~0) {
> -		if (delay > timeout) {
> -			pci_warn(dev, "not ready %dms after %s; giving up\n",
> -				 delay - 1, reset_type);
> -			return -ENOTTY;
> -		}
> -
> -		if (delay > 1000)
> -			pci_info(dev, "not ready %dms after %s; waiting\n",
> -				 delay - 1, reset_type);
> -
> -		msleep(delay);
> -		delay *= 2;
> -		pci_read_config_dword(dev, PCI_COMMAND, &id);
> -	}
> -
> -	if (delay > 1000)
> -		pci_info(dev, "ready %dms after %s\n", delay - 1,
> -			 reset_type);
> -
> -	return 0;
> -}
> -
>  /**
>   * pcie_has_flr - check if a device supports function level resets
>   * @dev: device to check
> -- 
> 2.17.1
> 

[PATCH V2 1/2] PCI: Move the definition of pci_dev_wait()

[Vidya Sagar]

Move the definition of pci_dev_wait() above pci_power_up() so that it
can be called from the latter with no change in functionality.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/pci.c | 82 +++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 599b2fc99234..71b45ce73bf6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1020,6 +1020,47 @@ void pci_wakeup_bus(struct pci_bus *bus)
 		pci_walk_bus(bus, pci_wakeup, NULL);
 }
 
+static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
+{
+	int delay = 1;
+	u32 id;
+
+	/*
+	 * 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
+	 * CRS SV is enabled and the read was for the Vendor ID; in that
+	 * case it synthesizes 0x0001 data).
+	 *
+	 * Wait for the device to return a non-CRS completion.  Read the
+	 * Command register instead of Vendor ID so we don't have to
+	 * contend with the CRS SV value.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &id);
+	while (id == ~0) {
+		if (delay > timeout) {
+			pci_warn(dev, "not ready %dms after %s; giving up\n",
+				 delay - 1, reset_type);
+			return -ENOTTY;
+		}
+
+		if (delay > 1000)
+			pci_info(dev, "not ready %dms after %s; waiting\n",
+				 delay - 1, reset_type);
+
+		msleep(delay);
+		delay *= 2;
+		pci_read_config_dword(dev, PCI_COMMAND, &id);
+	}
+
+	if (delay > 1000)
+		pci_info(dev, "ready %dms after %s\n", delay - 1,
+			 reset_type);
+
+	return 0;
+}
+
 /**
  * pci_power_up - Put the given device into D0
  * @dev: PCI device to power up
@@ -4420,47 +4461,6 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_wait_for_pending_transaction);
 
-static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
-{
-	int delay = 1;
-	u32 id;
-
-	/*
-	 * 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
-	 * CRS SV is enabled and the read was for the Vendor ID; in that
-	 * case it synthesizes 0x0001 data).
-	 *
-	 * Wait for the device to return a non-CRS completion.  Read the
-	 * Command register instead of Vendor ID so we don't have to
-	 * contend with the CRS SV value.
-	 */
-	pci_read_config_dword(dev, PCI_COMMAND, &id);
-	while (id == ~0) {
-		if (delay > timeout) {
-			pci_warn(dev, "not ready %dms after %s; giving up\n",
-				 delay - 1, reset_type);
-			return -ENOTTY;
-		}
-
-		if (delay > 1000)
-			pci_info(dev, "not ready %dms after %s; waiting\n",
-				 delay - 1, reset_type);
-
-		msleep(delay);
-		delay *= 2;
-		pci_read_config_dword(dev, PCI_COMMAND, &id);
-	}
-
-	if (delay > 1000)
-		pci_info(dev, "ready %dms after %s\n", delay - 1,
-			 reset_type);
-
-	return 0;
-}
-
 /**
  * pcie_has_flr - check if a device supports function level resets
  * @dev: device to check
-- 
2.17.1

[PATCH V2 2/2] PCI: Introduce pci_dev_wait() in pci_power_up() API

[Vidya Sagar]

Add pci_dev_wait() in pci_power_up() before accessing the configuration
space of a device for the first time in the system resume sequence.
This is  to accommodate devices (Ex:- Intel 750 NVMe) that respond with CRS
status while they get ready for configuration space access and before they
finally start responding with proper values.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 71b45ce73bf6..7672b9a44bac 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1086,6 +1086,13 @@ int pci_power_up(struct pci_dev *dev)
 		pci_wakeup_bus(dev->subordinate);
 	}
 
+	/*
+	 * Wait for those devices (Ex: Intel 750 NVMe) that are not ready yet
+	 * and responding with CRS statuses for the configuration space
+	 * requests.
+	 */
+	pci_dev_wait(dev, "Switch to D0", PCIE_RESET_READY_POLL_MS);
+
 	return pci_raw_set_power_state(dev, PCI_D0);
 }
 
-- 
2.17.1