Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1718037 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V12 4/5] PCI: Handle CRS ("device not ready") returned by device after FLR Sinan Kaya <okaya@codeaurora.org> - 2017-08-23 07:00 +0200
    Re: [PATCH V12 4/5] PCI: Handle CRS ("device not ready") returned by  device after FLR Bjorn Helgaas <helgaas@kernel.org> - 2017-08-23 23:40 +0200
      Re: [PATCH V12 4/5] PCI: Handle CRS ("device not ready") returned by  device after FLR Sinan Kaya <okaya@codeaurora.org> - 2017-08-24 00:00 +0200
        Re: [PATCH V12 4/5] PCI: Handle CRS ("device not ready") returned by  device after FLR Bjorn Helgaas <helgaas@kernel.org> - 2017-08-24 00:30 +0200

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

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

[toc] | [next] | [standalone]

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

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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);
 }
 
 /**

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web