[PATCH v13 00/12] Fix wakeup problems on some AMD platforms

[PATCH v13 00/12] Fix wakeup problems on some AMD platforms

[Mario Limonciello]

Problems have been reported on AMD laptops with suspend/resume
where particular root ports are put into D3 and then the system is unable
to resume properly.

This is caused by the policy used by the Linux kernel to put PCIe root ports
into D3. This series adjusts the policy to be more conservative and only
put root ports into D3 if the platform has indicated that it is necessary
to do so.

Andy Shevchenko (1):
  ACPI: x86: s2idle: Add for_each_lpi_constraint() helper

Mario Limonciello (11):
  PCI: Only put Intel PCIe ports >= 2015 into D3
  ACPI: Add comments to clarify some #ifdef statements
  ACPI: Adjust #ifdef for *_lps0_dev use
  ACPI: x86: s2idle: Post-increment variables when getting constraints
  ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects
  ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table
  ACPI: x86: s2idle: Add more debugging for AMD constraints parsing
  ACPI: x86: s2idle: Add a function to get constraints for a device
  PCI: ACPI: Add helper functions for converting ACPI <->PCI states
  PCI: ACPI: Use device constraints to opt devices into D3 support
  PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

 drivers/acpi/x86/s2idle.c |  99 ++++++++++++++++++++++++-------------
 drivers/pci/pci-acpi.c    | 101 +++++++++++++++++++++++++-------------
 drivers/pci/pci.c         |  18 ++++++-
 drivers/pci/pci.h         |   5 ++
 include/linux/acpi.h      |  14 ++++--
 5 files changed, 163 insertions(+), 74 deletions(-)
base-commit: 2ccdd1b13c591d306f0401d98dedc4bdcd02b421
-- 
2.34.1

[PATCH v13 01/12] PCI: Only put Intel PCIe ports >= 2015 into D3

[Mario Limonciello]

commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
changed pci_bridge_d3_possible() so that any vendor's PCIe ports
from modern machines (>=2015) are allowed to be put into D3.

Iain reports that USB devices can't be used to wake a Lenovo Z13
from suspend. This is because the PCIe root port has been put
into D3 and AMD's platform can't handle USB devices waking in this
case.

This behavior is only reported on Linux. Comparing the behavior
on Windows and Linux, Windows doesn't put the root ports into D3.

To fix the issue without regressing existing Intel systems,
limit the >=2015 check to only apply to Intel PCIe ports.

Cc: stable@vger.kernel.org
Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
Reported-by: Iain Lane <iain@orangesquash.org.uk>
Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * New patch
---
 drivers/pci/pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0c..051e88ee64c63 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3037,10 +3037,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 			return false;
 
 		/*
-		 * It should be safe to put PCIe ports from 2015 or newer
+		 * It is safe to put Intel PCIe ports from 2015 or newer
 		 * to D3.
 		 */
-		if (dmi_get_bios_year() >= 2015)
+		if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
+		    dmi_get_bios_year() >= 2015)
 			return true;
 		break;
 	}
-- 
2.34.1

[PATCH v13 02/12] ACPI: Add comments to clarify some #ifdef statements

[Mario Limonciello]

With nested #ifdef statements it's sometimes difficult to tell
which code goes with which statement.
* One comment said it was for the end of a CONFIG_X86 block,
  which was wrong as it was actually a CONFIG_IA64 block.
* Another block for CONFIG_ACPI didn't have a comment.

Fix the comment for the CONFIG_IA64 block and add a comment
for the CONFIG_ACPI block.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * Commit message improvements
v9->v10:
 * no changes
---
 include/linux/acpi.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 641dc48439873..0d5277b7c6323 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1117,10 +1117,10 @@ static inline void arch_reserve_mem_area(acpi_physical_address addr,
 					  size_t size)
 {
 }
-#endif /* CONFIG_X86 */
+#endif /* CONFIG_IA64 */
 #else
 #define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0)
-#endif
+#endif /* CONFIG_ACPI */
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM)
 int acpi_dev_suspend(struct device *dev, bool wakeup);
-- 
2.34.1

[PATCH v13 03/12] ACPI: Adjust #ifdef for *_lps0_dev use

[Mario Limonciello]

The `#ifdef` for acpi_register_lps0_dev() currently is guarded against
`CONFIG_X86`, but actually the functions contained in the block are
specifically sleep related functions.
Adjust the guard to also check for `CONFIG_SUSPEND`.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * Adjust commit messsage
v11->v12:
 * change to CONFIG_SUSPEND
v9->v10:
 * split from other patches
---
 include/linux/acpi.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 0d5277b7c6323..f1552c04a2856 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1100,7 +1100,7 @@ void acpi_os_set_prepare_extended_sleep(int (*func)(u8 sleep_state,
 
 acpi_status acpi_os_prepare_extended_sleep(u8 sleep_state,
 					   u32 val_a, u32 val_b);
-#ifdef CONFIG_X86
+#if defined(CONFIG_SUSPEND) && defined(CONFIG_X86)
 struct acpi_s2idle_dev_ops {
 	struct list_head list_node;
 	void (*prepare)(void);
@@ -1109,7 +1109,7 @@ struct acpi_s2idle_dev_ops {
 };
 int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
 void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
-#endif /* CONFIG_X86 */
+#endif /* CONFIG_SUSPEND && CONFIG_X86 */
 #ifndef CONFIG_IA64
 void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
 #else
-- 
2.34.1

[PATCH v13 04/12] ACPI: x86: s2idle: Post-increment variables when getting constraints

[Mario Limonciello]

When code uses a pre-increment it makes the reader question "why".
In the constraint fetching code there is no reason for the variables
to be pre-incremented so adjust to post-increment.
No intended functional changes.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * Add tag
 * Reword message
---
 drivers/acpi/x86/s2idle.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index ce62e61a9605e..7711dde68947f 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -123,13 +123,13 @@ static void lpi_device_get_constraints_amd(void)
 			acpi_handle_debug(lps0_device_handle,
 					  "LPI: constraints list begin:\n");
 
-			for (j = 0; j < package->package.count; ++j) {
+			for (j = 0; j < package->package.count; j++) {
 				union acpi_object *info_obj = &package->package.elements[j];
 				struct lpi_device_constraint_amd dev_info = {};
 				struct lpi_constraints *list;
 				acpi_status status;
 
-				for (k = 0; k < info_obj->package.count; ++k) {
+				for (k = 0; k < info_obj->package.count; k++) {
 					union acpi_object *obj = &info_obj->package.elements[k];
 
 					list = &lpi_constraints_table[lpi_constraints_table_size];
@@ -214,7 +214,7 @@ static void lpi_device_get_constraints(void)
 		if (!package)
 			continue;
 
-		for (j = 0; j < package->package.count; ++j) {
+		for (j = 0; j < package->package.count; j++) {
 			union acpi_object *element =
 					&(package->package.elements[j]);
 
@@ -246,7 +246,7 @@ static void lpi_device_get_constraints(void)
 
 		constraint->min_dstate = -1;
 
-		for (j = 0; j < package_count; ++j) {
+		for (j = 0; j < package_count; j++) {
 			union acpi_object *info_obj = &info.package[j];
 			union acpi_object *cnstr_pkg;
 			union acpi_object *obj;
-- 
2.34.1

[PATCH v13 05/12] ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects

[Mario Limonciello]

If a badly constructed firmware includes multiple `ACPI_TYPE_PACKAGE`
objects while evaluating the AMD LPS0 _DSM, there will be a memory
leak.  Explicitly guard against this.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/x86/s2idle.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 7711dde68947f..508decbac2986 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -113,6 +113,12 @@ static void lpi_device_get_constraints_amd(void)
 		union acpi_object *package = &out_obj->package.elements[i];
 
 		if (package->type == ACPI_TYPE_PACKAGE) {
+			if (lpi_constraints_table) {
+				acpi_handle_err(lps0_device_handle,
+						"Duplicate constraints list\n");
+				goto free_acpi_buffer;
+			}
+
 			lpi_constraints_table = kcalloc(package->package.count,
 							sizeof(*lpi_constraints_table),
 							GFP_KERNEL);
-- 
2.34.1

[PATCH v13 06/12] ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table

[Mario Limonciello]

The constraints table should be resetting the `list` object
after running through all of `info_obj` iterations.

This adjusts whitespace as well as less code will now be included
with each loop. This fixes a functional problem is fixed where a
badly formed package in the inner loop may have incorrect data.

Fixes: 146f1ed852a8 ("ACPI: PM: s2idle: Add AMD support to handle _DSM")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v11->v12:
 * Update commit message
v9->v10:
 * split from other patches
---
 drivers/acpi/x86/s2idle.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 508decbac2986..60835953ebfc4 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -135,12 +135,11 @@ static void lpi_device_get_constraints_amd(void)
 				struct lpi_constraints *list;
 				acpi_status status;
 
+				list = &lpi_constraints_table[lpi_constraints_table_size];
+
 				for (k = 0; k < info_obj->package.count; k++) {
 					union acpi_object *obj = &info_obj->package.elements[k];
 
-					list = &lpi_constraints_table[lpi_constraints_table_size];
-					list->min_dstate = -1;
-
 					switch (k) {
 					case 0:
 						dev_info.enabled = obj->integer.value;
@@ -155,27 +154,21 @@ static void lpi_device_get_constraints_amd(void)
 						dev_info.min_dstate = obj->integer.value;
 						break;
 					}
+				}
 
-					if (!dev_info.enabled || !dev_info.name ||
-					    !dev_info.min_dstate)
-						continue;
+				if (!dev_info.enabled || !dev_info.name ||
+				    !dev_info.min_dstate)
+					continue;
 
-					status = acpi_get_handle(NULL, dev_info.name,
-								 &list->handle);
-					if (ACPI_FAILURE(status))
-						continue;
+				status = acpi_get_handle(NULL, dev_info.name, &list->handle);
+				if (ACPI_FAILURE(status))
+					continue;
 
-					acpi_handle_debug(lps0_device_handle,
-							  "Name:%s\n", dev_info.name);
+				acpi_handle_debug(lps0_device_handle,
+						  "Name:%s\n", dev_info.name);
 
-					list->min_dstate = dev_info.min_dstate;
+				list->min_dstate = dev_info.min_dstate;
 
-					if (list->min_dstate < 0) {
-						acpi_handle_debug(lps0_device_handle,
-								  "Incomplete constraint defined\n");
-						continue;
-					}
-				}
 				lpi_constraints_table_size++;
 			}
 		}
-- 
2.34.1

[PATCH v13 07/12] ACPI: x86: s2idle: Add more debugging for AMD constraints parsing

[Mario Limonciello]

While parsing the constraints show all the entries for the table
to aid with debugging other problems later.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * move location of the message to catch non-enabled constraints too
v9->v10:
 * split from other patches
---
 drivers/acpi/x86/s2idle.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 60835953ebfc4..87563337a4786 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -156,6 +156,13 @@ static void lpi_device_get_constraints_amd(void)
 					}
 				}
 
+				acpi_handle_debug(lps0_device_handle,
+						  "Name:%s, Enabled: %d, States: %d, MinDstate: %d\n",
+						  dev_info.name,
+						  dev_info.enabled,
+						  dev_info.function_states,
+						  dev_info.min_dstate);
+
 				if (!dev_info.enabled || !dev_info.name ||
 				    !dev_info.min_dstate)
 					continue;
@@ -164,9 +171,6 @@ static void lpi_device_get_constraints_amd(void)
 				if (ACPI_FAILURE(status))
 					continue;
 
-				acpi_handle_debug(lps0_device_handle,
-						  "Name:%s\n", dev_info.name);
-
 				list->min_dstate = dev_info.min_dstate;
 
 				lpi_constraints_table_size++;
-- 
2.34.1

[PATCH v13 08/12] ACPI: x86: s2idle: Add for_each_lpi_constraint() helper

[Mario Limonciello]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

We have one existing and one coming user of this macro.
Introduce a helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v11->v12:
 * New patch from Andy
---
 drivers/acpi/x86/s2idle.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 87563337a4786..1aa3cd5677bd8 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -94,6 +94,11 @@ static struct lpi_constraints *lpi_constraints_table;
 static int lpi_constraints_table_size;
 static int rev_id;
 
+#define for_each_lpi_constraint(entry)						\
+	for (int i = 0;								\
+	     entry = &lpi_constraints_table[i], i < lpi_constraints_table_size;	\
+	     i++)
+
 static void lpi_device_get_constraints_amd(void)
 {
 	union acpi_object *out_obj;
@@ -296,30 +301,29 @@ static void lpi_device_get_constraints(void)
 
 static void lpi_check_constraints(void)
 {
-	int i;
+	struct lpi_constraints *entry;
 
-	for (i = 0; i < lpi_constraints_table_size; ++i) {
-		acpi_handle handle = lpi_constraints_table[i].handle;
-		struct acpi_device *adev = acpi_fetch_acpi_dev(handle);
+	for_each_lpi_constraint(entry) {
+		struct acpi_device *adev = acpi_fetch_acpi_dev(entry->handle);
 
 		if (!adev)
 			continue;
 
-		acpi_handle_debug(handle,
+		acpi_handle_debug(entry->handle,
 			"LPI: required min power state:%s current power state:%s\n",
-			acpi_power_state_string(lpi_constraints_table[i].min_dstate),
+			acpi_power_state_string(entry->min_dstate),
 			acpi_power_state_string(adev->power.state));
 
 		if (!adev->flags.power_manageable) {
-			acpi_handle_info(handle, "LPI: Device not power manageable\n");
-			lpi_constraints_table[i].handle = NULL;
+			acpi_handle_info(entry->handle, "LPI: Device not power manageable\n");
+			entry->handle = NULL;
 			continue;
 		}
 
-		if (adev->power.state < lpi_constraints_table[i].min_dstate)
-			acpi_handle_info(handle,
+		if (adev->power.state < entry->min_dstate)
+			acpi_handle_info(entry->handle,
 				"LPI: Constraint not met; min power state:%s current power state:%s\n",
-				acpi_power_state_string(lpi_constraints_table[i].min_dstate),
+				acpi_power_state_string(entry->min_dstate),
 				acpi_power_state_string(adev->power.state));
 	}
 }
-- 
2.34.1

[PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Mario Limonciello]

Other parts of the kernel may use constraints information to make
decisions on what power state to put a device into.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * Drop checking for enabled, just return constraints
v11->v12:
 * Use for_each_lpi_constraint instead
 * use CONFIG_SUSPEND instead of CONFIG_ACPI_SLEEP
v9->v10:
 * split from other patches
 * kerneldoc fixes
 * move debug statement to this function
---
 drivers/acpi/x86/s2idle.c | 24 ++++++++++++++++++++++++
 include/linux/acpi.h      |  6 ++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 1aa3cd5677bd8..dd961f3a60577 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -299,6 +299,30 @@ static void lpi_device_get_constraints(void)
 	ACPI_FREE(out_obj);
 }
 
+/**
+ * acpi_get_lps0_constraint - get any LPS0 constraint for a device
+ * @dev: device to get constraints for
+ *
+ * Returns:
+ *  - the value for constraint.
+ *  - Otherwise, -ENODEV.
+ */
+int acpi_get_lps0_constraint(struct device *dev)
+{
+	struct lpi_constraints *entry;
+
+	for_each_lpi_constraint(entry) {
+		if (!device_match_acpi_handle(dev, entry->handle))
+			continue;
+		acpi_handle_debug(entry->handle,
+				  "ACPI device constraint: %d\n", entry->min_dstate);
+		return entry->min_dstate;
+	}
+	dev_dbg(dev, "No ACPI device constraint specified\n");
+
+	return -ENODEV;
+}
+
 static void lpi_check_constraints(void)
 {
 	struct lpi_constraints *entry;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index f1552c04a2856..6745535444c76 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1109,6 +1109,12 @@ struct acpi_s2idle_dev_ops {
 };
 int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
 void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
+int acpi_get_lps0_constraint(struct device *dev);
+#else /* CONFIG_SUSPEND && CONFIG_X86 */
+static inline int acpi_get_lps0_constraint(struct device *dev)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_SUSPEND && CONFIG_X86 */
 #ifndef CONFIG_IA64
 void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
-- 
2.34.1

[PATCH v13 10/12] PCI: ACPI: Add helper functions for converting ACPI <->PCI states

[Mario Limonciello]

Several functions do internal mappings in either direction. Add
helpers for this functionality.  No intended functional changes.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10->v11:
 * New patch
---
 drivers/pci/pci-acpi.c | 87 +++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a05350a4e49cb..b5b65cdfa3b8b 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -884,6 +884,56 @@ acpi_status pci_acpi_add_pm_notifier(struct acpi_device *dev,
 	return acpi_add_pm_notifier(dev, &pci_dev->dev, pci_acpi_wake_dev);
 }
 
+/**
+ * map_pci_to_acpi - map a PCI power state to an ACPI D-state
+ * @state: PCI power state to map
+ *
+ * Returns: Corresponding ACPI D-state, otherwise ACPI_STATE_UNKNOWN
+ */
+static u8 map_pci_to_acpi(pci_power_t state)
+{
+	switch (state) {
+	case PCI_D0:
+		return ACPI_STATE_D0;
+	case PCI_D1:
+		return ACPI_STATE_D1;
+	case PCI_D2:
+		return ACPI_STATE_D2;
+	case PCI_D3hot:
+		return ACPI_STATE_D3_HOT;
+	case PCI_D3cold:
+		return ACPI_STATE_D3_COLD;
+	}
+
+	return ACPI_STATE_UNKNOWN;
+}
+
+/**
+ * map_acpi_to_pci - map an ACPI D-state to a PCI power state
+ * @state: ACPI D-state to map
+ *
+ * Returns: Corresponding PCI power state, otherwise PCI_POWER_ERROR.
+ */
+static pci_power_t map_acpi_to_pci(int state)
+{
+	switch (state) {
+	case ACPI_STATE_D0:
+		return PCI_D0;
+	case ACPI_STATE_D1:
+		return PCI_D1;
+	case ACPI_STATE_D2:
+		return PCI_D2;
+	case ACPI_STATE_D3_HOT:
+		return PCI_D3hot;
+	case ACPI_STATE_D3_COLD:
+		return PCI_D3cold;
+	case ACPI_STATE_UNKNOWN:
+		return PCI_UNKNOWN;
+	}
+
+	return PCI_POWER_ERROR;
+}
+
 /*
  * _SxD returns the D-state with the highest power
  * (lowest D-state number) supported in the S-state "x".
@@ -919,19 +969,7 @@ pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
 	if (acpi_state < 0)
 		return PCI_POWER_ERROR;
 
-	switch (acpi_state) {
-	case ACPI_STATE_D0:
-		return PCI_D0;
-	case ACPI_STATE_D1:
-		return PCI_D1;
-	case ACPI_STATE_D2:
-		return PCI_D2;
-	case ACPI_STATE_D3_HOT:
-		return PCI_D3hot;
-	case ACPI_STATE_D3_COLD:
-		return PCI_D3cold;
-	}
-	return PCI_POWER_ERROR;
+	return map_acpi_to_pci(acpi_state);
 }
 
 static struct acpi_device *acpi_pci_find_companion(struct device *dev);
@@ -1056,13 +1094,6 @@ static void acpi_pci_config_space_access(struct pci_dev *dev, bool enable)
 int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
-	static const u8 state_conv[] = {
-		[PCI_D0] = ACPI_STATE_D0,
-		[PCI_D1] = ACPI_STATE_D1,
-		[PCI_D2] = ACPI_STATE_D2,
-		[PCI_D3hot] = ACPI_STATE_D3_HOT,
-		[PCI_D3cold] = ACPI_STATE_D3_COLD,
-	};
 	int error;
 
 	/* If the ACPI device has _EJ0, ignore the device */
@@ -1089,7 +1120,7 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 		acpi_pci_config_space_access(dev, false);
 	}
 
-	error = acpi_device_set_power(adev, state_conv[state]);
+	error = acpi_device_set_power(adev, map_pci_to_acpi(state));
 	if (error)
 		return error;
 
@@ -1111,23 +1142,11 @@ int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
-	static const pci_power_t state_conv[] = {
-		[ACPI_STATE_D0]      = PCI_D0,
-		[ACPI_STATE_D1]      = PCI_D1,
-		[ACPI_STATE_D2]      = PCI_D2,
-		[ACPI_STATE_D3_HOT]  = PCI_D3hot,
-		[ACPI_STATE_D3_COLD] = PCI_D3cold,
-	};
-	int state;
 
 	if (!adev || !acpi_device_power_manageable(adev))
 		return PCI_UNKNOWN;
 
-	state = adev->power.state;
-	if (state == ACPI_STATE_UNKNOWN)
-		return PCI_UNKNOWN;
-
-	return state_conv[state];
+	return map_acpi_to_pci(adev->power.state);
 }
 
 void acpi_pci_refresh_power_state(struct pci_dev *dev)
-- 
2.34.1

[PATCH v13 11/12] PCI: ACPI: Use device constraints to opt devices into D3 support

[Mario Limonciello]

In Windows, systems that support Modern Standby specify hardware
pre-conditions for the SoC to be able to achieve the lowest power state
by using 'device constraints' in a SOC specific "Power Engine
Plugin" (PEP) [1] [2].

Device constraints are specified in the return value for a _DSM of
a PNP0D80 device, and Linux enumerates the constraints during probing.

In cases that the existing logic for pci_bridge_d3_possible() may not
enable D3 support query for any constraints specified by the device.
If the constraints specify D3 support, then use D3 for the PCI device.

Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf [2]
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * Move back to PCI code
 * Trim commit message
v11->v12:
 * Adjust for dropped patch 8/9 from v11.
 * Update comment
v10->v11:
 * Fix kernel kernel build robot errors and various sparse warnings
   related to new handling of pci_power_t.
 * Use the new helpers introduced in previous patches
---
 drivers/pci/pci-acpi.c | 14 ++++++++++++++
 drivers/pci/pci.c      | 12 ++++++++++++
 drivers/pci/pci.h      |  5 +++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b5b65cdfa3b8b..bcc76e9d399c5 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1081,6 +1081,20 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 	return false;
 }
 
+/**
+ * acpi_pci_check_d3_constraint - Check if device specifies a D3 platform constraint
+ * @dev: PCI device to check
+ *
+ * This function checks if the platform specifies that a given PCI device should
+ * be put into D3 to satisfy a low power platform constraint
+ *
+ * Returns: TRUE if constraint for D3hot or deeper, FALSE otherwise.
+ */
+bool acpi_pci_check_d3_constraint(struct pci_dev *dev)
+{
+	return acpi_get_lps0_constraint(&dev->dev) >= ACPI_STATE_D3_HOT;
+}
+
 static void acpi_pci_config_space_access(struct pci_dev *dev, bool enable)
 {
 	int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 051e88ee64c63..0fc8d35154f97 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1082,6 +1082,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
 	return acpi_pci_bridge_d3(dev);
 }
 
+static inline bool platform_check_d3_constraint(struct pci_dev *dev)
+{
+	if (pci_use_mid_pm())
+		return false;
+
+	return acpi_pci_check_d3_constraint(dev);
+}
+
 /**
  * pci_update_current_state - Read power state of given device and cache it
  * @dev: PCI device to handle.
@@ -3036,6 +3044,10 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (dmi_check_system(bridge_d3_blacklist))
 			return false;
 
+		/* the platform specifies in LPS0 constraints to use D3 */
+		if (platform_check_d3_constraint(bridge))
+			return true;
+
 		/*
 		 * It is safe to put Intel PCIe ports from 2015 or newer
 		 * to D3.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a4c3974340576..ac06c781a9b7c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -707,6 +707,7 @@ void pci_set_acpi_fwnode(struct pci_dev *dev);
 int pci_dev_acpi_reset(struct pci_dev *dev, bool probe);
 bool acpi_pci_power_manageable(struct pci_dev *dev);
 bool acpi_pci_bridge_d3(struct pci_dev *dev);
+bool acpi_pci_check_d3_constraint(struct pci_dev *dev);
 int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state);
 pci_power_t acpi_pci_get_power_state(struct pci_dev *dev);
 void acpi_pci_refresh_power_state(struct pci_dev *dev);
@@ -731,6 +732,10 @@ static inline bool acpi_pci_bridge_d3(struct pci_dev *dev)
 {
 	return false;
 }
+static inline bool acpi_pci_check_d3_constraint(struct pci_dev *dev)
+{
+	return false;
+}
 static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 {
 	return -ENODEV;
-- 
2.34.1

[PATCH v13 12/12] PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

[Mario Limonciello]

Intel systems that need to have PCIe ports in D3 for low power idle
specify this by constraints on the ACPI PNP0D80 device. As this information
is queried, limit the DMI BIOS year check to stop at 2024. This will
allow future systems to rely on the constraints check to set up policy
like non-Intel systems do.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v12->v13:
 * New patch
---
 drivers/pci/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0fc8d35154f97..5b9e11e254f34 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3049,10 +3049,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 			return true;
 
 		/*
-		 * It is safe to put Intel PCIe ports from 2015 or newer
+		 * It is safe to put Intel PCIe ports from 2015 to 2024
 		 * to D3.
 		 */
 		if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
+		    dmi_get_bios_year() <= 2024 &&
 		    dmi_get_bios_year() >= 2015)
 			return true;
 		break;
-- 
2.34.1

Re: [PATCH v13 00/12] Fix wakeup problems on some AMD platforms

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 7:14 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Problems have been reported on AMD laptops with suspend/resume
> where particular root ports are put into D3 and then the system is unable
> to resume properly.
>
> This is caused by the policy used by the Linux kernel to put PCIe root ports
> into D3. This series adjusts the policy to be more conservative and only
> put root ports into D3 if the platform has indicated that it is necessary
> to do so.
>
> Andy Shevchenko (1):
>   ACPI: x86: s2idle: Add for_each_lpi_constraint() helper
>
> Mario Limonciello (11):
>   PCI: Only put Intel PCIe ports >= 2015 into D3
>   ACPI: Add comments to clarify some #ifdef statements
>   ACPI: Adjust #ifdef for *_lps0_dev use
>   ACPI: x86: s2idle: Post-increment variables when getting constraints
>   ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects
>   ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table
>   ACPI: x86: s2idle: Add more debugging for AMD constraints parsing
>   ACPI: x86: s2idle: Add a function to get constraints for a device
>   PCI: ACPI: Add helper functions for converting ACPI <->PCI states
>   PCI: ACPI: Use device constraints to opt devices into D3 support
>   PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

I think that patches [02-08/11] can be applied before the rest of the series.

In fact, I'd like to do that and expose a forward-only branch containing them.

Then, patches [1,09-11/11] will become a separate PCI/ACPI specific
series that should be somewhat easier to grasp.

What do you think?

Re: [PATCH v13 01/12] PCI: Only put Intel PCIe ports >= 2015 into D3

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 7:14 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> from modern machines (>=2015) are allowed to be put into D3.
>
> Iain reports that USB devices can't be used to wake a Lenovo Z13
> from suspend. This is because the PCIe root port has been put
> into D3 and AMD's platform can't handle USB devices waking in this
> case.
>
> This behavior is only reported on Linux. Comparing the behavior
> on Windows and Linux, Windows doesn't put the root ports into D3.
>
> To fix the issue without regressing existing Intel systems,
> limit the >=2015 check to only apply to Intel PCIe ports.
>
> Cc: stable@vger.kernel.org
> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> Reported-by: Iain Lane <iain@orangesquash.org.uk>
> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v12->v13:
>  * New patch
> ---
>  drivers/pci/pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60230da957e0c..051e88ee64c63 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3037,10 +3037,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>                         return false;
>
>                 /*
> -                * It should be safe to put PCIe ports from 2015 or newer
> +                * It is safe to put Intel PCIe ports from 2015 or newer
>                  * to D3.
>                  */

I would say "Allow Intel PCIe ports from 2015 onward to go into D3 to
achieve additional energy conservation on some platforms" without the
"It is safe" part that is kind of obvious (it wouldn't be done if it
were unsafe).

And analogously in patch [12/12].

> -               if (dmi_get_bios_year() >= 2015)
> +               if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
> +                   dmi_get_bios_year() >= 2015)
>                         return true;
>                 break;
>         }
> --
> 2.34.1
>

RE: [PATCH v13 01/12] PCI: Only put Intel PCIe ports >= 2015 into D3

[David Laight]

From: Rafael J. Wysocki
> Sent: Friday, August 18, 2023 9:12 AM
> 
> On Fri, Aug 18, 2023 at 7:14 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> > from modern machines (>=2015) are allowed to be put into D3.
> >
> > Iain reports that USB devices can't be used to wake a Lenovo Z13
> > from suspend. This is because the PCIe root port has been put
> > into D3 and AMD's platform can't handle USB devices waking in this
> > case.
> >
...
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 60230da957e0c..051e88ee64c63 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3037,10 +3037,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >                         return false;
> >
> >                 /*
> > -                * It should be safe to put PCIe ports from 2015 or newer
> > +                * It is safe to put Intel PCIe ports from 2015 or newer
> >                  * to D3.
> >                  */
> 
> I would say "Allow Intel PCIe ports from 2015 onward to go into D3 to
> achieve additional energy conservation on some platforms" without the
> "It is safe" part that is kind of obvious (it wouldn't be done if it
> were unsafe).

Just say why...

"Don't put root ports into D3 on non-Intel systems to avoid issues
with USB devices being unable to wake up some AMD based laptops."

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Other parts of the kernel may use constraints information to make
> decisions on what power state to put a device into.

I would say more about what the constraints are in this changelog, or
it becomes cryptic to people who are not familiar with the S0ix/LPS0
terminology.

Something like

"Some platforms may achieve additional energy conservation in deep
idle states provided that the states of all devices on the platform
meet specific requirements. In particular, they each may need to be
put into a specific minimum low-power state (or a deeper one) for this
purpose. The table of Low-Power S0 Idle (LPS0) constraints provided by
the platform firmware on some platforms using ACPI specifies that
minimum low-power state for each device.

That information may help to make decisions on what power state to put
a given device into when it is suspended, so introduce a function
allowing its caller to retrieve the minimum LPS0 low-power state for a
given device."

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v12->v13:
>  * Drop checking for enabled, just return constraints
> v11->v12:
>  * Use for_each_lpi_constraint instead
>  * use CONFIG_SUSPEND instead of CONFIG_ACPI_SLEEP
> v9->v10:
>  * split from other patches
>  * kerneldoc fixes
>  * move debug statement to this function
> ---
>  drivers/acpi/x86/s2idle.c | 24 ++++++++++++++++++++++++
>  include/linux/acpi.h      |  6 ++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index 1aa3cd5677bd8..dd961f3a60577 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -299,6 +299,30 @@ static void lpi_device_get_constraints(void)
>         ACPI_FREE(out_obj);
>  }
>
> +/**
> + * acpi_get_lps0_constraint - get any LPS0 constraint for a device

"get the minimum LPS0 low-power state for a device".

> + * @dev: device to get constraints for
> + *
> + * Returns:
> + *  - the value for constraint.
> + *  - Otherwise, -ENODEV.

Why not ACPI_STATE_UNKNOWN?

> + */
> +int acpi_get_lps0_constraint(struct device *dev)

I think that some overhead would be reduced below if this were taking
a struct acpi_device pointer as the argument.

> +{
> +       struct lpi_constraints *entry;
> +
> +       for_each_lpi_constraint(entry) {
> +               if (!device_match_acpi_handle(dev, entry->handle))
> +                       continue;
> +               acpi_handle_debug(entry->handle,
> +                                 "ACPI device constraint: %d\n", entry->min_dstate);
> +               return entry->min_dstate;
> +       }
> +       dev_dbg(dev, "No ACPI device constraint specified\n");
> +
> +       return -ENODEV;

ACPI_STATE_UNKNOWN?

> +}
> +
>  static void lpi_check_constraints(void)
>  {
>         struct lpi_constraints *entry;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index f1552c04a2856..6745535444c76 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1109,6 +1109,12 @@ struct acpi_s2idle_dev_ops {
>  };
>  int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
>  void acpi_unregister_lps0_dev(struct acpi_s2idle_dev_ops *arg);
> +int acpi_get_lps0_constraint(struct device *dev);
> +#else /* CONFIG_SUSPEND && CONFIG_X86 */
> +static inline int acpi_get_lps0_constraint(struct device *dev)
> +{
> +       return -ENODEV;
> +}
>  #endif /* CONFIG_SUSPEND && CONFIG_X86 */
>  #ifndef CONFIG_IA64
>  void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
> --
> 2.34.1
>

Re: [PATCH v13 01/12] PCI: Only put Intel PCIe ports >= 2015 into D3

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 10:21 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Rafael J. Wysocki
> > Sent: Friday, August 18, 2023 9:12 AM
> >
> > On Fri, Aug 18, 2023 at 7:14 AM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> > >
> > > commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > > changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> > > from modern machines (>=2015) are allowed to be put into D3.
> > >
> > > Iain reports that USB devices can't be used to wake a Lenovo Z13
> > > from suspend. This is because the PCIe root port has been put
> > > into D3 and AMD's platform can't handle USB devices waking in this
> > > case.
> > >
> ...
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 60230da957e0c..051e88ee64c63 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3037,10 +3037,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> > >                         return false;
> > >
> > >                 /*
> > > -                * It should be safe to put PCIe ports from 2015 or newer
> > > +                * It is safe to put Intel PCIe ports from 2015 or newer
> > >                  * to D3.
> > >                  */
> >
> > I would say "Allow Intel PCIe ports from 2015 onward to go into D3 to
> > achieve additional energy conservation on some platforms" without the
> > "It is safe" part that is kind of obvious (it wouldn't be done if it
> > were unsafe).
>
> Just say why...
>
> "Don't put root ports into D3 on non-Intel systems to avoid issues
> with USB devices being unable to wake up some AMD based laptops."

Well, both pieces of information are actually useful: Why it is done
on Intel systems in the first place and why it cannot be done on AMD
systems.

Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:

...

> > +int acpi_get_lps0_constraint(struct device *dev)
> 
> I think that some overhead would be reduced below if this were taking
> a struct acpi_device pointer as the argument.

Hmm... Either you need a pointer to handle, which involves pointer arithmetics
or something else. I would believe if you tell that ACPI handle should be passed,
but current suggestion is not obvious to me how it may help.

> > +{
> > +       struct lpi_constraints *entry;
> > +
> > +       for_each_lpi_constraint(entry) {
> > +               if (!device_match_acpi_handle(dev, entry->handle))

Here we retrieve handle...

> > +                       continue;
> > +               acpi_handle_debug(entry->handle,
> > +                                 "ACPI device constraint: %d\n", entry->min_dstate);
> > +               return entry->min_dstate;
> > +       }

> > +       dev_dbg(dev, "No ACPI device constraint specified\n");

...and here we are using dev directly (otherwise acpi_handle_dbg() should be used).

> > +       return -ENODEV;
> 
> ACPI_STATE_UNKNOWN?
> 
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 00/12] Fix wakeup problems on some AMD platforms

[Mario Limonciello]

On 8/18/2023 03:06, Rafael J. Wysocki wrote:
> On Fri, Aug 18, 2023 at 7:14 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> Problems have been reported on AMD laptops with suspend/resume
>> where particular root ports are put into D3 and then the system is unable
>> to resume properly.
>>
>> This is caused by the policy used by the Linux kernel to put PCIe root ports
>> into D3. This series adjusts the policy to be more conservative and only
>> put root ports into D3 if the platform has indicated that it is necessary
>> to do so.
>>
>> Andy Shevchenko (1):
>>    ACPI: x86: s2idle: Add for_each_lpi_constraint() helper
>>
>> Mario Limonciello (11):
>>    PCI: Only put Intel PCIe ports >= 2015 into D3
>>    ACPI: Add comments to clarify some #ifdef statements
>>    ACPI: Adjust #ifdef for *_lps0_dev use
>>    ACPI: x86: s2idle: Post-increment variables when getting constraints
>>    ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects
>>    ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table
>>    ACPI: x86: s2idle: Add more debugging for AMD constraints parsing
>>    ACPI: x86: s2idle: Add a function to get constraints for a device
>>    PCI: ACPI: Add helper functions for converting ACPI <->PCI states
>>    PCI: ACPI: Use device constraints to opt devices into D3 support
>>    PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024
> 
> I think that patches [02-08/11] can be applied before the rest of the series.
> 
> In fact, I'd like to do that and expose a forward-only branch containing them.
> 
> Then, patches [1,09-11/11] will become a separate PCI/ACPI specific
> series that should be somewhat easier to grasp.
> 
> What do you think?

Sure, this makes sense to me, but we'll still need to land it in the 
same maintainer's tree since there are new symbols.

I'll adjust for the other feedback and split it into two series.

Re: [PATCH v13 01/12] PCI: Only put Intel PCIe ports >= 2015 into D3

[Mario Limonciello]

On 8/18/2023 04:19, Rafael J. Wysocki wrote:
> On Fri, Aug 18, 2023 at 10:21 AM David Laight <David.Laight@aculab.com> wrote:
>>
>> From: Rafael J. Wysocki
>>> Sent: Friday, August 18, 2023 9:12 AM
>>>
>>> On Fri, Aug 18, 2023 at 7:14 AM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>>> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
>>>> from modern machines (>=2015) are allowed to be put into D3.
>>>>
>>>> Iain reports that USB devices can't be used to wake a Lenovo Z13
>>>> from suspend. This is because the PCIe root port has been put
>>>> into D3 and AMD's platform can't handle USB devices waking in this
>>>> case.
>>>>
>> ...
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index 60230da957e0c..051e88ee64c63 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -3037,10 +3037,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>>>>                          return false;
>>>>
>>>>                  /*
>>>> -                * It should be safe to put PCIe ports from 2015 or newer
>>>> +                * It is safe to put Intel PCIe ports from 2015 or newer
>>>>                   * to D3.
>>>>                   */
>>>
>>> I would say "Allow Intel PCIe ports from 2015 onward to go into D3 to
>>> achieve additional energy conservation on some platforms" without the
>>> "It is safe" part that is kind of obvious (it wouldn't be done if it
>>> were unsafe).
>>
>> Just say why...
>>
>> "Don't put root ports into D3 on non-Intel systems to avoid issues
>> with USB devices being unable to wake up some AMD based laptops."
> 
> Well, both pieces of information are actually useful: Why it is done
> on Intel systems in the first place and why it cannot be done on AMD
> systems.

Thanks guys, I'll add both in next version.

Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Mario Limonciello]

On 8/18/2023 05:47, Andy Shevchenko wrote:
> On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote:
>> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
>> <mario.limonciello@amd.com> wrote:
> 
> ...
> 
>>> +int acpi_get_lps0_constraint(struct device *dev)
>>
>> I think that some overhead would be reduced below if this were taking
>> a struct acpi_device pointer as the argument.
> 
> Hmm... Either you need a pointer to handle, which involves pointer arithmetics
> or something else. I would believe if you tell that ACPI handle should be passed,
> but current suggestion is not obvious to me how it may help.

To Rafael's point about overhead there are potentially "less" calls into 
acpi_get_lps0_constraint if it's a 'struct acpi_device' pointer because 
it won't be called by caller for any devices that don't have an ACPI 
companion.

> 
>>> +{
>>> +       struct lpi_constraints *entry;
>>> +
>>> +       for_each_lpi_constraint(entry) {
>>> +               if (!device_match_acpi_handle(dev, entry->handle))
> 
> Here we retrieve handle...
> 
>>> +                       continue;
>>> +               acpi_handle_debug(entry->handle,
>>> +                                 "ACPI device constraint: %d\n", entry->min_dstate);
>>> +               return entry->min_dstate;
>>> +       }
> 
>>> +       dev_dbg(dev, "No ACPI device constraint specified\n");
> 
> ...and here we are using dev directly (otherwise acpi_handle_dbg() should be used).

I'll just move the debugging statements into the caller of 
acpi_get_lps0_constraint().

> 
>>> +       return -ENODEV;
>>
>> ACPI_STATE_UNKNOWN?

Much better, thanks.

>>
>>> +}
> 

Re: [PATCH v13 01/12] PCI: Only put Intel PCIe ports >= 2015 into D3

[Kuppuswamy Sathyanarayanan]

Hi,

On 8/17/2023 10:13 PM, Mario Limonciello wrote:
> commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> changed pci_bridge_d3_possible() so that any vendor's PCIe ports
> from modern machines (>=2015) are allowed to be put into D3.
> 
> Iain reports that USB devices can't be used to wake a Lenovo Z13
> from suspend. This is because the PCIe root port has been put
> into D3 and AMD's platform can't handle USB devices waking in this
> case.
> 
> This behavior is only reported on Linux. Comparing the behavior
> on Windows and Linux, Windows doesn't put the root ports into D3.
> 
> To fix the issue without regressing existing Intel systems,
> limit the >=2015 check to only apply to Intel PCIe ports.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> Reported-by: Iain Lane <iain@orangesquash.org.uk>
> Closes: https://forums.lenovo.com/t5/Ubuntu/Z13-can-t-resume-from-suspend-with-external-USB-keyboard/m-p/5217121
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---

Looks good to me.

Reviewed-by:Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> v12->v13:
>  * New patch
> ---
>  drivers/pci/pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60230da957e0c..051e88ee64c63 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3037,10 +3037,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  			return false;
>  
>  		/*
> -		 * It should be safe to put PCIe ports from 2015 or newer
> +		 * It is safe to put Intel PCIe ports from 2015 or newer
>  		 * to D3.
>  		 */
> -		if (dmi_get_bios_year() >= 2015)
> +		if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
> +		    dmi_get_bios_year() >= 2015)
>  			return true;
>  		break;
>  	}

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 4:04 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 8/18/2023 05:47, Andy Shevchenko wrote:
> > On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote:
> >> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
> >> <mario.limonciello@amd.com> wrote:
> >
> > ...
> >
> >>> +int acpi_get_lps0_constraint(struct device *dev)
> >>
> >> I think that some overhead would be reduced below if this were taking
> >> a struct acpi_device pointer as the argument.
> >
> > Hmm... Either you need a pointer to handle, which involves pointer arithmetics
> > or something else. I would believe if you tell that ACPI handle should be passed,
> > but current suggestion is not obvious to me how it may help.
>
> To Rafael's point about overhead there are potentially "less" calls into
> acpi_get_lps0_constraint if it's a 'struct acpi_device' pointer because
> it won't be called by caller for any devices that don't have an ACPI
> companion.
>
> >
> >>> +{
> >>> +       struct lpi_constraints *entry;
> >>> +
> >>> +       for_each_lpi_constraint(entry) {
> >>> +               if (!device_match_acpi_handle(dev, entry->handle))
> >
> > Here we retrieve handle...

Which uses ACPI_HANDLE() to retrieve the companion ACPI handle for
dev. This checks dev against NULL and then passes it to
ACPI_COMPANION() which resolves to to_acpi_device_node() on the dev's
fwnode field and that involves an is_acpi_device_node() check and some
pointer arithmetic via container_of().  Of course, this needs to be
done in every iteration of the loop until a matching handle is found
(or not), and because dev is always the same, the result of this will
be the same every time. If this is not pointless overhead then I'm not
quite sure how to call it.

Now, if struct acpi_device *adev is passed as the argument, the check
above reduces to (adev->handle == entry->handle) which is much more
straightforward IMV.

> >
> >>> +                       continue;
> >>> +               acpi_handle_debug(entry->handle,
> >>> +                                 "ACPI device constraint: %d\n", entry->min_dstate);
> >>> +               return entry->min_dstate;
> >>> +       }
> >
> >>> +       dev_dbg(dev, "No ACPI device constraint specified\n");
> >
> > ...and here we are using dev directly (otherwise acpi_handle_dbg() should be used).
>
> I'll just move the debugging statements into the caller of
> acpi_get_lps0_constraint().

Yeah, that works too.

Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 05:38:15PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 18, 2023 at 4:04 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> > On 8/18/2023 05:47, Andy Shevchenko wrote:
> > > On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote:
> > >> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
> > >> <mario.limonciello@amd.com> wrote:

...

> > >> I think that some overhead would be reduced below if this were taking
> > >> a struct acpi_device pointer as the argument.
> > >
> > > Hmm... Either you need a pointer to handle, which involves pointer arithmetics
> > > or something else. I would believe if you tell that ACPI handle should be passed,
> > > but current suggestion is not obvious to me how it may help.
> >
> > To Rafael's point about overhead there are potentially "less" calls into
> > acpi_get_lps0_constraint if it's a 'struct acpi_device' pointer because
> > it won't be called by caller for any devices that don't have an ACPI
> > companion.
> >
> > >>> +       struct lpi_constraints *entry;
> > >>> +
> > >>> +       for_each_lpi_constraint(entry) {
> > >>> +               if (!device_match_acpi_handle(dev, entry->handle))
> > >
> > > Here we retrieve handle...
> 
> Which uses ACPI_HANDLE() to retrieve the companion ACPI handle for
> dev. This checks dev against NULL and then passes it to
> ACPI_COMPANION() which resolves to to_acpi_device_node() on the dev's
> fwnode field and that involves an is_acpi_device_node() check and some
> pointer arithmetic via container_of().  Of course, this needs to be
> done in every iteration of the loop until a matching handle is found
> (or not), and because dev is always the same, the result of this will
> be the same every time. If this is not pointless overhead then I'm not
> quite sure how to call it.
> 
> Now, if struct acpi_device *adev is passed as the argument, the check
> above reduces to (adev->handle == entry->handle) which is much more
> straightforward IMV.

Yes, this is fine, and if we move out dev_dbg() call, the suggestion makes
even more sense. I agree with your arguments.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 09/12] ACPI: x86: s2idle: Add a function to get constraints for a device

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 5:38 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Aug 18, 2023 at 4:04 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> >
> > On 8/18/2023 05:47, Andy Shevchenko wrote:
> > > On Fri, Aug 18, 2023 at 10:31:03AM +0200, Rafael J. Wysocki wrote:
> > >> On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
> > >> <mario.limonciello@amd.com> wrote:
> > >
> > > ...
> > >
> > >>> +int acpi_get_lps0_constraint(struct device *dev)
> > >>
> > >> I think that some overhead would be reduced below if this were taking
> > >> a struct acpi_device pointer as the argument.

Besides, I don't think that the constraints should be checked directly
from pci_bridge_d3_possible().

I'd rather check them from acpi_pci_bridge_d3() which knows the ACPI
companion of the given device already and can use it directly.

Re: [PATCH v13 11/12] PCI: ACPI: Use device constraints to opt devices into D3 support

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> In Windows, systems that support Modern Standby specify hardware
> pre-conditions for the SoC to be able to achieve the lowest power state
> by using 'device constraints' in a SOC specific "Power Engine
> Plugin" (PEP) [1] [2].

The connection between the device power states and the "PEP
constraints" is still rather unclear after reading the above.  I would
add something like

"For each device, the constraint is the minimum (shallowest) power
state in which the device can be for the platform to be still able to
achieve significant energy conservation in a system-wide low-power
idle configuration."

> Device constraints are specified in the return value for a _DSM of
> a PNP0D80 device, and Linux enumerates the constraints during probing.
>
> In cases that the existing logic for pci_bridge_d3_possible() may not
> enable D3 support query for any constraints specified by the device.
> If the constraints specify D3 support, then use D3 for the PCI device.

The above paragraph is not particularly clear IMO.  I would say
something like this instead:

"For PCI bridges (including PCIe ports), the constraints may be
regarded as an additional source of information regarding the power
state to put the device into when it is suspended.  In particular, if
the constraint for a given PCI bridge is D3hot, the platform regards
D3hot as a valid power state for the bridge and it is reasonable to
expect that there won't be any side effects caused by putting the
bridge into that power state.

Accordingly, take the low-power S0 idle (LPS0) constraints into
account when deciding whether or not to allow a given PCI bridge to be
put into D3."

> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [1]
> Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf [2]
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v12->v13:
>  * Move back to PCI code
>  * Trim commit message
> v11->v12:
>  * Adjust for dropped patch 8/9 from v11.
>  * Update comment
> v10->v11:
>  * Fix kernel kernel build robot errors and various sparse warnings
>    related to new handling of pci_power_t.
>  * Use the new helpers introduced in previous patches
> ---
>  drivers/pci/pci-acpi.c | 14 ++++++++++++++
>  drivers/pci/pci.c      | 12 ++++++++++++
>  drivers/pci/pci.h      |  5 +++++
>  3 files changed, 31 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b5b65cdfa3b8b..bcc76e9d399c5 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1081,6 +1081,20 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>         return false;
>  }
>
> +/**
> + * acpi_pci_check_d3_constraint - Check if device specifies a D3 platform constraint
> + * @dev: PCI device to check
> + *
> + * This function checks if the platform specifies that a given PCI device should
> + * be put into D3 to satisfy a low power platform constraint
> + *
> + * Returns: TRUE if constraint for D3hot or deeper, FALSE otherwise.
> + */
> +bool acpi_pci_check_d3_constraint(struct pci_dev *dev)
> +{
> +       return acpi_get_lps0_constraint(&dev->dev) >= ACPI_STATE_D3_HOT;
> +}
> +
>  static void acpi_pci_config_space_access(struct pci_dev *dev, bool enable)
>  {
>         int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 051e88ee64c63..0fc8d35154f97 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1082,6 +1082,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>         return acpi_pci_bridge_d3(dev);
>  }
>
> +static inline bool platform_check_d3_constraint(struct pci_dev *dev)
> +{
> +       if (pci_use_mid_pm())
> +               return false;
> +
> +       return acpi_pci_check_d3_constraint(dev);
> +}

As I said elsewhere, the above looks redundant to me.  I would rather
make acpi_pci_bridge_d3() use the PEP constraints as an additional
source of information.

> +
>  /**
>   * pci_update_current_state - Read power state of given device and cache it
>   * @dev: PCI device to handle.
> @@ -3036,6 +3044,10 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>                 if (dmi_check_system(bridge_d3_blacklist))
>                         return false;
>
> +               /* the platform specifies in LPS0 constraints to use D3 */
> +               if (platform_check_d3_constraint(bridge))
> +                       return true;
> +
>                 /*
>                  * It is safe to put Intel PCIe ports from 2015 or newer
>                  * to D3.

Re: [PATCH v13 12/12] PCI: ACPI: Limit the Intel specific opt-in to D3 to 2024

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 7:15 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Intel systems that need to have PCIe ports in D3 for low power idle
> specify this by constraints on the ACPI PNP0D80 device. As this information
> is queried, limit the DMI BIOS year check to stop at 2024. This will
> allow future systems to rely on the constraints check to set up policy
> like non-Intel systems do.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v12->v13:
>  * New patch
> ---
>  drivers/pci/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0fc8d35154f97..5b9e11e254f34 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3049,10 +3049,11 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>                         return true;
>
>                 /*
> -                * It is safe to put Intel PCIe ports from 2015 or newer
> +                * It is safe to put Intel PCIe ports from 2015 to 2024
>                  * to D3.
>                  */
>                 if (bridge->vendor == PCI_VENDOR_ID_INTEL &&
> +                   dmi_get_bios_year() <= 2024 &&
>                     dmi_get_bios_year() >= 2015)

A minor nit: The above would be somewhat easier to follow if it is
written in a reverse order, that is

dmi_get_bios_year() >= 2015 && dmi_get_bios_year() <= 2024

and maybe call dmi_get_bios_year() once to avoid the redundant string parsing?

>                         return true;
>                 break;
> --