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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>