[PATCH V12 1/5] PCI: Don't ignore valid response before CRS timeout

[PATCH V12 1/5] PCI: Don't ignore valid response before CRS timeout

[Sinan Kaya]

From: Bjorn Helgaas <bhelgaas@google.com>

While waiting for a device to become ready (i.e., to return a non-CRS
completion to a read of its Vendor ID), if we got a valid response to the
very last read before timing out, we printed a warning and gave up on the
device even though it was actually ready.

For a typical 60s timeout, we wait about 65s (it's not exact because of the
exponential backoff), but we treated devices that became ready between 33s
and 65s as though they failed.

Move the Device ID read later so we check whether the device is ready
immediately, before checking for a timeout.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
[okaya: reorder reads so that we check device presence after sleep]
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/probe.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c31310d..2849e0e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1847,17 +1847,18 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
 		if (!crs_timeout)
 			return false;
 
-		msleep(delay);
-		delay *= 2;
-		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
-			return false;
-		/* Card hasn't responded in 60 seconds?  Must be stuck. */
 		if (delay > crs_timeout) {
 			printk(KERN_WARNING "pci %04x:%02x:%02x.%d: not responding\n",
 			       pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
 			       PCI_FUNC(devfn));
 			return false;
 		}
+
+		msleep(delay);
+		delay *= 2;
+
+		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
+			return false;
 	}
 
 	return true;
-- 
1.9.1

[PATCH V12 2/5] PCI: add pci_bus_crs_visibility_pending() function to detect CRS response

[Sinan Kaya]

We need a wrapper function to determine when a particular device is
returning Configuration Request Retry Status (CRS) response. This will only
happen if the root port supports CRS visibility.

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

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 22e0617..7fa583a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -235,6 +235,10 @@ enum pci_bar_type {
 	pci_bar_mem64,		/* A 64-bit memory BAR */
 };
 
+static inline bool pci_bus_crs_visibility_pending(u32 l)
+{
+	return (l & 0xffff) == 0x0001;
+}
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
-- 
1.9.1

[PATCH V12 3/5] PCI: Factor out pci_bus_wait_crs()

[Sinan Kaya]

Configuration Request Retry Status (CRS) was previously hidden inside
pci_bus_read_dev_vendor_id().  We want to add support for CRS in other
situations, such as waiting for a device to become ready after a Function
Level Reset.

Move CRS handling into pci_bus_wait_crs() so it can be called from other
places.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/pci/pci.h   |  1 +
 drivers/pci/probe.c | 46 ++++++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 7fa583a..56d2515 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -239,6 +239,7 @@ static inline bool pci_bus_crs_visibility_pending(u32 l)
 {
 	return (l & 0xffff) == 0x0001;
 }
+bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 l, int timeout);
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2849e0e..93b89dd 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1824,30 +1824,23 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_alloc_dev);
 
-bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
-				int crs_timeout)
+bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 l, int timeout)
 {
 	int delay = 1;
 
-	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
-		return false;
+	if ((l & 0xffff) != 0x0001)
+		return true;	/* not a CRS completion */
 
-	/* some broken boards return 0 or ~0 if a slot is empty: */
-	if (*l == 0xffffffff || *l == 0x00000000 ||
-	    *l == 0x0000ffff || *l == 0xffff0000)
-		return false;
+	if (!timeout)
+		return false;	/* CRS, but caller doesn't want to wait */
 
 	/*
-	 * Configuration Request Retry Status.  Some root ports return the
-	 * actual device ID instead of the synthetic ID (0xFFFF) required
-	 * by the PCIe spec.  Ignore the device ID and only check for
-	 * (vendor id == 1).
+	 * We got the reserved Vendor ID that indicates a completion with
+	 * Configuration Request Retry Status (CRS).  Retry until we get a
+	 * valid Vendor ID or we time out.
 	 */
-	while ((*l & 0xffff) == 0x0001) {
-		if (!crs_timeout)
-			return false;
-
-		if (delay > crs_timeout) {
+	while ((l & 0xffff) == 0x0001) {
+		if (delay > timeout) {
 			printk(KERN_WARNING "pci %04x:%02x:%02x.%d: not responding\n",
 			       pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
 			       PCI_FUNC(devfn));
@@ -1857,12 +1850,29 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
 		msleep(delay);
 		delay *= 2;
 
-		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
+		if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &l))
 			return false;
 	}
 
 	return true;
 }
+
+bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
+				int timeout)
+{
+	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l))
+		return false;
+
+	/* some broken boards return 0 or ~0 if a slot is empty: */
+	if (*l == 0xffffffff || *l == 0x00000000 ||
+	    *l == 0x0000ffff || *l == 0xffff0000)
+		return false;
+
+	if (pci_bus_crs_visibility_pending(*l))
+		return pci_bus_wait_crs(bus, devfn, *l, timeout);
+
+	return true;
+}
 EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
 
 /*
-- 
1.9.1

[PATCH V12 4/5] PCI: Handle CRS ("device not ready") returned by device after FLR

[Sinan Kaya]

Sporadic reset issues have been observed with Intel 750 NVMe drive while
assigning the physical function to the guest machine.  The sequence of
events observed is as follows:

  - perform a Function Level Reset (FLR)
  - sleep up to 1000ms total
  - read ~0 from PCI_COMMAND
  - warn that the device didn't return from FLR
  - touch the device before it's ready
  - device drops config writes when we restore register settings
  - incomplete register restore leaves device in inconsistent state
  - device probe fails because device is in inconsistent state

After reset, an endpoint may respond to config requests with Configuration
Request Retry Status (CRS) to indicate that it is not ready to accept new
requests.  See PCIe r3.1, sec 2.3.1 and 6.6.2.

After an FLR, read the Vendor ID and use pci_bus_wait_crs() to wait for a
value that indicates the device is ready only if CRS visibility is
supported and device is responding with 0x0001.

If pci_bus_wait_crs() fails, i.e., the device has responded with CRS status
for at least the timeout interval, fall back to the old behavior of reading
the Command register until it is not ~0.

Also increase the timeout value from 1 second to 60 seconds to have
consistent behavior for root ports that do and do not support CRS
visibility.

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index af0cc34..5980ab3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3819,19 +3819,35 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
  */
 static void pci_flr_wait(struct pci_dev *dev)
 {
+	int timeout_ms = 60000;
 	int i = 0;
 	u32 id;
 
+	/*
+	 * Per PCIe r3.1, sec 6.6.2, the device should finish FLR within
+	 * 100ms, but even after that, it may respond to config requests
+	 * with CRS status if it requires more time.
+	 */
+	msleep(100);
+
+	if (pci_bus_read_config_dword(dev->bus, dev->devfn, PCI_VENDOR_ID, &id))
+		return;
+
+	if (pci_bus_crs_visibility_pending(id) &&
+		pci_bus_wait_crs(dev->bus, dev->devfn, id, timeout_ms))
+		return;
+
+	timeout_ms -= 100;
 	do {
 		msleep(100);
 		pci_read_config_dword(dev, PCI_COMMAND, &id);
-	} while (i++ < 10 && id == ~0);
+	} while (i++ < (timeout_ms / 100) && id == ~0);
 
 	if (id == ~0)
 		dev_warn(&dev->dev, "Failed to return from FLR\n");
 	else if (i > 1)
 		dev_info(&dev->dev, "Required additional %dms to return from FLR\n",
-			 (i - 1) * 100);
+			 i * 100);
 }
 
 /**
-- 
1.9.1

[PATCH V12 4/5] PCI: Handle CRS ("device not ready") returned by device after FLR

[Bjorn Helgaas]

On Wed, Aug 23, 2017 at 12:56:10AM -0400, Sinan Kaya wrote:
> Sporadic reset issues have been observed with Intel 750 NVMe drive while
> assigning the physical function to the guest machine.  The sequence of
> events observed is as follows:
> 
>   - perform a Function Level Reset (FLR)
>   - sleep up to 1000ms total
>   - read ~0 from PCI_COMMAND
>   - warn that the device didn't return from FLR
>   - touch the device before it's ready
>   - device drops config writes when we restore register settings
>   - incomplete register restore leaves device in inconsistent state
>   - device probe fails because device is in inconsistent state
> 
> After reset, an endpoint may respond to config requests with Configuration
> Request Retry Status (CRS) to indicate that it is not ready to accept new
> requests.  See PCIe r3.1, sec 2.3.1 and 6.6.2.
> 
> After an FLR, read the Vendor ID and use pci_bus_wait_crs() to wait for a
> value that indicates the device is ready only if CRS visibility is
> supported and device is responding with 0x0001.
> 
> If pci_bus_wait_crs() fails, i.e., the device has responded with CRS status
> for at least the timeout interval, fall back to the old behavior of reading
> the Command register until it is not ~0.
> 
> Also increase the timeout value from 1 second to 60 seconds to have
> consistent behavior for root ports that do and do not support CRS
> visibility.

So, after all this work, I bet you a nickel that merely increasing the
timeout from 1 sec to 60 sec (while keeping just the original PCI_COMMAND
loop) would be sufficient to fix this problem.

Reading PCI_COMMAND will cause CRS completions just like reading
PCI_VENDOR_ID will.  The only difference is that there's no CRS SV
handling, and the Root Port will synthesize ~0 data when it stops retrying
the read.

If we increase the timeout, is there still value in adding the
pci_bus_wait_crs() stuff?  I'm not sure there is.

> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index af0cc34..5980ab3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3819,19 +3819,35 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
>   */
>  static void pci_flr_wait(struct pci_dev *dev)
>  {
> +	int timeout_ms = 60000;
>  	int i = 0;
>  	u32 id;
>  
> +	/*
> +	 * Per PCIe r3.1, sec 6.6.2, the device should finish FLR within
> +	 * 100ms, but even after that, it may respond to config requests
> +	 * with CRS status if it requires more time.
> +	 */
> +	msleep(100);
> +
> +	if (pci_bus_read_config_dword(dev->bus, dev->devfn, PCI_VENDOR_ID, &id))
> +		return;
> +
> +	if (pci_bus_crs_visibility_pending(id) &&
> +		pci_bus_wait_crs(dev->bus, dev->devfn, id, timeout_ms))
> +		return;
> +
> +	timeout_ms -= 100;
>  	do {
>  		msleep(100);
>  		pci_read_config_dword(dev, PCI_COMMAND, &id);
> -	} while (i++ < 10 && id == ~0);
> +	} while (i++ < (timeout_ms / 100) && id == ~0);
>  
>  	if (id == ~0)
>  		dev_warn(&dev->dev, "Failed to return from FLR\n");
>  	else if (i > 1)
>  		dev_info(&dev->dev, "Required additional %dms to return from FLR\n",
> -			 (i - 1) * 100);
> +			 i * 100);
>  }
>  
>  /**
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V12 4/5] PCI: Handle CRS ("device not ready") returned by device after FLR

[Sinan Kaya]

On 8/23/2017 5:38 PM, Bjorn Helgaas wrote:
> If we increase the timeout, is there still value in adding the
> pci_bus_wait_crs() stuff?  I'm not sure there is.

I agree increasing the timeout is more than enough for FLR case. 

However, I was considering the wait and pending functions as a utility
that I can reuse towards fixing CRS in other conditions like secondary
bus reset and potentially PM.

I'm planning to have a CRS session in the Linux Plumbers Conference 
to talk about CRS use cases. 

I was going to follow up this series with secondary bus reset next once
this goes in. 

I'm unable to test PM. I can't promise how I fix/test it.

-- 
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 V12 4/5] PCI: Handle CRS ("device not ready") returned by device after FLR

[Bjorn Helgaas]

On Wed, Aug 23, 2017 at 05:51:19PM -0400, Sinan Kaya wrote:
> On 8/23/2017 5:38 PM, Bjorn Helgaas wrote:
> > If we increase the timeout, is there still value in adding the
> > pci_bus_wait_crs() stuff?  I'm not sure there is.
> 
> I agree increasing the timeout is more than enough for FLR case. 

If that's the case, I think there's no value in adding additional
complexity to the path.  If we increase the timeout, we might pretty
it up a little along the lines of the patch below.

> However, I was considering the wait and pending functions as a utility
> that I can reuse towards fixing CRS in other conditions like secondary
> bus reset and potentially PM.
> 
> I'm planning to have a CRS session in the Linux Plumbers Conference 
> to talk about CRS use cases. 

Great idea.  I'm kind of confused on its value in general myself.  And
there's now a new mechanism (Function Readiness Status) that I don't
think we have any support for at all.

> I was going to follow up this series with secondary bus reset next once
> this goes in. 

I think I'm OK with everything except the pci_flr_wait() change.  The
rest of it makes sense (although I don't think there are any users
outside probe.c, so maybe should be static for now).

> I'm unable to test PM. I can't promise how I fix/test it.


diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index af0cc3456dc1..b04c99e60b77 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3811,27 +3811,50 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_wait_for_pending_transaction);
 
-/*
- * We should only need to wait 100ms after FLR, but some devices take longer.
- * Wait for up to 1000ms for config space to return something other than -1.
- * Intel IGD requires this when an LCD panel is attached.  We read the 2nd
- * dword because VFs don't implement the 1st dword.
- */
 static void pci_flr_wait(struct pci_dev *dev)
 {
-	int i = 0;
+	int delay = 1, timeout = 60000;
 	u32 id;
 
-	do {
-		msleep(100);
+	/*
+	 * 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
+	 * 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) {
+			dev_warn(&dev->dev, "not ready %dms after FLR; giving up\n",
+				 delay - 1);
+			return;
+		}
+
+		if (delay > 1000)
+			dev_info(&dev->dev, "not ready %dms after FLR; waiting\n",
+				 delay - 1);
+
+		msleep(delay);
+		delay *= 2;
+
 		pci_read_config_dword(dev, PCI_COMMAND, &id);
-	} while (i++ < 10 && id == ~0);
+	}
 
-	if (id == ~0)
-		dev_warn(&dev->dev, "Failed to return from FLR\n");
-	else if (i > 1)
-		dev_info(&dev->dev, "Required additional %dms to return from FLR\n",
-			 (i - 1) * 100);
+	if (delay > 1000)
+		dev_info(&dev->dev, "ready %dms after FLR\n", delay - 1);
 }
 
 /**

[PATCH V12 5/5] PCI: Warn periodically while waiting for device to become ready

[Sinan Kaya]

Add a print statement in pci_bus_wait_crs() so that user observes the
progress of device polling instead of silently waiting for timeout to be
reached.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
[bhelgaas: check for timeout first so we don't print "waiting, giving up",
always print time we've slept (not the actual timeout, print a "ready"
message if we've printed a "waiting" message]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 93b89dd..4b30df7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1841,11 +1841,16 @@ bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 l, int timeout)
 	 */
 	while ((l & 0xffff) == 0x0001) {
 		if (delay > timeout) {
-			printk(KERN_WARNING "pci %04x:%02x:%02x.%d: not responding\n",
-			       pci_domain_nr(bus), bus->number, PCI_SLOT(devfn),
-			       PCI_FUNC(devfn));
+			pr_warn("pci %04x:%02x:%02x.%d: not ready after %dms; giving up\n",
+				pci_domain_nr(bus), bus->number,
+				PCI_SLOT(devfn), PCI_FUNC(devfn), delay - 1);
+
 			return false;
 		}
+		if (delay >= 1000)
+			pr_info("pci %04x:%02x:%02x.%d: not ready after %dms; waiting\n",
+				pci_domain_nr(bus), bus->number,
+				PCI_SLOT(devfn), PCI_FUNC(devfn), delay - 1);
 
 		msleep(delay);
 		delay *= 2;
@@ -1854,6 +1859,11 @@ bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 l, int timeout)
 			return false;
 	}
 
+	if (delay >= 1000)
+		pr_info("pci %04x:%02x:%02x.%d: ready after %dms\n",
+			pci_domain_nr(bus), bus->number,
+			PCI_SLOT(devfn), PCI_FUNC(devfn), delay - 1);
+
 	return true;
 }
 
-- 
1.9.1