[PATCH v4 0/5] Verify devices transition from D3cold to D0

[PATCH v4 0/5] Verify devices transition from D3cold to D0

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Gary has reported that when a dock is plugged into a system at the same
time the autosuspend delay has tripped that the USB4 stack malfunctions.

Messages show up like this:

```
thunderbolt 0000:e5:00.6: ring_interrupt_active: interrupt for TX ring 0 is already enabled
```

Furthermore the USB4 router is non-functional at this point.

Those messages happen because the device is still in D3cold at the time
that the PCI core handed control back to the USB4 connection manager
(thunderbolt).

The issue is that it takes time for a device to enter D3cold and do a
conventional reset, and then more time for it to exit D3cold.

This appears not to be a new problem; previously there were very similar
reports from Ryzen XHCI controllers.  Quirks were added for those.
Furthermore; adding extra logging it's apparent that other PCI devices
in the system can take more than 10ms to recover from D3cold as well.

This series add a wait into pci_power_up() specifically for D3cold exit and
then drops the quirks that were previously used for the Ryzen XHCI controllers.

v3->v4:
 * Rebase on 6.11-rc4
 * Sparse fix
 * Index based array init
Mario Limonciello (5):
  PCI: Use an enum for reset type in pci_dev_wait()
  PCI: Check PCI_PM_CTRL instead of PCI_COMMAND in pci_dev_wait()
  PCI: Verify functions currently in D3cold have entered D0
  PCI: Allow Ryzen XHCI controllers into D3cold and drop delays
  PCI: Drop Radeon quirk for Macbook Pro 8.2

 drivers/pci/pci-driver.c    |  2 +-
 drivers/pci/pci.c           | 68 +++++++++++++++++++++++++++----------
 drivers/pci/pci.h           | 12 ++++++-
 drivers/pci/pcie/dpc.c      |  2 +-
 drivers/pci/quirks.c        | 25 --------------
 drivers/usb/host/xhci-pci.c | 11 ------
 6 files changed, 63 insertions(+), 57 deletions(-)

-- 
2.43.0

[PATCH v4 1/5] PCI: Use an enum for reset type in pci_dev_wait()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

A string is passed to all callers of pci_dev_wait() which is utilized
to demonstrate what kind of reset happened when there was a problem.

This doesn't allow making the behavior for different reset types
conditional though. Lay some plumbing to allow making comparisons of
reset types with integers instead. No functional changes.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3->v4:
 * Use index-based array initialization format for pci_reset_types
 * Fix LKP reported sparse issue
---
 drivers/pci/pci-driver.c |  2 +-
 drivers/pci/pci.c        | 29 +++++++++++++++++++----------
 drivers/pci/pci.h        | 11 ++++++++++-
 drivers/pci/pcie/dpc.c   |  2 +-
 4 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f412ef73a6e4b..ac3cfbfa137d9 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -572,7 +572,7 @@ static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev)
 {
 	int ret;
 
-	ret = pci_bridge_wait_for_secondary_bus(pci_dev, "resume");
+	ret = pci_bridge_wait_for_secondary_bus(pci_dev, PCI_DEV_WAIT_RESUME);
 	if (ret) {
 		/*
 		 * The downstream link failed to come up, so mark the
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ffaaca0978cbc..e4a7f5dfe6bf4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -181,6 +181,15 @@ static int __init pcie_port_pm_setup(char *str)
 }
 __setup("pcie_port_pm=", pcie_port_pm_setup);
 
+static const char * const pci_reset_types[] = {
+	[PCI_DEV_WAIT_FLR] = "FLR",
+	[PCI_DEV_WAIT_AF_FLR] = "AF_FLR",
+	[PCI_DEV_WAIT_D3HOT_D0] = "PM D3HOT->D0",
+	[PCI_DEV_WAIT_BUS_RESET] = "bus reset",
+	[PCI_DEV_WAIT_RESUME] = "resume",
+	[PCI_DEV_WAIT_DPC] = "DPC",
+};
+
 /**
  * pci_bus_max_busnr - returns maximum PCI bus number of given bus' children
  * @bus: pointer to PCI bus structure to search
@@ -1279,7 +1288,7 @@ void pci_resume_bus(struct pci_bus *bus)
 		pci_walk_bus(bus, pci_resume_one, NULL);
 }
 
-static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
+static int pci_dev_wait(struct pci_dev *dev, enum pci_reset_type reset_type, int timeout)
 {
 	int delay = 1;
 	bool retrain = false;
@@ -1317,7 +1326,7 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 
 		if (delay > timeout) {
 			pci_warn(dev, "not ready %dms after %s; giving up\n",
-				 delay - 1, reset_type);
+				 delay - 1, pci_reset_types[reset_type]);
 			return -ENOTTY;
 		}
 
@@ -1330,7 +1339,7 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 				}
 			}
 			pci_info(dev, "not ready %dms after %s; waiting\n",
-				 delay - 1, reset_type);
+				 delay - 1, pci_reset_types[reset_type]);
 		}
 
 		msleep(delay);
@@ -1339,10 +1348,10 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 
 	if (delay > PCI_RESET_WAIT)
 		pci_info(dev, "ready %dms after %s\n", delay - 1,
-			 reset_type);
+			 pci_reset_types[reset_type]);
 	else
 		pci_dbg(dev, "ready %dms after %s\n", delay - 1,
-			reset_type);
+			pci_reset_types[reset_type]);
 
 	return 0;
 }
@@ -4536,7 +4545,7 @@ int pcie_flr(struct pci_dev *dev)
 	 */
 	msleep(100);
 
-	return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
+	return pci_dev_wait(dev, PCI_DEV_WAIT_FLR, PCIE_RESET_READY_POLL_MS);
 }
 EXPORT_SYMBOL_GPL(pcie_flr);
 
@@ -4603,7 +4612,7 @@ static int pci_af_flr(struct pci_dev *dev, bool probe)
 	 */
 	msleep(100);
 
-	return pci_dev_wait(dev, "AF_FLR", PCIE_RESET_READY_POLL_MS);
+	return pci_dev_wait(dev, PCI_DEV_WAIT_AF_FLR, PCIE_RESET_READY_POLL_MS);
 }
 
 /**
@@ -4648,7 +4657,7 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
 	pci_dev_d3_sleep(dev);
 
-	return pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS);
+	return pci_dev_wait(dev, PCI_DEV_WAIT_D3HOT_D0, PCIE_RESET_READY_POLL_MS);
 }
 
 /**
@@ -4822,7 +4831,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 pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, enum pci_reset_type reset_type)
 {
 	struct pci_dev *child __free(pci_dev_put) = NULL;
 	int delay;
@@ -4959,7 +4968,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
 			      __builtin_return_address(0));
 	pcibios_reset_secondary_bus(dev);
 
-	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
+	return pci_bridge_wait_for_secondary_bus(dev, PCI_DEV_WAIT_BUS_RESET);
 }
 EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 79c8398f39384..477257e843952 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -4,6 +4,15 @@
 
 #include <linux/pci.h>
 
+enum pci_reset_type {
+	PCI_DEV_WAIT_FLR,
+	PCI_DEV_WAIT_AF_FLR,
+	PCI_DEV_WAIT_D3HOT_D0,
+	PCI_DEV_WAIT_BUS_RESET,
+	PCI_DEV_WAIT_RESUME,
+	PCI_DEV_WAIT_DPC,
+};
+
 /* Number of possible devfns: 0.0 to 1f.7 inclusive */
 #define MAX_NR_DEVFNS 256
 
@@ -137,7 +146,7 @@ void pci_msi_init(struct pci_dev *dev);
 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);
-int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
+int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, enum pci_reset_type 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 2b6ef7efa3c11..95cd985244729 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -174,7 +174,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")) {
+	if (pci_bridge_wait_for_secondary_bus(pdev, PCI_DEV_WAIT_DPC)) {
 		clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
 		ret = PCI_ERS_RESULT_DISCONNECT;
 	} else {
-- 
2.43.0

Re: [PATCH v4 1/5] PCI: Use an enum for reset type in pci_dev_wait()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 6954 bytes --]

On Thu, 22 Aug 2024, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> A string is passed to all callers of pci_dev_wait() which is utilized
> to demonstrate what kind of reset happened when there was a problem.
> 
> This doesn't allow making the behavior for different reset types
> conditional though. Lay some plumbing to allow making comparisons of
> reset types with integers instead. No functional changes.
> 
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> ---
> v3->v4:
>  * Use index-based array initialization format for pci_reset_types
>  * Fix LKP reported sparse issue
> ---
>  drivers/pci/pci-driver.c |  2 +-
>  drivers/pci/pci.c        | 29 +++++++++++++++++++----------
>  drivers/pci/pci.h        | 11 ++++++++++-
>  drivers/pci/pcie/dpc.c   |  2 +-
>  4 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f412ef73a6e4b..ac3cfbfa137d9 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -572,7 +572,7 @@ static void pci_pm_bridge_power_up_actions(struct pci_dev *pci_dev)
>  {
>  	int ret;
>  
> -	ret = pci_bridge_wait_for_secondary_bus(pci_dev, "resume");
> +	ret = pci_bridge_wait_for_secondary_bus(pci_dev, PCI_DEV_WAIT_RESUME);
>  	if (ret) {
>  		/*
>  		 * The downstream link failed to come up, so mark the
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ffaaca0978cbc..e4a7f5dfe6bf4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -181,6 +181,15 @@ static int __init pcie_port_pm_setup(char *str)
>  }
>  __setup("pcie_port_pm=", pcie_port_pm_setup);
>  
> +static const char * const pci_reset_types[] = {
> +	[PCI_DEV_WAIT_FLR] = "FLR",
> +	[PCI_DEV_WAIT_AF_FLR] = "AF_FLR",
> +	[PCI_DEV_WAIT_D3HOT_D0] = "PM D3HOT->D0",
> +	[PCI_DEV_WAIT_BUS_RESET] = "bus reset",
> +	[PCI_DEV_WAIT_RESUME] = "resume",
> +	[PCI_DEV_WAIT_DPC] = "DPC",
> +};
> +
>  /**
>   * pci_bus_max_busnr - returns maximum PCI bus number of given bus' children
>   * @bus: pointer to PCI bus structure to search
> @@ -1279,7 +1288,7 @@ void pci_resume_bus(struct pci_bus *bus)
>  		pci_walk_bus(bus, pci_resume_one, NULL);
>  }
>  
> -static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
> +static int pci_dev_wait(struct pci_dev *dev, enum pci_reset_type reset_type, int timeout)
>  {
>  	int delay = 1;
>  	bool retrain = false;
> @@ -1317,7 +1326,7 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  
>  		if (delay > timeout) {
>  			pci_warn(dev, "not ready %dms after %s; giving up\n",
> -				 delay - 1, reset_type);
> +				 delay - 1, pci_reset_types[reset_type]);
>  			return -ENOTTY;
>  		}
>  
> @@ -1330,7 +1339,7 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  				}
>  			}
>  			pci_info(dev, "not ready %dms after %s; waiting\n",
> -				 delay - 1, reset_type);
> +				 delay - 1, pci_reset_types[reset_type]);
>  		}
>  
>  		msleep(delay);
> @@ -1339,10 +1348,10 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>  
>  	if (delay > PCI_RESET_WAIT)
>  		pci_info(dev, "ready %dms after %s\n", delay - 1,
> -			 reset_type);
> +			 pci_reset_types[reset_type]);
>  	else
>  		pci_dbg(dev, "ready %dms after %s\n", delay - 1,
> -			reset_type);
> +			pci_reset_types[reset_type]);
>  
>  	return 0;
>  }
> @@ -4536,7 +4545,7 @@ int pcie_flr(struct pci_dev *dev)
>  	 */
>  	msleep(100);
>  
> -	return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
> +	return pci_dev_wait(dev, PCI_DEV_WAIT_FLR, PCIE_RESET_READY_POLL_MS);
>  }
>  EXPORT_SYMBOL_GPL(pcie_flr);
>  
> @@ -4603,7 +4612,7 @@ static int pci_af_flr(struct pci_dev *dev, bool probe)
>  	 */
>  	msleep(100);
>  
> -	return pci_dev_wait(dev, "AF_FLR", PCIE_RESET_READY_POLL_MS);
> +	return pci_dev_wait(dev, PCI_DEV_WAIT_AF_FLR, PCIE_RESET_READY_POLL_MS);
>  }
>  
>  /**
> @@ -4648,7 +4657,7 @@ static int pci_pm_reset(struct pci_dev *dev, bool probe)
>  	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
>  	pci_dev_d3_sleep(dev);
>  
> -	return pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS);
> +	return pci_dev_wait(dev, PCI_DEV_WAIT_D3HOT_D0, PCIE_RESET_READY_POLL_MS);
>  }
>  
>  /**
> @@ -4822,7 +4831,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 pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, enum pci_reset_type reset_type)
>  {
>  	struct pci_dev *child __free(pci_dev_put) = NULL;
>  	int delay;
> @@ -4959,7 +4968,7 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
>  			      __builtin_return_address(0));
>  	pcibios_reset_secondary_bus(dev);
>  
> -	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> +	return pci_bridge_wait_for_secondary_bus(dev, PCI_DEV_WAIT_BUS_RESET);
>  }
>  EXPORT_SYMBOL_GPL(pci_bridge_secondary_bus_reset);
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 79c8398f39384..477257e843952 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -4,6 +4,15 @@
>  
>  #include <linux/pci.h>
>  
> +enum pci_reset_type {
> +	PCI_DEV_WAIT_FLR,
> +	PCI_DEV_WAIT_AF_FLR,
> +	PCI_DEV_WAIT_D3HOT_D0,
> +	PCI_DEV_WAIT_BUS_RESET,
> +	PCI_DEV_WAIT_RESUME,
> +	PCI_DEV_WAIT_DPC,
> +};
> +
>  /* Number of possible devfns: 0.0 to 1f.7 inclusive */
>  #define MAX_NR_DEVFNS 256
>  
> @@ -137,7 +146,7 @@ void pci_msi_init(struct pci_dev *dev);
>  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);
> -int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
> +int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, enum pci_reset_type 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 2b6ef7efa3c11..95cd985244729 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -174,7 +174,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")) {
> +	if (pci_bridge_wait_for_secondary_bus(pdev, PCI_DEV_WAIT_DPC)) {
>  		clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
>  		ret = PCI_ERS_RESULT_DISCONNECT;
>  	} else {
> 

[PATCH] x86/tsc: Use rdtsc_ordered() when RDTSCP or LFENCE_RDTSC are supported

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

On AMD processors the TSC has been reported drifting on and off for
various platforms.  This has been root caused to becaused by out of order
TSC and HPET counter values.  When the SoC supports RDTSCP or LFENCE_RDTSC
use ordered tsc reads instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/include/asm/tsc.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 94408a784c8e7..1c0cda1702bec 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -24,6 +24,9 @@ static inline cycles_t get_cycles(void)
 	if (!IS_ENABLED(CONFIG_X86_TSC) &&
 	    !cpu_feature_enabled(X86_FEATURE_TSC))
 		return 0;
+	if (cpu_feature_enabled(X86_FEATURE_LFENCE_RDTSC) ||
+	    cpu_feature_enabled(X86_FEATURE_RDTSCP))
+		return rdtsc_ordered();
 	return rdtsc();
 }
 #define get_cycles get_cycles
-- 
2.43.0

Re: [PATCH] x86/tsc: Use rdtsc_ordered() when RDTSCP or LFENCE_RDTSC are supported

[Mario Limonciello]

On 8/22/24 23:25, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> On AMD processors the TSC has been reported drifting on and off for
> various platforms.  This has been root caused to becaused by out of order
> TSC and HPET counter values.  When the SoC supports RDTSCP or LFENCE_RDTSC
> use ordered tsc reads instead.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   arch/x86/include/asm/tsc.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
> index 94408a784c8e7..1c0cda1702bec 100644
> --- a/arch/x86/include/asm/tsc.h
> +++ b/arch/x86/include/asm/tsc.h
> @@ -24,6 +24,9 @@ static inline cycles_t get_cycles(void)
>   	if (!IS_ENABLED(CONFIG_X86_TSC) &&
>   	    !cpu_feature_enabled(X86_FEATURE_TSC))
>   		return 0;
> +	if (cpu_feature_enabled(X86_FEATURE_LFENCE_RDTSC) ||
> +	    cpu_feature_enabled(X86_FEATURE_RDTSCP))
> +		return rdtsc_ordered();
>   	return rdtsc();
>   }
>   #define get_cycles get_cycles

Sorry; this unrelated patch I didn't intend to include in the series, it 
was in my working directory by accident.  Please disregard it for now, 
but review the rest of the series.

Thanks!

Re: [PATCH] x86/tsc: Use rdtsc_ordered() when RDTSCP or LFENCE_RDTSC are supported

[Thomas Gleixner]

On Thu, Aug 22 2024 at 23:25, Mario Limonciello wrote:

Why is this hidden in a reply to the middle of a PCI patch series?

Sigh.

> From: Mario Limonciello <mario.limonciello@amd.com>
>
> On AMD processors the TSC has been reported drifting on and off for
> various platforms.  This has been root caused to becaused by out of order
> TSC and HPET counter values.  When the SoC supports RDTSCP or LFENCE_RDTSC
> use ordered tsc reads instead.

This really wants a fixes tag.

Re: [PATCH] x86/tsc: Use rdtsc_ordered() when RDTSCP or LFENCE_RDTSC are supported

[Mario Limonciello]

On 8/25/24 07:22, Thomas Gleixner wrote:
> On Thu, Aug 22 2024 at 23:25, Mario Limonciello wrote:
> 
> Why is this hidden in a reply to the middle of a PCI patch series?
> 
> Sigh.

As I mentioned in my reply I didn't mean for this to go out at this 
time.  Sorry for the noise!

It's still under testing that it REALLY helps things.
It was in my working directory and I totally missed it when I sent this 
PCI series.

> 
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> On AMD processors the TSC has been reported drifting on and off for
>> various platforms.  This has been root caused to becaused by out of order
>> TSC and HPET counter values.  When the SoC supports RDTSCP or LFENCE_RDTSC
>> use ordered tsc reads instead.
> 
> This really wants a fixes tag.

Yes; assuming it really helps I will send it properly with tags and to 
the right lists.

[PATCH v4 2/5] PCI: Check PCI_PM_CTRL instead of PCI_COMMAND in pci_dev_wait()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

A device that has gone through a reset may return a value in PCI_COMMAND
but that doesn't mean it's finished transitioning to D0.  On devices that
support power management explicitly check PCI_PM_CTRL on everything but
system resume to ensure the transition happened.

Devices that don't support power management and system resume will
continue to use PCI_COMMAND.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e4a7f5dfe6bf4..b7717155e2fd0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1308,21 +1308,33 @@ static int pci_dev_wait(struct pci_dev *dev, enum pci_reset_type reset_type, int
 	 * 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.
+	 * Wait for the device to return a non-CRS completion.  On devices
+	 * that support PM control and on waits that aren't part of system
+	 * resume read the PM control register to ensure the device has
+	 * transitioned to D0.  On devices that don't support PM control,
+	 * or during system resume read the command register to instead of
+	 * Vendor ID so we don't have to contend with the CRS SV value.
 	 */
 	for (;;) {
-		u32 id;
-
 		if (pci_dev_is_disconnected(dev)) {
 			pci_dbg(dev, "disconnected; not waiting\n");
 			return -ENOTTY;
 		}
 
-		pci_read_config_dword(dev, PCI_COMMAND, &id);
-		if (!PCI_POSSIBLE_ERROR(id))
-			break;
+		if (dev->pm_cap && reset_type != PCI_DEV_WAIT_RESUME) {
+			u16 pmcsr;
+
+			pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
+			if (!PCI_POSSIBLE_ERROR(pmcsr) &&
+				(pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D0)
+				break;
+		} else {
+			u32 id;
+
+			pci_read_config_dword(dev, PCI_COMMAND, &id);
+			if (!PCI_POSSIBLE_ERROR(id))
+				break;
+		}
 
 		if (delay > timeout) {
 			pci_warn(dev, "not ready %dms after %s; giving up\n",
-- 
2.43.0

Re: [PATCH v4 2/5] PCI: Check PCI_PM_CTRL instead of PCI_COMMAND in pci_dev_wait()

[Ilpo Järvinen]

On Thu, 22 Aug 2024, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> A device that has gone through a reset may return a value in PCI_COMMAND
> but that doesn't mean it's finished transitioning to D0.  On devices that
> support power management explicitly check PCI_PM_CTRL on everything but
> system resume to ensure the transition happened.
> 
> Devices that don't support power management and system resume will
> continue to use PCI_COMMAND.

It feels part of the coverletter text would belong into this patch.

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/pci.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e4a7f5dfe6bf4..b7717155e2fd0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1308,21 +1308,33 @@ static int pci_dev_wait(struct pci_dev *dev, enum pci_reset_type reset_type, int
>  	 * 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.
> +	 * Wait for the device to return a non-CRS completion.  On devices
> +	 * that support PM control and on waits that aren't part of system
> +	 * resume read the PM control register to ensure the device has
> +	 * transitioned to D0.  On devices that don't support PM control,
> +	 * or during system resume read the command register to instead of
> +	 * Vendor ID so we don't have to contend with the CRS SV value.
>  	 */
>  	for (;;) {
> -		u32 id;
> -
>  		if (pci_dev_is_disconnected(dev)) {
>  			pci_dbg(dev, "disconnected; not waiting\n");
>  			return -ENOTTY;
>  		}
>  
> -		pci_read_config_dword(dev, PCI_COMMAND, &id);
> -		if (!PCI_POSSIBLE_ERROR(id))
> -			break;
> +		if (dev->pm_cap && reset_type != PCI_DEV_WAIT_RESUME) {
> +			u16 pmcsr;
> +
> +			pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> +			if (!PCI_POSSIBLE_ERROR(pmcsr) &&
> +				(pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D0)

Misleading indentation.

-- 
 i.

> +				break;
> +		} else {
> +			u32 id;
> +
> +			pci_read_config_dword(dev, PCI_COMMAND, &id);
> +			if (!PCI_POSSIBLE_ERROR(id))
> +				break;
> +		}
>  
>  		if (delay > timeout) {
>  			pci_warn(dev, "not ready %dms after %s; giving up\n",
> 

[PATCH v4 3/5] PCI: Verify functions currently in D3cold have entered D0

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

It is reported that USB4 routers and downstream devices may behave
incorrectly if a dock cable is plugged in at approximately the time that
the autosuspend_delay is configured. In this situation the device has
attempted to enter D3cold, but didn't finish D3cold entry when the PCI
core tried to transition it back to D0.

Empirically measuring this situation an "aborted" D3cold exit takes
~60ms and a "normal" D3cold exit takes ~6ms.

The PCI-PM 1.2 spec specifies that the restore time for functions
in D3cold is either 'Full context restore or boot latency'.

As PCIe r6.0 sec 5.8 specifies that the device will have gone
through a conventional reset, it may take some time for the
device to be ready.

Wait up to 1 sec as specified in PCIe r6.0 sec 6.6.1 for a device
in D3cold to return to D0.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/pci.c | 11 +++++++++++
 drivers/pci/pci.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b7717155e2fd0..7e861b6923d0a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1425,6 +1425,17 @@ int pci_power_up(struct pci_dev *dev)
 	else if (state == PCI_D2)
 		udelay(PCI_PM_D2_DELAY);
 
+	/*
+	 * D3cold -> D0 will have gone through a conventional reset and may need
+	 * time to be ready.
+	 */
+	if (dev->current_state == PCI_D3cold) {
+		int ret;
+
+		ret = pci_dev_wait(dev, PCI_DEV_WAIT_D3COLD_D0, PCI_RESET_WAIT);
+		if (ret)
+			return ret;
+	}
 end:
 	dev->current_state = PCI_D0;
 	if (need_restore)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 477257e843952..a675f5d55f298 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -11,6 +11,7 @@ enum pci_reset_type {
 	PCI_DEV_WAIT_BUS_RESET,
 	PCI_DEV_WAIT_RESUME,
 	PCI_DEV_WAIT_DPC,
+	PCI_DEV_WAIT_D3COLD_D0,
 };
 
 /* Number of possible devfns: 0.0 to 1f.7 inclusive */
-- 
2.43.0

Re: [PATCH v4 3/5] PCI: Verify functions currently in D3cold have entered D0

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

On Thu, 22 Aug 2024, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> It is reported that USB4 routers and downstream devices may behave
> incorrectly if a dock cable is plugged in at approximately the time that
> the autosuspend_delay is configured. In this situation the device has
> attempted to enter D3cold, but didn't finish D3cold entry when the PCI
> core tried to transition it back to D0.
> 
> Empirically measuring this situation an "aborted" D3cold exit takes
> ~60ms and a "normal" D3cold exit takes ~6ms.
> 
> The PCI-PM 1.2 spec specifies that the restore time for functions
> in D3cold is either 'Full context restore or boot latency'.
> 
> As PCIe r6.0 sec 5.8 specifies that the device will have gone
> through a conventional reset, it may take some time for the
> device to be ready.
> 
> Wait up to 1 sec as specified in PCIe r6.0 sec 6.6.1 for a device
> in D3cold to return to D0.
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/pci.c | 11 +++++++++++
>  drivers/pci/pci.h |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b7717155e2fd0..7e861b6923d0a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1425,6 +1425,17 @@ int pci_power_up(struct pci_dev *dev)
>  	else if (state == PCI_D2)
>  		udelay(PCI_PM_D2_DELAY);
>  
> +	/*
> +	 * D3cold -> D0 will have gone through a conventional reset and may need
> +	 * time to be ready.
> +	 */
> +	if (dev->current_state == PCI_D3cold) {
> +		int ret;
> +
> +		ret = pci_dev_wait(dev, PCI_DEV_WAIT_D3COLD_D0, PCI_RESET_WAIT);
> +		if (ret)
> +			return ret;
> +	}
>  end:
>  	dev->current_state = PCI_D0;
>  	if (need_restore)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 477257e843952..a675f5d55f298 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -11,6 +11,7 @@ enum pci_reset_type {
>  	PCI_DEV_WAIT_BUS_RESET,
>  	PCI_DEV_WAIT_RESUME,
>  	PCI_DEV_WAIT_DPC,
> +	PCI_DEV_WAIT_D3COLD_D0,

Don't you need to add a string for this too? :-/

I wonder if it would be prudent to add PCI_DEV_WAIT_MAX and 
use static_assert() for the sizeof the pci_reset_types[] in patch 1 to 
autodetect mismatch (though it won't help if something is added in the 
middle of the list).

-- 
 i.

[PATCH v4 4/5] PCI: Allow Ryzen XHCI controllers into D3cold and drop delays

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

As the PCI core now has a delay after D3cold exit, the Ryzen XHCI
controllers that were quirked to not use D3cold and to add a delay
on D3hot no longer need these quirks.

Drop both the PCI and XHCI sets of quirks.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/quirks.c        | 17 -----------------
 drivers/usb/host/xhci-pci.c | 11 -----------
 2 files changed, 28 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a2ce4e08edf5a..3480a0445ff50 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2059,23 +2059,6 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
 			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8,
 			      quirk_nvidia_hda_pm);
 
-/*
- * Ryzen5/7 XHCI controllers fail upon resume from runtime suspend or s2idle.
- * https://bugzilla.kernel.org/show_bug.cgi?id=205587
- *
- * The kernel attempts to transition these devices to D3cold, but that seems
- * to be ineffective on the platforms in question; the PCI device appears to
- * remain on in D3hot state. The D3hot-to-D0 transition then requires an
- * extended delay in order to succeed.
- */
-static void quirk_ryzen_xhci_d3hot(struct pci_dev *dev)
-{
-	quirk_d3hot_delay(dev, 20);
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x15e0, quirk_ryzen_xhci_d3hot);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x15e1, quirk_ryzen_xhci_d3hot);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x1639, quirk_ryzen_xhci_d3hot);
-
 #ifdef CONFIG_X86_IO_APIC
 static int dmi_disable_ioapicreroute(const struct dmi_system_id *d)
 {
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index dc1e345ab67ea..d726810a04838 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -316,10 +316,6 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		(pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
 		xhci->quirks |= XHCI_U2_DISABLE_WAKE;
 
-	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
-		pdev->device == PCI_DEVICE_ID_AMD_RENOIR_XHCI)
-		xhci->quirks |= XHCI_BROKEN_D3COLD_S2I;
-
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 		xhci->quirks |= XHCI_INTEL_HOST;
@@ -752,13 +748,6 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
 		pci_d3cold_disable(pdev);
 
-#ifdef CONFIG_SUSPEND
-	/* d3cold is broken, but only when s2idle is used */
-	if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE &&
-	    xhci->quirks & (XHCI_BROKEN_D3COLD_S2I))
-		pci_d3cold_disable(pdev);
-#endif
-
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_quirk(hcd);
 
-- 
2.43.0

Re: [PATCH v4 4/5] PCI: Allow Ryzen XHCI controllers into D3cold and drop delays

[Greg Kroah-Hartman]

On Thu, Aug 22, 2024 at 11:25:07PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> As the PCI core now has a delay after D3cold exit, the Ryzen XHCI
> controllers that were quirked to not use D3cold and to add a delay
> on D3hot no longer need these quirks.
> 
> Drop both the PCI and XHCI sets of quirks.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v4 5/5] PCI: Drop Radeon quirk for Macbook Pro 8.2

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

commit 5938628c51a7 ("drm/radeon: make MacBook Pro d3_delay quirk more
generic") introduced a generic quirk for Macbook Pro 8.2s that contain
Radeon graphics to ensure that enough time had past when the device
was powered on.

As the PCI core now verifies the device is in D0 during power on this
extra artificial delay is no longer necessary.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pci/quirks.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 3480a0445ff50..e76ff1037fb35 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2038,14 +2038,6 @@ static void quirk_d3hot_delay(struct pci_dev *dev, unsigned int delay)
 		 dev->d3hot_delay);
 }
 
-static void quirk_radeon_pm(struct pci_dev *dev)
-{
-	if (dev->subsystem_vendor == PCI_VENDOR_ID_APPLE &&
-	    dev->subsystem_device == 0x00e2)
-		quirk_d3hot_delay(dev, 20);
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
-
 /*
  * NVIDIA Ampere-based HDA controllers can wedge the whole device if a bus
  * reset is performed too soon after transition to D0, extend d3hot_delay
-- 
2.43.0