[PATCH v12 0/9] Fix wakeup problems on some AMD platforms

[PATCH v12 0/9] 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.

The issue boils down to the currently selected kernel policy for root port
behavior at suspend time:
0) If the machine is from 2015 or later
1) If a PCIe root port is power manageable by the platform then platform
   will be used to determine the power state of the root port at suspend.
2) If the PCIe root is not power manageable by the platform then the kernel
   will check if it was configured to wakeup.
3) If it was, then it will be put into the deepest state that supports
   wakeup from PME.
4) If it wasn't, then it will be put into D3hot.

This patch series adjusts it so that PCI device constraints for low power
idle are considered when the system is put into s2idle.

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

Mario Limonciello (8):
  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: Store if constraint is enabled
  ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

 drivers/acpi/x86/s2idle.c | 131 +++++++++++++++++++++++++-------------
 include/linux/acpi.h      |   8 +--
 2 files changed, 90 insertions(+), 49 deletions(-)


base-commit: 2ccdd1b13c591d306f0401d98dedc4bdcd02b421
-- 
2.34.1

[PATCH v12 1/9] 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 was wrong,
so fix it and add another comment to clarify another.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
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

Re: [PATCH v12 1/9] ACPI: Add comments to clarify some #ifdef statements

[Kuppuswamy Sathyanarayanan]

On 8/16/2023 1:41 PM, Mario Limonciello wrote:
> With nested #ifdef statements it's sometimes difficult to tell
> which code goes with which statement.  One comment was wrong,
> so fix it and add another comment to clarify another.

I would explain what is wrong clearly in the commit log. I see this file has
many #ifdefs without comments. Do you want to fix them all?

> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 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);

[PATCH v12 2/9] ACPI: Adjust #ifdef for *_lps0_dev use

[Mario Limonciello]

The #ifdef currently is guarded against CONFIG_X86, but these are
actually sleep related functions so they should be tied to
CONFIG_SUSPEND.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
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

Re: [PATCH v12 2/9] ACPI: Adjust #ifdef for *_lps0_dev use

[Kuppuswamy Sathyanarayanan]

On 8/16/2023 1:41 PM, Mario Limonciello wrote:
> The #ifdef currently is guarded against CONFIG_X86, but these are
> actually sleep related functions so they should be tied to
> CONFIG_SUSPEND.

Explain why #ifdef and functions you are talking about clearly in the 
commit log?

>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 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

[PATCH v12 3/9] ACPI: x86: s2idle: Post-increment variables when getting constraints

[Mario Limonciello]

There is no reason for the variables to be pre-incremented.
No intended functional changes.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 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

Re: [PATCH v12 3/9] ACPI: x86: s2idle: Post-increment variables when getting constraints

[Kuppuswamy Sathyanarayanan]

On 8/16/2023 1:41 PM, Mario Limonciello wrote:
> There is no reason for the variables to be pre-incremented.
> No intended functional changes.
>
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---

I think you need to explain bit more in commit log.  Otherwise, looks good.

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

>   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;

Re: [PATCH v12 3/9] ACPI: x86: s2idle: Post-increment variables when getting constraints

[Andy Shevchenko]

On Wed, Aug 16, 2023 at 07:42:19PM -0700, Kuppuswamy Sathyanarayanan wrote:
> On 8/16/2023 1:41 PM, Mario Limonciello wrote:

...

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

It's not the first time your tag gets broken. Can you fix it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12 3/9] ACPI: x86: s2idle: Post-increment variables when getting constraints

[Kuppuswamy Sathyanarayanan]

On 8/17/2023 3:10 AM, Andy Shevchenko wrote:
> On Wed, Aug 16, 2023 at 07:42:19PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> On 8/16/2023 1:41 PM, Mario Limonciello wrote:
> 
> ...
> 
>> Reviewed-by: Kuppuswamy Sathyanarayanan
>> <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> It's not the first time your tag gets broken. Can you fix it?
> 

Sorry, changed the system recently and did not re-configure the email
client settings. I hope it is fixed now.

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

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v12 3/9] ACPI: x86: s2idle: Post-increment variables when getting constraints

[Andy Shevchenko]

On Thu, Aug 17, 2023 at 08:28:09AM -0700, Kuppuswamy Sathyanarayanan wrote:
> On 8/17/2023 3:10 AM, Andy Shevchenko wrote:
> > On Wed, Aug 16, 2023 at 07:42:19PM -0700, Kuppuswamy Sathyanarayanan wrote:
> >> On 8/16/2023 1:41 PM, Mario Limonciello wrote:

...

> >> Reviewed-by: Kuppuswamy Sathyanarayanan
> >> <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > It's not the first time your tag gets broken. Can you fix it?
> > 
> 
> Sorry, changed the system recently and did not re-configure the email
> client settings. I hope it is fixed now.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Seems good to me, thank you!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v12 4/9] 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 v12 5/9] 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 | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 508decbac2986..290514c21bb90 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -135,12 +135,12 @@ static void lpi_device_get_constraints_amd(void)
 				struct lpi_constraints *list;
 				acpi_status status;
 
+				list = &lpi_constraints_table[lpi_constraints_table_size];
+				list->min_dstate = -EINVAL;
+
 				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 +155,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 v12 6/9] 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>
---
v9->v10:
 * split from other patches
---
 drivers/acpi/x86/s2idle.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 290514c21bb90..b8b3151215f0a 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -166,7 +166,11 @@ static void lpi_device_get_constraints_amd(void)
 					continue;
 
 				acpi_handle_debug(lps0_device_handle,
-						  "Name:%s\n", dev_info.name);
+						  "Name:%s, Enabled: %d, States: %d, MinDstate: %d\n",
+						  dev_info.name,
+						  dev_info.enabled,
+						  dev_info.function_states,
+						  dev_info.min_dstate);
 
 				list->min_dstate = dev_info.min_dstate;
 
-- 
2.34.1

[PATCH v12 7/9] ACPI: x86: s2idle: Store if constraint is enabled

[Mario Limonciello]

Constraints are currently only stored when enabled.  To enable
the ability to check if constraints are present they need to be
stored even if disabled.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v9->v10:
 * split from other patches
---
 drivers/acpi/x86/s2idle.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index b8b3151215f0a..826f2924d05fe 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -78,6 +78,7 @@ struct lpi_device_constraint {
 struct lpi_constraints {
 	acpi_handle handle;
 	int min_dstate;
+	bool enabled;
 };
 
 /* AMD Constraint package structure */
@@ -157,8 +158,7 @@ static void lpi_device_get_constraints_amd(void)
 					}
 				}
 
-				if (!dev_info.enabled || !dev_info.name ||
-				    !dev_info.min_dstate)
+				if (!dev_info.name)
 					continue;
 
 				status = acpi_get_handle(NULL, dev_info.name, &list->handle);
@@ -173,7 +173,7 @@ static void lpi_device_get_constraints_amd(void)
 						  dev_info.min_dstate);
 
 				list->min_dstate = dev_info.min_dstate;
-
+				list->enabled = dev_info.enabled;
 				lpi_constraints_table_size++;
 			}
 		}
@@ -236,7 +236,7 @@ static void lpi_device_get_constraints(void)
 			}
 		}
 
-		if (!info.enabled || !info.package || !info.name)
+		if (!info.package || !info.name)
 			continue;
 
 		constraint = &lpi_constraints_table[lpi_constraints_table_size];
@@ -248,7 +248,7 @@ static void lpi_device_get_constraints(void)
 		acpi_handle_debug(lps0_device_handle,
 				  "index:%d Name:%s\n", i, info.name);
 
-		constraint->min_dstate = -1;
+		constraint->min_dstate = -EINVAL;
 
 		for (j = 0; j < package_count; j++) {
 			union acpi_object *info_obj = &info.package[j];
@@ -285,7 +285,7 @@ static void lpi_device_get_constraints(void)
 					  "Incomplete constraint defined\n");
 			continue;
 		}
-
+		constraint->enabled = info.enabled;
 		lpi_constraints_table_size++;
 	}
 
-- 
2.34.1

[PATCH v12 8/9] 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 826f2924d05fe..8bda45579d44a 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -95,6 +95,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;
@@ -297,30 +302,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 v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Mario Limonciello]

Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
storing a value to the `bridge_d3` variable in the `struct pci_dev`
structure.

pci_power_manageable() uses this variable to indicate a PCIe port can
enter D3.
pci_pm_suspend_noirq() uses the return from pci_power_manageable() to
decide whether to try to put a device into its target state for a sleep
cycle via pci_prepare_to_sleep().

For devices that support D3, the target state is selected by this policy:
1. If platform_pci_power_manageable():
   Use platform_pci_choose_state()
2. If the device is armed for wakeup:
   Select the deepest D-state that supports a PME.
3. Else:
   Use D3hot.

Devices are considered power manageable by the platform when they have
one or more objects described in the table in section 7.3 of the ACPI 6.5
specification [1].

When devices are not considered power manageable; specs are ambiguous as
to what should happen. Linux puts PCIe ports into D3 due to the above
described policy.  Windows 11 uses the PoFX framework to let the an
SOC specific Power Engine Plugin (PEP) [2] [3] [4] to decide what to do
with those devices.

Specifically Microsoft documentation says:

```
Devices that are integrated into the SoC can be power-managed through
the Windows Power Framework (PoFx). These framework-integrated devices
are power-managed by PoFx through a SoC-specific power engine plug-in
(microPEP) that knows the specifics of the SoC's power and clock controls.
```

Effectively this causes PCIe root ports on a variety of AMD systems to be
put into D0 on Windows 11 but D3 on Linux.

Instead of only using constraints for debugging messages use them to
enforce that PCI devices have been put into the expected state when
the system is being put into s2idle.
* If a device constraint is present but disabled then choose D0.
* If a device constraint is present and enabled then use it.

Link: https://uefi.org/specs/ACPI/6.5/07_Power_and_Performance_Mgmt.html#device-power-management-objects [1]
Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/platform-design-for-modern-standby#low-power-core-silicon-cpu-soc-dram [2]
Link: https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf [3]
Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-power-management#device-power-management-in-windows [4]
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>
---
 drivers/acpi/x86/s2idle.c | 51 ++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 8bda45579d44a..5192a7147655d 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/pci.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/suspend.h>
@@ -300,28 +301,61 @@ static void lpi_device_get_constraints(void)
 	ACPI_FREE(out_obj);
 }
 
+static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
+{
+	pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
+
+	if (pdev->current_state == target)
+		return;
+
+	/* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
+	if (target == ACPI_STATE_D3_HOT &&
+	    (pdev->current_state == PCI_D3hot ||
+	     pdev->current_state == PCI_D3cold))
+		return;
+
+	if (pm_debug_messages_on)
+		acpi_handle_info(entry->handle,
+				 "LPI: PCI device in %s, not in %s\n",
+				 acpi_power_state_string(pdev->current_state),
+				 acpi_power_state_string(target));
+
+	/* don't try with things that PCI core hasn't touched */
+	if (pdev->current_state == PCI_UNKNOWN) {
+		entry->handle = NULL;
+		return;
+	}
+
+	pci_set_power_state(pdev, target);
+}
+
 static void lpi_check_constraints(void)
 {
 	struct lpi_constraints *entry;
 
 	for_each_lpi_constraint(entry) {
 		struct acpi_device *adev = acpi_fetch_acpi_dev(entry->handle);
+		struct device *dev;
 
 		if (!adev)
 			continue;
 
+		/* Check and adjust PCI devices explicitly */
+		dev = acpi_get_first_physical_node(adev);
+		if (dev && dev_is_pci(dev)) {
+			lpi_check_pci_dev(entry, to_pci_dev(dev));
+			continue;
+		}
+		if (!entry->enabled)
+			continue;
 		acpi_handle_debug(entry->handle,
 			"LPI: required min power state:%s current power state:%s\n",
 			acpi_power_state_string(entry->min_dstate),
 			acpi_power_state_string(adev->power.state));
 
-		if (!adev->flags.power_manageable) {
-			acpi_handle_info(entry->handle, "LPI: Device not power manageable\n");
-			entry->handle = NULL;
-			continue;
-		}
-
-		if (adev->power.state < entry->min_dstate)
+		if (pm_debug_messages_on &&
+		    adev->flags.power_manageable &&
+		    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(entry->min_dstate),
@@ -512,8 +546,7 @@ int acpi_s2idle_prepare_late(void)
 	if (!lps0_device_handle || sleep_no_lps0)
 		return 0;
 
-	if (pm_debug_messages_on)
-		lpi_check_constraints();
+	lpi_check_constraints();
 
 	/* Screen off */
 	if (lps0_dsm_func_mask > 0)
-- 
2.34.1

Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2ccdd1b13c591d306f0401d98dedc4bdcd02b421]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-Add-comments-to-clarify-some-ifdef-statements/20230817-061259
base:   2ccdd1b13c591d306f0401d98dedc4bdcd02b421
patch link:    https://lore.kernel.org/r/20230816204143.66281-10-mario.limonciello%40amd.com
patch subject: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices
config: i386-randconfig-i063-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171239.xQFwhccA-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171239.xQFwhccA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308171239.xQFwhccA-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/acpi/x86/s2idle.c:306:67: sparse: sparse: restricted pci_power_t degrades to integer
>> drivers/acpi/x86/s2idle.c:306:45: sparse: sparse: incorrect type in initializer (different base types) @@     expected restricted pci_power_t [usertype] target @@     got int @@
   drivers/acpi/x86/s2idle.c:306:45: sparse:     expected restricted pci_power_t [usertype] target
   drivers/acpi/x86/s2idle.c:306:45: sparse:     got int
   drivers/acpi/x86/s2idle.c:312:13: sparse: sparse: restricted pci_power_t degrades to integer
>> drivers/acpi/x86/s2idle.c:318:17: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected int state @@     got restricted pci_power_t [usertype] current_state @@
   drivers/acpi/x86/s2idle.c:318:17: sparse:     expected int state
   drivers/acpi/x86/s2idle.c:318:17: sparse:     got restricted pci_power_t [usertype] current_state
>> drivers/acpi/x86/s2idle.c:318:17: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected int state @@     got restricted pci_power_t [usertype] target @@
   drivers/acpi/x86/s2idle.c:318:17: sparse:     expected int state
   drivers/acpi/x86/s2idle.c:318:17: sparse:     got restricted pci_power_t [usertype] target
   drivers/acpi/x86/s2idle.c:522:13: sparse: sparse: restricted suspend_state_t degrades to integer
   drivers/acpi/x86/s2idle.c:522:33: sparse: sparse: restricted suspend_state_t degrades to integer

vim +306 drivers/acpi/x86/s2idle.c

   303	
   304	static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
   305	{
 > 306		pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
   307	
   308		if (pdev->current_state == target)
   309			return;
   310	
   311		/* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
   312		if (target == ACPI_STATE_D3_HOT &&
   313		    (pdev->current_state == PCI_D3hot ||
   314		     pdev->current_state == PCI_D3cold))
   315			return;
   316	
   317		if (pm_debug_messages_on)
 > 318			acpi_handle_info(entry->handle,
   319					 "LPI: PCI device in %s, not in %s\n",
   320					 acpi_power_state_string(pdev->current_state),
   321					 acpi_power_state_string(target));
   322	
   323		/* don't try with things that PCI core hasn't touched */
   324		if (pdev->current_state == PCI_UNKNOWN) {
   325			entry->handle = NULL;
   326			return;
   327		}
   328	
   329		pci_set_power_state(pdev, target);
   330	}
   331	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Bjorn Helgaas]

On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
> storing a value to the `bridge_d3` variable in the `struct pci_dev`
> structure.
> ...

> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
> +{
> +	pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
> +
> +	if (pdev->current_state == target)
> +		return;
> +
> +	/* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
> +	if (target == ACPI_STATE_D3_HOT &&

ACPI_STATE_D3_HOT is not a valid pci_power_t value.

> +	    (pdev->current_state == PCI_D3hot ||
> +	     pdev->current_state == PCI_D3cold))
> +		return;
> +
> +	if (pm_debug_messages_on)
> +		acpi_handle_info(entry->handle,
> +				 "LPI: PCI device in %s, not in %s\n",
> +				 acpi_power_state_string(pdev->current_state),
> +				 acpi_power_state_string(target));
> +
> +	/* don't try with things that PCI core hasn't touched */
> +	if (pdev->current_state == PCI_UNKNOWN) {
> +		entry->handle = NULL;
> +		return;
> +	}
> +
> +	pci_set_power_state(pdev, target);

It doesn't seem logical for a "check_constraints()" function that
takes no parameters and returns nothing to actively set the PCI power
state.

lpi_check_constraints() returns nothing, and from the fact that it was
previously only called when "pm_debug_messages_on", I infer that it
should have no side effects.

IMHO "lpi_check_constraints" is not a great name because "check"
doesn't suggest anything specific about what it does.
"dump_constraints()" -- fine.  "log_unmet_constraints()" -- fine
(seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
low power idle constraints for debug only"), which added it.

> +}
> +
>  static void lpi_check_constraints(void)
>  {
>  	struct lpi_constraints *entry;
>  
>  	for_each_lpi_constraint(entry) {
>  		struct acpi_device *adev = acpi_fetch_acpi_dev(entry->handle);
> +		struct device *dev;
>  
>  		if (!adev)
>  			continue;
>  
> +		/* Check and adjust PCI devices explicitly */
> +		dev = acpi_get_first_physical_node(adev);
> +		if (dev && dev_is_pci(dev)) {
> +			lpi_check_pci_dev(entry, to_pci_dev(dev));
> +			continue;
> +		}
> +		if (!entry->enabled)
> +			continue;
>  		acpi_handle_debug(entry->handle,
>  			"LPI: required min power state:%s current power state:%s\n",
>  			acpi_power_state_string(entry->min_dstate),
>  			acpi_power_state_string(adev->power.state));
>  
> -		if (!adev->flags.power_manageable) {
> -			acpi_handle_info(entry->handle, "LPI: Device not power manageable\n");
> -			entry->handle = NULL;
> -			continue;
> -		}
> -
> -		if (adev->power.state < entry->min_dstate)
> +		if (pm_debug_messages_on &&
> +		    adev->flags.power_manageable &&
> +		    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(entry->min_dstate),
> @@ -512,8 +546,7 @@ int acpi_s2idle_prepare_late(void)
>  	if (!lps0_device_handle || sleep_no_lps0)
>  		return 0;
>  
> -	if (pm_debug_messages_on)
> -		lpi_check_constraints();
> +	lpi_check_constraints();
>  
>  	/* Screen off */
>  	if (lps0_dsm_func_mask > 0)
> -- 
> 2.34.1
> 

Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Limonciello, Mario]

On 8/17/2023 2:25 PM, Bjorn Helgaas wrote:
> On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
>> storing a value to the `bridge_d3` variable in the `struct pci_dev`
>> structure.
>> ...
> 
>> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
>> +{
>> +	pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
>> +
>> +	if (pdev->current_state == target)
>> +		return;
>> +
>> +	/* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
>> +	if (target == ACPI_STATE_D3_HOT &&
> 
> ACPI_STATE_D3_HOT is not a valid pci_power_t value.

Based on this, kernel robot sparse complaints and your comments on v11's 
last patch I am going to split off to another function that returns the
pci_power_t state based upon the situation and better comment the reason 
for the D0 when not enabled.

> 
>> +	    (pdev->current_state == PCI_D3hot ||
>> +	     pdev->current_state == PCI_D3cold))
>> +		return;
>> +
>> +	if (pm_debug_messages_on)
>> +		acpi_handle_info(entry->handle,
>> +				 "LPI: PCI device in %s, not in %s\n",
>> +				 acpi_power_state_string(pdev->current_state),
>> +				 acpi_power_state_string(target));
>> +
>> +	/* don't try with things that PCI core hasn't touched */
>> +	if (pdev->current_state == PCI_UNKNOWN) {
>> +		entry->handle = NULL;
>> +		return;
>> +	}
>> +
>> +	pci_set_power_state(pdev, target);
> 
> It doesn't seem logical for a "check_constraints()" function that
> takes no parameters and returns nothing to actively set the PCI power
> state.
> 
> lpi_check_constraints() returns nothing, and from the fact that it was
> previously only called when "pm_debug_messages_on", I infer that it
> should have no side effects.
> 
> IMHO "lpi_check_constraints" is not a great name because "check"
> doesn't suggest anything specific about what it does.
> "dump_constraints()" -- fine.  "log_unmet_constraints()" -- fine
> (seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
> low power idle constraints for debug only"), which added it.
> 

Great feedback, thanks.  I'm thinking to instead change it to:

lpi_enforce_constraints()

>> +}
>> +
>>   static void lpi_check_constraints(void)
>>   {
>>   	struct lpi_constraints *entry;
>>   
>>   	for_each_lpi_constraint(entry) {
>>   		struct acpi_device *adev = acpi_fetch_acpi_dev(entry->handle);
>> +		struct device *dev;
>>   
>>   		if (!adev)
>>   			continue;
>>   
>> +		/* Check and adjust PCI devices explicitly */
>> +		dev = acpi_get_first_physical_node(adev);
>> +		if (dev && dev_is_pci(dev)) {
>> +			lpi_check_pci_dev(entry, to_pci_dev(dev));
>> +			continue;
>> +		}
>> +		if (!entry->enabled)
>> +			continue;
>>   		acpi_handle_debug(entry->handle,
>>   			"LPI: required min power state:%s current power state:%s\n",
>>   			acpi_power_state_string(entry->min_dstate),
>>   			acpi_power_state_string(adev->power.state));
>>   
>> -		if (!adev->flags.power_manageable) {
>> -			acpi_handle_info(entry->handle, "LPI: Device not power manageable\n");
>> -			entry->handle = NULL;
>> -			continue;
>> -		}
>> -
>> -		if (adev->power.state < entry->min_dstate)
>> +		if (pm_debug_messages_on &&
>> +		    adev->flags.power_manageable &&
>> +		    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(entry->min_dstate),
>> @@ -512,8 +546,7 @@ int acpi_s2idle_prepare_late(void)
>>   	if (!lps0_device_handle || sleep_no_lps0)
>>   		return 0;
>>   
>> -	if (pm_debug_messages_on)
>> -		lpi_check_constraints();
>> +	lpi_check_constraints();
>>   
>>   	/* Screen off */
>>   	if (lps0_dsm_func_mask > 0)
>> -- 
>> 2.34.1
>>

Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 9:30 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
>
> On 8/17/2023 2:25 PM, Bjorn Helgaas wrote:
> > On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
> >> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
> >> storing a value to the `bridge_d3` variable in the `struct pci_dev`
> >> structure.
> >> ...
> >
> >> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
> >> +{
> >> +    pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
> >> +
> >> +    if (pdev->current_state == target)
> >> +            return;
> >> +
> >> +    /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
> >> +    if (target == ACPI_STATE_D3_HOT &&
> >
> > ACPI_STATE_D3_HOT is not a valid pci_power_t value.
>
> Based on this, kernel robot sparse complaints and your comments on v11's
> last patch I am going to split off to another function that returns the
> pci_power_t state based upon the situation and better comment the reason
> for the D0 when not enabled.
>
> >
> >> +        (pdev->current_state == PCI_D3hot ||
> >> +         pdev->current_state == PCI_D3cold))
> >> +            return;
> >> +
> >> +    if (pm_debug_messages_on)
> >> +            acpi_handle_info(entry->handle,
> >> +                             "LPI: PCI device in %s, not in %s\n",
> >> +                             acpi_power_state_string(pdev->current_state),
> >> +                             acpi_power_state_string(target));
> >> +
> >> +    /* don't try with things that PCI core hasn't touched */
> >> +    if (pdev->current_state == PCI_UNKNOWN) {
> >> +            entry->handle = NULL;
> >> +            return;
> >> +    }
> >> +
> >> +    pci_set_power_state(pdev, target);
> >
> > It doesn't seem logical for a "check_constraints()" function that
> > takes no parameters and returns nothing to actively set the PCI power
> > state.
> >
> > lpi_check_constraints() returns nothing, and from the fact that it was
> > previously only called when "pm_debug_messages_on", I infer that it
> > should have no side effects.
> >
> > IMHO "lpi_check_constraints" is not a great name because "check"
> > doesn't suggest anything specific about what it does.
> > "dump_constraints()" -- fine.  "log_unmet_constraints()" -- fine
> > (seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
> > low power idle constraints for debug only"), which added it.
> >
>
> Great feedback, thanks.  I'm thinking to instead change it to:
>
> lpi_enforce_constraints()

Don't even try to go this way, please.

Originally, the LPI constraints are there to indicate to Windows
whether or not it should attempt to enter Connected/Modern Standby.

Because Linux doesn't do Modern Standby, it doesn't use the LPI
constraints the way Windows does and it really shouldn't do that.

I think that the exercise here is to use the information from the
constraints list as an indication whether or not a given PCI Root Port
is supposed to be put into D3hot/cold on suspend-to-idle and this has
nothing to do with enforcement.

Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Limonciello, Mario]

On 8/17/2023 2:37 PM, Rafael J. Wysocki wrote:
> On Thu, Aug 17, 2023 at 9:30 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
>>
>>
>>
>> On 8/17/2023 2:25 PM, Bjorn Helgaas wrote:
>>> On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
>>>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
>>>> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
>>>> storing a value to the `bridge_d3` variable in the `struct pci_dev`
>>>> structure.
>>>> ...
>>>
>>>> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
>>>> +{
>>>> +    pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
>>>> +
>>>> +    if (pdev->current_state == target)
>>>> +            return;
>>>> +
>>>> +    /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
>>>> +    if (target == ACPI_STATE_D3_HOT &&
>>>
>>> ACPI_STATE_D3_HOT is not a valid pci_power_t value.
>>
>> Based on this, kernel robot sparse complaints and your comments on v11's
>> last patch I am going to split off to another function that returns the
>> pci_power_t state based upon the situation and better comment the reason
>> for the D0 when not enabled.
>>
>>>
>>>> +        (pdev->current_state == PCI_D3hot ||
>>>> +         pdev->current_state == PCI_D3cold))
>>>> +            return;
>>>> +
>>>> +    if (pm_debug_messages_on)
>>>> +            acpi_handle_info(entry->handle,
>>>> +                             "LPI: PCI device in %s, not in %s\n",
>>>> +                             acpi_power_state_string(pdev->current_state),
>>>> +                             acpi_power_state_string(target));
>>>> +
>>>> +    /* don't try with things that PCI core hasn't touched */
>>>> +    if (pdev->current_state == PCI_UNKNOWN) {
>>>> +            entry->handle = NULL;
>>>> +            return;
>>>> +    }
>>>> +
>>>> +    pci_set_power_state(pdev, target);
>>>
>>> It doesn't seem logical for a "check_constraints()" function that
>>> takes no parameters and returns nothing to actively set the PCI power
>>> state.
>>>
>>> lpi_check_constraints() returns nothing, and from the fact that it was
>>> previously only called when "pm_debug_messages_on", I infer that it
>>> should have no side effects.
>>>
>>> IMHO "lpi_check_constraints" is not a great name because "check"
>>> doesn't suggest anything specific about what it does.
>>> "dump_constraints()" -- fine.  "log_unmet_constraints()" -- fine
>>> (seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
>>> low power idle constraints for debug only"), which added it.
>>>
>>
>> Great feedback, thanks.  I'm thinking to instead change it to:
>>
>> lpi_enforce_constraints()
> 
> Don't even try to go this way, please.
> 
> Originally, the LPI constraints are there to indicate to Windows
> whether or not it should attempt to enter Connected/Modern Standby.
> 
> Because Linux doesn't do Modern Standby, it doesn't use the LPI
> constraints the way Windows does and it really shouldn't do that.
> 
> I think that the exercise here is to use the information from the
> constraints list as an indication whether or not a given PCI Root Port
> is supposed to be put into D3hot/cold on suspend-to-idle and this has
> nothing to do with enforcement.

What do you think about me making the changes to pci_prepare_to_sleep()?

Something like this:

@@ -2733,11 +2742,17 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
  {
         bool wakeup = device_may_wakeup(&dev->dev);
         pci_power_t target_state = pci_target_state(dev, wakeup);
+       pci_power_t constraint;
         int error;

         if (target_state == PCI_POWER_ERROR)
                 return -EIO;

+       /* if platform indicates device constraint for suspend, use it */
+       constraint = platform_check_constraint(dev, target_state);
+       if (constraint != PCI_POWER_ERROR)
+               target_state = constraint;
+
         pci_enable_wake(dev, target_state, wakeup);

         error = pci_set_power_state(dev, target_state);

Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 9:40 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
>
> On 8/17/2023 2:37 PM, Rafael J. Wysocki wrote:
> > On Thu, Aug 17, 2023 at 9:30 PM Limonciello, Mario
> > <mario.limonciello@amd.com> wrote:
> >>
> >>
> >>
> >> On 8/17/2023 2:25 PM, Bjorn Helgaas wrote:
> >>> On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
> >>>> Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> >>>> PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
> >>>> storing a value to the `bridge_d3` variable in the `struct pci_dev`
> >>>> structure.
> >>>> ...
> >>>
> >>>> +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
> >>>> +{
> >>>> +    pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
> >>>> +
> >>>> +    if (pdev->current_state == target)
> >>>> +            return;
> >>>> +
> >>>> +    /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
> >>>> +    if (target == ACPI_STATE_D3_HOT &&
> >>>
> >>> ACPI_STATE_D3_HOT is not a valid pci_power_t value.
> >>
> >> Based on this, kernel robot sparse complaints and your comments on v11's
> >> last patch I am going to split off to another function that returns the
> >> pci_power_t state based upon the situation and better comment the reason
> >> for the D0 when not enabled.
> >>
> >>>
> >>>> +        (pdev->current_state == PCI_D3hot ||
> >>>> +         pdev->current_state == PCI_D3cold))
> >>>> +            return;
> >>>> +
> >>>> +    if (pm_debug_messages_on)
> >>>> +            acpi_handle_info(entry->handle,
> >>>> +                             "LPI: PCI device in %s, not in %s\n",
> >>>> +                             acpi_power_state_string(pdev->current_state),
> >>>> +                             acpi_power_state_string(target));
> >>>> +
> >>>> +    /* don't try with things that PCI core hasn't touched */
> >>>> +    if (pdev->current_state == PCI_UNKNOWN) {
> >>>> +            entry->handle = NULL;
> >>>> +            return;
> >>>> +    }
> >>>> +
> >>>> +    pci_set_power_state(pdev, target);
> >>>
> >>> It doesn't seem logical for a "check_constraints()" function that
> >>> takes no parameters and returns nothing to actively set the PCI power
> >>> state.
> >>>
> >>> lpi_check_constraints() returns nothing, and from the fact that it was
> >>> previously only called when "pm_debug_messages_on", I infer that it
> >>> should have no side effects.
> >>>
> >>> IMHO "lpi_check_constraints" is not a great name because "check"
> >>> doesn't suggest anything specific about what it does.
> >>> "dump_constraints()" -- fine.  "log_unmet_constraints()" -- fine
> >>> (seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
> >>> low power idle constraints for debug only"), which added it.
> >>>
> >>
> >> Great feedback, thanks.  I'm thinking to instead change it to:
> >>
> >> lpi_enforce_constraints()
> >
> > Don't even try to go this way, please.
> >
> > Originally, the LPI constraints are there to indicate to Windows
> > whether or not it should attempt to enter Connected/Modern Standby.
> >
> > Because Linux doesn't do Modern Standby, it doesn't use the LPI
> > constraints the way Windows does and it really shouldn't do that.
> >
> > I think that the exercise here is to use the information from the
> > constraints list as an indication whether or not a given PCI Root Port
> > is supposed to be put into D3hot/cold on suspend-to-idle and this has
> > nothing to do with enforcement.
>
> What do you think about me making the changes to pci_prepare_to_sleep()?
>
> Something like this:
>
> @@ -2733,11 +2742,17 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
>   {
>          bool wakeup = device_may_wakeup(&dev->dev);
>          pci_power_t target_state = pci_target_state(dev, wakeup);
> +       pci_power_t constraint;
>          int error;
>
>          if (target_state == PCI_POWER_ERROR)
>                  return -EIO;
>
> +       /* if platform indicates device constraint for suspend, use it */
> +       constraint = platform_check_constraint(dev, target_state);
> +       if (constraint != PCI_POWER_ERROR)
> +               target_state = constraint;
> +
>          pci_enable_wake(dev, target_state, wakeup);
>
>          error = pci_set_power_state(dev, target_state);

I think that this is going to regress things in the field.

I agree with replacing and/or amending the dmi_get_bios_year() check
in pci_bridge_d3_possible() with the information from the constraints
list, but I don't agree with using it for pretty much anything else
that may affect functionality.

Re: [PATCH v12 9/9] ACPI: x86: s2idle: Enforce LPS0 constraints for PCI devices

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 9:25 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Aug 16, 2023 at 03:41:43PM -0500, Mario Limonciello wrote:
> > Since commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into D3 during suspend")
> > PCIe ports from modern machines (>=2015) are allowed to be put into D3 by
> > storing a value to the `bridge_d3` variable in the `struct pci_dev`
> > structure.
> > ...
>
> > +static void lpi_check_pci_dev(struct lpi_constraints *entry, struct pci_dev *pdev)
> > +{
> > +     pci_power_t target = entry->enabled ? entry->min_dstate : PCI_D0;
> > +
> > +     if (pdev->current_state == target)
> > +             return;
> > +
> > +     /* constraint of ACPI D3hot means PCI D3hot _or_ D3cold */
> > +     if (target == ACPI_STATE_D3_HOT &&
>
> ACPI_STATE_D3_HOT is not a valid pci_power_t value.
>
> > +         (pdev->current_state == PCI_D3hot ||
> > +          pdev->current_state == PCI_D3cold))
> > +             return;
> > +
> > +     if (pm_debug_messages_on)
> > +             acpi_handle_info(entry->handle,
> > +                              "LPI: PCI device in %s, not in %s\n",
> > +                              acpi_power_state_string(pdev->current_state),
> > +                              acpi_power_state_string(target));
> > +
> > +     /* don't try with things that PCI core hasn't touched */
> > +     if (pdev->current_state == PCI_UNKNOWN) {
> > +             entry->handle = NULL;
> > +             return;
> > +     }
> > +
> > +     pci_set_power_state(pdev, target);
>
> It doesn't seem logical for a "check_constraints()" function that
> takes no parameters and returns nothing to actively set the PCI power
> state.
>
> lpi_check_constraints() returns nothing, and from the fact that it was
> previously only called when "pm_debug_messages_on", I infer that it
> should have no side effects.

That's correct, it is not expected to have side effects.

> IMHO "lpi_check_constraints" is not a great name because "check"
> doesn't suggest anything specific about what it does.
> "dump_constraints()" -- fine.  "log_unmet_constraints()" -- fine
> (seems like the original intention of 726fb6b4f2a8 ("ACPI / PM: Check
> low power idle constraints for debug only"), which added it.

It seems that we are entering bikeshedding territory here ...