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@xxxxxxxxxx>
> Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
> ---
> 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
>
