[PATCH v2 0/2] PCI/PM: Resume/reset wait time change

[PATCH v2 0/2] PCI/PM: Resume/reset wait time change

[Mika Westerberg]

Hi all,

This series first increases the time we wait on resume path to
accommondate certain device, as reported in [1], and then "optimizes"
the timeout for slow links to avoid too long waits if a device is
disconnected during suspend.

Previous version can be found here:

  https://lore.kernel.org/linux-pci/20230321095031.65709-1-mika.westerberg@linux.intel.com/

Changes from the previous version:

  * Split the patch into two: one that increases the resume timeout (on
    all links, I was not able to figure out a simple way to limit this
    only for the fast links) and the one that decreases the timeout on
    slow links.

  * Use dev->link_active_reporting instead of speed to figure out slow
    vs. fast links.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=216728

Mika Westerberg (2):
  PCI/PM: Increase wait time after resume
  PCI/PM: Decrease wait time for devices behind slow links

 drivers/pci/pci-driver.c |  2 +-
 drivers/pci/pci.c        | 42 ++++++++++++++++++++++++++--------------
 drivers/pci/pci.h        | 16 +--------------
 drivers/pci/pcie/dpc.c   |  3 +--
 4 files changed, 30 insertions(+), 33 deletions(-)

-- 
2.39.2

[PATCH v2 1/2] PCI/PM: Increase wait time after resume

[Mika Westerberg]

The PCIe spec prescribes that a device may take up to 1 second to
recover from reset and this same delay is prescribed when coming out of
D3cold (as that involves reset too). The device may extend this 1 second
delay through Request Retry Status completions and we accommondate for
that in Linux with 60 second cap, only in reset code path, not in resume
code path.

However, a device has surfaced, namely Intel Titan Ridge xHCI, which
requires longer delay also in the resume code path. For this reason make
the resume code path to use this same extended delay than with the reset
path.

Reported-by: Chris Chiu <chris.chiu@canonical.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216728
Cc: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci-driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 57ddcc59af30..6b5b2a818e65 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -572,7 +572,8 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
 
 static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev)
 {
-	pci_bridge_wait_for_secondary_bus(pci_dev, "resume", PCI_RESET_WAIT);
+	pci_bridge_wait_for_secondary_bus(pci_dev, "resume",
+					  PCIE_RESET_READY_POLL_MS);
 	/*
 	 * When powering on a bridge from D3cold, the whole hierarchy may be
 	 * powered on into D0uninitialized state, resume them to give them a
-- 
2.39.2

[PATCH v2 2/2] PCI/PM: Decrease wait time for devices behind slow links

[Mika Westerberg]

In order speed up reset and resume time of devices behind slow links,
decrease the wait time to 1s. This should give enough time for them to
respond. While doing this, instead of looking at the speed we check if
the port supports active link reporting. If it does we can wait longer
but if it does not we wait for the 1s prescribed in the PCIe spec.

Since pci_bridge_wait_for_secondary_bus() handles all the delays
internally now move the wait constants from drivers/pci/pci.h into
drivers/pci/pci.c.

Cc: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci-driver.c |  3 +--
 drivers/pci/pci.c        | 42 ++++++++++++++++++++++++++--------------
 drivers/pci/pci.h        | 16 +--------------
 drivers/pci/pcie/dpc.c   |  3 +--
 4 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 6b5b2a818e65..1a5ee65edb10 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -572,8 +572,7 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
 
 static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev)
 {
-	pci_bridge_wait_for_secondary_bus(pci_dev, "resume",
-					  PCIE_RESET_READY_POLL_MS);
+	pci_bridge_wait_for_secondary_bus(pci_dev, "resume");
 	/*
 	 * When powering on a bridge from D3cold, the whole hierarchy may be
 	 * powered on into D0uninitialized state, resume them to give them a
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7a67611dc5f4..5302d900dbe7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -64,6 +64,19 @@ struct pci_pme_device {
 
 #define PME_TIMEOUT 1000 /* How long between PME checks */
 
+/*
+ * Following exit from Conventional Reset, devices must be ready within 1 sec
+ * (PCIe r6.0 sec 6.6.1).  A D3cold to D0 transition implies a Conventional
+ * Reset (PCIe r6.0 sec 5.8).
+ */
+#define PCI_RESET_WAIT		1000	/* msec */
+/*
+ * Devices may extend the 1 sec period through Request Retry Status completions
+ * (PCIe r6.0 sec 2.3.1).  The spec does not provide an upper limit, but 60 sec
+ * ought to be enough for any device to become responsive.
+ */
+#define PCIE_RESET_READY_POLL_MS 60000	/* msec */
+
 static void pci_dev_d3_sleep(struct pci_dev *dev)
 {
 	unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay);
@@ -4939,7 +4952,6 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
  * pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible
  * @dev: PCI bridge
  * @reset_type: reset type in human-readable form
- * @timeout: maximum time to wait for devices on secondary bus (milliseconds)
  *
  * Handle necessary delays before access to the devices on the secondary
  * side of the bridge are permitted after D3cold to D0 transition
@@ -4952,8 +4964,7 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
  * Return 0 on success or -ENOTTY if the first device on the secondary bus
  * failed to become accessible.
  */
-int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
-				      int timeout)
+int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
 {
 	struct pci_dev *child;
 	int delay;
@@ -5018,20 +5029,22 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
 	if (!pcie_downstream_port(dev))
 		return 0;
 
-	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
+	if (!dev->link_active_reporting) {
 		pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
 		msleep(delay);
-	} else {
-		pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
-			delay);
-		if (!pcie_wait_for_link_delay(dev, true, delay)) {
-			/* Did not train, no need to wait any further */
-			pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
-			return -ENOTTY;
-		}
+
+		return pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay);
+	}
+
+	pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
+		delay);
+	if (!pcie_wait_for_link_delay(dev, true, delay)) {
+		/* Did not train, no need to wait any further */
+		pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
+		return -ENOTTY;
 	}
 
-	return pci_dev_wait(child, reset_type, timeout - delay);
+	return pci_dev_wait(child, reset_type, PCIE_RESET_READY_POLL_MS - delay);
 }
 
 void pci_reset_secondary_bus(struct pci_dev *dev)
@@ -5068,8 +5081,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
 {
 	pcibios_reset_secondary_bus(dev);
 
-	return pci_bridge_wait_for_secondary_bus(dev, "bus reset",
-						 PCIE_RESET_READY_POLL_MS);
+	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
 }
 EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d2c08670a20e..f2d3aeab91f4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -64,19 +64,6 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
 #define PCI_PM_D3HOT_WAIT       10	/* msec */
 #define PCI_PM_D3COLD_WAIT      100	/* msec */
 
-/*
- * Following exit from Conventional Reset, devices must be ready within 1 sec
- * (PCIe r6.0 sec 6.6.1).  A D3cold to D0 transition implies a Conventional
- * Reset (PCIe r6.0 sec 5.8).
- */
-#define PCI_RESET_WAIT		1000	/* msec */
-/*
- * Devices may extend the 1 sec period through Request Retry Status completions
- * (PCIe r6.0 sec 2.3.1).  The spec does not provide an upper limit, but 60 sec
- * ought to be enough for any device to become responsive.
- */
-#define PCIE_RESET_READY_POLL_MS 60000	/* msec */
-
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_refresh_power_state(struct pci_dev *dev);
 int pci_power_up(struct pci_dev *dev);
@@ -100,8 +87,7 @@ void pci_msix_init(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
 void pci_bridge_reconfigure_ltr(struct pci_dev *dev);
-int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
-				      int timeout);
+int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index a5d7c69b764e..3ceed8e3de41 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -170,8 +170,7 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 			      PCI_EXP_DPC_STATUS_TRIGGER);
 
-	if (pci_bridge_wait_for_secondary_bus(pdev, "DPC",
-					      PCIE_RESET_READY_POLL_MS)) {
+	if (pci_bridge_wait_for_secondary_bus(pdev, "DPC")) {
 		clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
 		ret = PCI_ERS_RESULT_DISCONNECT;
 	} else {
-- 
2.39.2

Re: [PATCH v2 2/2] PCI/PM: Decrease wait time for devices behind slow links

[Bjorn Helgaas]

Hi Mika,

I need some help because I have a hard time applying sec 6.6.1.

On Tue, Apr 04, 2023 at 08:27:14AM +0300, Mika Westerberg wrote:
> In order speed up reset and resume time of devices behind slow links,
> decrease the wait time to 1s. This should give enough time for them to
> respond.

Is there some spec language behind this?  In sec 6.6.1, I see that all
devices "must be able to receive a Configuration Request and return a
Successful Completion".

A preceding rule says devices with slow links must enter LTSSM Detect
within 20ms, but I don't see a direct connection from that to a
shorter wait time.

> While doing this, instead of looking at the speed we check if
> the port supports active link reporting.

Why check dev->link_active_reporting (i.e., PCI_EXP_LNKCAP_DLLLARC)
instead of the link speed described by the spec?

DLLLARC is required for fast links, but it's not prohibited for slower
links and it's *required* for hotplug ports with slow links, so
dev->link_active_reporting is not completely determined by link speed.

IIUC, the current code basically has these cases:

  1) All devices on secondary bus have zero D3cold delay:
       return immediately; no delay at all

  2) Non-PCIe bridge:
       sleep 1000ms
       sleep  100ms (typical, depends on downstream devices)

  3) Speed <= 5 GT/s:
       sleep 100ms (typical)
       sleep up to 59.9s (typical) waiting for valid config read

  4) Speed > 5 GT/s (DLLLARC required):
       sleep 20ms
       sleep up to 1000ms waiting for DLLLA
       sleep 100ms (typical)
       sleep up to 59.9s (typical) waiting for valid config read

This patch changes cases 3) and 4) to:

  3) DLLLARC not supported:
       sleep 100ms (typical)
       sleep up to 1.0s (typical) waiting for valid config read

  4) DLLLARC supported:
       no change in wait times, ~60s total

And testing dev->link_active_reporting instead of speed means slow
hotplug ports (and possibly other slow ports that implement DLLLARC)
that previously were in case 3) will now be in case 4).

> If it does we can wait longer
> but if it does not we wait for the 1s prescribed in the PCIe spec.
> 
> Since pci_bridge_wait_for_secondary_bus() handles all the delays
> internally now move the wait constants from drivers/pci/pci.h into
> drivers/pci/pci.c.
> 
> Cc: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pci-driver.c |  3 +--
>  drivers/pci/pci.c        | 42 ++++++++++++++++++++++++++--------------
>  drivers/pci/pci.h        | 16 +--------------
>  drivers/pci/pcie/dpc.c   |  3 +--
>  4 files changed, 30 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 6b5b2a818e65..1a5ee65edb10 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -572,8 +572,7 @@ static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
>  
>  static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev)
>  {
> -	pci_bridge_wait_for_secondary_bus(pci_dev, "resume",
> -					  PCIE_RESET_READY_POLL_MS);
> +	pci_bridge_wait_for_secondary_bus(pci_dev, "resume");
>  	/*
>  	 * When powering on a bridge from D3cold, the whole hierarchy may be
>  	 * powered on into D0uninitialized state, resume them to give them a
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7a67611dc5f4..5302d900dbe7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -64,6 +64,19 @@ struct pci_pme_device {
>  
>  #define PME_TIMEOUT 1000 /* How long between PME checks */
>  
> +/*
> + * Following exit from Conventional Reset, devices must be ready within 1 sec
> + * (PCIe r6.0 sec 6.6.1).  A D3cold to D0 transition implies a Conventional
> + * Reset (PCIe r6.0 sec 5.8).
> + */
> +#define PCI_RESET_WAIT		1000	/* msec */
> +/*
> + * Devices may extend the 1 sec period through Request Retry Status completions
> + * (PCIe r6.0 sec 2.3.1).  The spec does not provide an upper limit, but 60 sec
> + * ought to be enough for any device to become responsive.
> + */
> +#define PCIE_RESET_READY_POLL_MS 60000	/* msec */
> +
>  static void pci_dev_d3_sleep(struct pci_dev *dev)
>  {
>  	unsigned int delay_ms = max(dev->d3hot_delay, pci_pm_d3hot_delay);
> @@ -4939,7 +4952,6 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
>   * pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible
>   * @dev: PCI bridge
>   * @reset_type: reset type in human-readable form
> - * @timeout: maximum time to wait for devices on secondary bus (milliseconds)
>   *
>   * Handle necessary delays before access to the devices on the secondary
>   * side of the bridge are permitted after D3cold to D0 transition
> @@ -4952,8 +4964,7 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
>   * Return 0 on success or -ENOTTY if the first device on the secondary bus
>   * failed to become accessible.
>   */
> -int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
> -				      int timeout)
> +int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  {
>  	struct pci_dev *child;
>  	int delay;
> @@ -5018,20 +5029,22 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
>  	if (!pcie_downstream_port(dev))
>  		return 0;
>  
> -	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> +	if (!dev->link_active_reporting) {

>  		pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
>  		msleep(delay);
> -	} else {
> -		pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
> -			delay);
> -		if (!pcie_wait_for_link_delay(dev, true, delay)) {
> -			/* Did not train, no need to wait any further */
> -			pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
> -			return -ENOTTY;
> -		}
> +
> +		return pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay);
> +	}
> +
> +	pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
> +		delay);
> +	if (!pcie_wait_for_link_delay(dev, true, delay)) {
> +		/* Did not train, no need to wait any further */
> +		pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
> +		return -ENOTTY;
>  	}
>  
> -	return pci_dev_wait(child, reset_type, timeout - delay);
> +	return pci_dev_wait(child, reset_type, PCIE_RESET_READY_POLL_MS - delay);
>  }
>  
>  void pci_reset_secondary_bus(struct pci_dev *dev)
> @@ -5068,8 +5081,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
>  {
>  	pcibios_reset_secondary_bus(dev);
>  
> -	return pci_bridge_wait_for_secondary_bus(dev, "bus reset",
> -						 PCIE_RESET_READY_POLL_MS);
> +	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
>  }
>  EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index d2c08670a20e..f2d3aeab91f4 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -64,19 +64,6 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
>  #define PCI_PM_D3HOT_WAIT       10	/* msec */
>  #define PCI_PM_D3COLD_WAIT      100	/* msec */
>  
> -/*
> - * Following exit from Conventional Reset, devices must be ready within 1 sec
> - * (PCIe r6.0 sec 6.6.1).  A D3cold to D0 transition implies a Conventional
> - * Reset (PCIe r6.0 sec 5.8).
> - */
> -#define PCI_RESET_WAIT		1000	/* msec */
> -/*
> - * Devices may extend the 1 sec period through Request Retry Status completions
> - * (PCIe r6.0 sec 2.3.1).  The spec does not provide an upper limit, but 60 sec
> - * ought to be enough for any device to become responsive.
> - */
> -#define PCIE_RESET_READY_POLL_MS 60000	/* msec */
> -
>  void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>  void pci_refresh_power_state(struct pci_dev *dev);
>  int pci_power_up(struct pci_dev *dev);
> @@ -100,8 +87,7 @@ void pci_msix_init(struct pci_dev *dev);
>  bool pci_bridge_d3_possible(struct pci_dev *dev);
>  void pci_bridge_d3_update(struct pci_dev *dev);
>  void pci_bridge_reconfigure_ltr(struct pci_dev *dev);
> -int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type,
> -				      int timeout);
> +int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
>  
>  static inline void pci_wakeup_event(struct pci_dev *dev)
>  {
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index a5d7c69b764e..3ceed8e3de41 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -170,8 +170,7 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
>  	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
>  			      PCI_EXP_DPC_STATUS_TRIGGER);
>  
> -	if (pci_bridge_wait_for_secondary_bus(pdev, "DPC",
> -					      PCIE_RESET_READY_POLL_MS)) {
> +	if (pci_bridge_wait_for_secondary_bus(pdev, "DPC")) {
>  		clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
>  		ret = PCI_ERS_RESULT_DISCONNECT;
>  	} else {
> -- 
> 2.39.2
> 

Re: [PATCH v2 2/2] PCI/PM: Decrease wait time for devices behind slow links

[Mika Westerberg]

Hi,

On Tue, Apr 04, 2023 at 04:36:55PM -0500, Bjorn Helgaas wrote:
> Hi Mika,
> 
> I need some help because I have a hard time applying sec 6.6.1.
> 
> On Tue, Apr 04, 2023 at 08:27:14AM +0300, Mika Westerberg wrote:
> > In order speed up reset and resume time of devices behind slow links,
> > decrease the wait time to 1s. This should give enough time for them to
> > respond.
> 
> Is there some spec language behind this?  In sec 6.6.1, I see that all
> devices "must be able to receive a Configuration Request and return a
> Successful Completion".
> 
> A preceding rule says devices with slow links must enter LTSSM Detect
> within 20ms, but I don't see a direct connection from that to a
> shorter wait time.

I think this (PCIe 5.0 p. 553):

"Following a Conventional Reset of a device, within 1.0 s the device
 must be able to receive a Configuration Request and return a Successful
 Completion if the Request is valid."

> > While doing this, instead of looking at the speed we check if
> > the port supports active link reporting.
> 
> Why check dev->link_active_reporting (i.e., PCI_EXP_LNKCAP_DLLLARC)
> instead of the link speed described by the spec?

This is what Sathyanarayanan suggested in the previous version comments.

> DLLLARC is required for fast links, but it's not prohibited for slower
> links and it's *required* for hotplug ports with slow links, so
> dev->link_active_reporting is not completely determined by link speed.
> 
> IIUC, the current code basically has these cases:
> 
>   1) All devices on secondary bus have zero D3cold delay:
>        return immediately; no delay at all
> 
>   2) Non-PCIe bridge:
>        sleep 1000ms
>        sleep  100ms (typical, depends on downstream devices)
> 
>   3) Speed <= 5 GT/s:
>        sleep 100ms (typical)
>        sleep up to 59.9s (typical) waiting for valid config read
> 
>   4) Speed > 5 GT/s (DLLLARC required):
>        sleep 20ms
>        sleep up to 1000ms waiting for DLLLA
>        sleep 100ms (typical)
>        sleep up to 59.9s (typical) waiting for valid config read
> 
> This patch changes cases 3) and 4) to:
> 
>   3) DLLLARC not supported:
>        sleep 100ms (typical)
>        sleep up to 1.0s (typical) waiting for valid config read
> 
>   4) DLLLARC supported:
>        no change in wait times, ~60s total
> 
> And testing dev->link_active_reporting instead of speed means slow
> hotplug ports (and possibly other slow ports that implement DLLLARC)
> that previously were in case 3) will now be in case 4).

Yes, and we do that because if the device gets unplugged while we were
in susppend we don't want to wait for the total 60s for it to become
ready. That's what the DLLLARC can tell us (for ports that support it).
For the ports that do not we want to give the device some time but not
to wait for that 60s so we wait for the 1s as the "minimum" requirement
from the spec before it can be determined "broken".

Re: [PATCH v2 2/2] PCI/PM: Decrease wait time for devices behind slow links

[Bjorn Helgaas]

On Wed, Apr 05, 2023 at 12:39:29PM +0300, Mika Westerberg wrote:
> On Tue, Apr 04, 2023 at 04:36:55PM -0500, Bjorn Helgaas wrote:
> > On Tue, Apr 04, 2023 at 08:27:14AM +0300, Mika Westerberg wrote:
> > > In order speed up reset and resume time of devices behind slow links,
> > > decrease the wait time to 1s. This should give enough time for them to
> > > respond.
> > 
> > Is there some spec language behind this?  In sec 6.6.1, I see that all
> > devices "must be able to receive a Configuration Request and return a
> > Successful Completion".
> > 
> > A preceding rule says devices with slow links must enter LTSSM Detect
> > within 20ms, but I don't see a direct connection from that to a
> > shorter wait time.
> 
> I think this (PCIe 5.0 p. 553):
> 
> "Following a Conventional Reset of a device, within 1.0 s the device
>  must be able to receive a Configuration Request and return a Successful
>  Completion if the Request is valid."

Right, I think this applies to all devices, regardless of link speed.

> > > While doing this, instead of looking at the speed we check if
> > > the port supports active link reporting.
> > 
> > Why check dev->link_active_reporting (i.e., PCI_EXP_LNKCAP_DLLLARC)
> > instead of the link speed described by the spec?
> 
> This is what Sathyanarayanan suggested in the previous version comments.
> 
> > DLLLARC is required for fast links, but it's not prohibited for slower
> > links and it's *required* for hotplug ports with slow links, so
> > dev->link_active_reporting is not completely determined by link speed.
> > 
> > IIUC, the current code basically has these cases:
> > 
> >   1) All devices on secondary bus have zero D3cold delay:
> >        return immediately; no delay at all
> > 
> >   2) Non-PCIe bridge:
> >        sleep 1000ms
> >        sleep  100ms (typical, depends on downstream devices)
> > 
> >   3) Speed <= 5 GT/s:
> >        sleep 100ms (typical)
> >        sleep up to 59.9s (typical) waiting for valid config read
> > 
> >   4) Speed > 5 GT/s (DLLLARC required):
> >        sleep 20ms
> >        sleep up to 1000ms waiting for DLLLA
> >        sleep 100ms (typical)
> >        sleep up to 59.9s (typical) waiting for valid config read
> > 
> > This patch changes cases 3) and 4) to:
> > 
> >   3) DLLLARC not supported:
> >        sleep 100ms (typical)
> >        sleep up to 1.0s (typical) waiting for valid config read
> > 
> >   4) DLLLARC supported:
> >        no change in wait times, ~60s total
> > 
> > And testing dev->link_active_reporting instead of speed means slow
> > hotplug ports (and possibly other slow ports that implement DLLLARC)
> > that previously were in case 3) will now be in case 4).
> 
> Yes, and we do that because if the device gets unplugged while we were
> in susppend we don't want to wait for the total 60s for it to become
> ready. That's what the DLLLARC can tell us (for ports that support it).
> For the ports that do not we want to give the device some time but not
> to wait for that 60s so we wait for the 1s as the "minimum" requirement
> from the spec before it can be determined "broken".

Ah, thanks, I think I see what you're doing here.

I think what makes this confusing is that there are several reasons
for waiting, and they're mixed together and done at various places:

  1) We want to avoid PCIe protocol errors, e.g., Completion Timeout,
  and the spec specifies how long to wait before sending a config
  request to a device below the port.

  2) We want to wait for slow devices to finish internal
  initialization even after it can respond to config requests, e.g.,
  with Request Retry Status completions.  The spec doesn't say how
  long to wait here, so we arbitrarily wait up to 60s.

  3) We want to detect missing devices (possibly removed while
  suspended) quickly, without waiting 60s.

I think this might be easier to follow if we restructured a bit, maybe
something like this:

  bool wait_for_link_active(struct pci_dev *bridge)
  {
    /* I don't see a max time to Link Active in the spec (?) */

    for (timeout = 1000; timeout > 0; timeout -= 10) {
      pcie_capability_read_word(bridge, PCI_EXP_LNKSTA, &status);
      if (status & PCI_EXP_LNKSTA_DLLLA)
        return true;
      msleep(10);
    }
    return false;
  }

  pci_bridge_wait_for_secondary_bus(...)
  {
    ...
    if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
      msleep(100);
    } else {
      link_active = wait_for_link_active(dev);
      if (link_active)
        msleep(100);
    }

    /* Everything above is delays mandated by PCIe r6.0 sec 6.6.1 */

    if (dev->link_active_reporting) {
      pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
      if (!(status & PCI_EXP_LNKSTA_DLLLA))
        /* all downstream devices are disconnected; maybe mark them? */
        return;
    }

    /* Wait for non-RRS completion */
    pci_dev_wait(child, PCIE_RESET_READY_POLL_MS);
  }

Re: [PATCH v2 0/2] PCI/PM: Resume/reset wait time change

[Bjorn Helgaas]

On Tue, Apr 04, 2023 at 08:27:12AM +0300, Mika Westerberg wrote:
> Hi all,
> 
> This series first increases the time we wait on resume path to
> accommondate certain device, as reported in [1], and then "optimizes"
> the timeout for slow links to avoid too long waits if a device is
> disconnected during suspend.
> 
> Previous version can be found here:
> 
>   https://lore.kernel.org/linux-pci/20230321095031.65709-1-mika.westerberg@linux.intel.com/
> 
> Changes from the previous version:
> 
>   * Split the patch into two: one that increases the resume timeout (on
>     all links, I was not able to figure out a simple way to limit this
>     only for the fast links) and the one that decreases the timeout on
>     slow links.
> 
>   * Use dev->link_active_reporting instead of speed to figure out slow
>     vs. fast links.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=216728
> 
> Mika Westerberg (2):
>   PCI/PM: Increase wait time after resume

I applied the above to pci/reset for v6.4.

>   PCI/PM: Decrease wait time for devices behind slow links

Part of this patch is removing the pci_bridge_wait_for_secondary_bus()
timeout parameter, since all callers now supply the same value
(PCIE_RESET_READY_POLL_MS).  I extracted that part out and applied it
as well.

I'm hoping we can restructure the rest of this as mentioned in the
thread.  If that's not possible, can you rebase what's left on top of
this?

  https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=reset

>  drivers/pci/pci-driver.c |  2 +-
>  drivers/pci/pci.c        | 42 ++++++++++++++++++++++++++--------------
>  drivers/pci/pci.h        | 16 +--------------
>  drivers/pci/pcie/dpc.c   |  3 +--
>  4 files changed, 30 insertions(+), 33 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 0/2] PCI/PM: Resume/reset wait time change

[Mika Westerberg]

Hi,

On Tue, Apr 11, 2023 at 05:40:14PM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 04, 2023 at 08:27:12AM +0300, Mika Westerberg wrote:
> > Hi all,
> > 
> > This series first increases the time we wait on resume path to
> > accommondate certain device, as reported in [1], and then "optimizes"
> > the timeout for slow links to avoid too long waits if a device is
> > disconnected during suspend.
> > 
> > Previous version can be found here:
> > 
> >   https://lore.kernel.org/linux-pci/20230321095031.65709-1-mika.westerberg@linux.intel.com/
> > 
> > Changes from the previous version:
> > 
> >   * Split the patch into two: one that increases the resume timeout (on
> >     all links, I was not able to figure out a simple way to limit this
> >     only for the fast links) and the one that decreases the timeout on
> >     slow links.
> > 
> >   * Use dev->link_active_reporting instead of speed to figure out slow
> >     vs. fast links.
> > 
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=216728
> > 
> > Mika Westerberg (2):
> >   PCI/PM: Increase wait time after resume
> 
> I applied the above to pci/reset for v6.4.

Thanks!

> >   PCI/PM: Decrease wait time for devices behind slow links
> 
> Part of this patch is removing the pci_bridge_wait_for_secondary_bus()
> timeout parameter, since all callers now supply the same value
> (PCIE_RESET_READY_POLL_MS).  I extracted that part out and applied it
> as well.
> 
> I'm hoping we can restructure the rest of this as mentioned in the
> thread.  If that's not possible, can you rebase what's left on top of
> this?
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=reset

Sure. The end result is below. I did not add the wait_for_link_active()
because we already have the pcie_wait_for_link_delay(), and the
msleep(100) really needs to be msleep(delay) because we extract that
'delay' from the device d3cold_delay which can be more than 100 ms. Let
me know what you think. I will send a proper patch tomorrow if no
objections.

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0b4f3b08f780..61bf8a4b2099 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5037,6 +5037,22 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
 		}
 	}
 
+	/*
+	 * Everything above is handling the delays mandated by the PCIe r6.0
+	 * sec 6.6.1.
+	 *
+	 * If the port supports active link reporting we now check one more
+	 * time if the link is active and if not bail out early with the
+	 * assumption that the device is not present anymore.
+	 */
+	if (dev->link_active_reporting) {
+		u16 status;
+
+		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
+		if (!(status & PCI_EXP_LNKSTA_DLLLA))
+			return -ENOTTY;
+	}
+
 	return pci_dev_wait(child, reset_type,
 			    PCIE_RESET_READY_POLL_MS - delay);
 }

Re: [PATCH v2 0/2] PCI/PM: Resume/reset wait time change

[Bjorn Helgaas]

On Wed, Apr 12, 2023 at 11:00:19AM +0300, Mika Westerberg wrote:
> On Tue, Apr 11, 2023 at 05:40:14PM -0500, Bjorn Helgaas wrote:
> ...

> > I'm hoping we can restructure the rest of this as mentioned in the
> > thread.  If that's not possible, can you rebase what's left on top of
> > this?
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=reset
> 
> Sure. The end result is below. I did not add the wait_for_link_active()
> because we already have the pcie_wait_for_link_delay(), and the
> msleep(100) really needs to be msleep(delay) because we extract that
> 'delay' from the device d3cold_delay which can be more than 100 ms.

Agreed about delay vs 100ms.  I used 100ms in my pidgin patch just for
simplicity.

Yeah, I think this is OK.  I think there's room for improvement around
pcie_wait_for_link_delay() because it does several things that get a
little muddled when mixed together, but your patch is fine as-is:

  - When !pdev->link_active_reporting, it ignores the "active"
    parameter and always returns true ("link is up").  It happens that
    we never wait for the link to go *down* when link_active_reporting
    isn't supported, so the behavior is fine, but this is a weird
    restriction of the interface.

  - IIUC, the 20ms delay comes from a requirement that *hardware* must
    enter LTSSM within 20ms, not software delay requirement.  Also,
    20ms only applies to devices at <= 5.0 GT/s; faster devices have
    up to 100ms.  From the spec point of view, I don't think there's a
    reason to delay config reads to the Downstream Port (we only need
    to delay config accesses to devices below), so I would argue for
    removing all this.

  - It conflates the 1000ms timeout waiting for the link to come up
    with the 100ms wait times in the spec.  AFAIK, we made up the
    1000ms number out of thin air, and I think it would be clearer to
    handle it at a separate place (as opposed to "msleep(timeout +
    delay)").

  - There's only one path (dpc_reset_link()) that waits for the link
    to go *down*.  In that case, pcie_wait_for_link() passes a delay
    of 100ms, but since active==false and spec requires that Ports
    with DPC must implement link_active_reporting, that 100ms is
    ignored.  Works like we want, but ... it feels like it relies a
    little bit too much on internal knowledge.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0b4f3b08f780..61bf8a4b2099 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5037,6 +5037,22 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  		}
>  	}
>  
> +	/*
> +	 * Everything above is handling the delays mandated by the PCIe r6.0
> +	 * sec 6.6.1.
> +	 *
> +	 * If the port supports active link reporting we now check one more
> +	 * time if the link is active and if not bail out early with the
> +	 * assumption that the device is not present anymore.
> +	 */
> +	if (dev->link_active_reporting) {
> +		u16 status;
> +
> +		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
> +		if (!(status & PCI_EXP_LNKSTA_DLLLA))
> +			return -ENOTTY;
> +	}
> +
>  	return pci_dev_wait(child, reset_type,
>  			    PCIE_RESET_READY_POLL_MS - delay);

I have to smile about this: we make sure we don't wait that extra
0.1s when the overall timeout is a completely arbitrary 60s :)

Bjorn