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

[PATCH v11 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 device constraints for low power idle
are considered if the device is not power manageable by the platform.

Mario Limonciello (9):
  ACPI: Add comments to clarify some #ifdef statements
  ACPI: Adjust #ifdef for *_lps0_dev use
  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: Add a function to get constraints for a device
  PCI: ACPI: Add helper functions for converting ACPI <->PCI states
  PCI: Split PME state selection into a local static function
  PCI: ACPI: Use device constraints to decide PCI target state fallback
    policy

 drivers/acpi/x86/s2idle.c | 75 ++++++++++++++++++++---------
 drivers/pci/pci-acpi.c    | 99 +++++++++++++++++++++++++--------------
 drivers/pci/pci.c         | 53 ++++++++++++++-------
 drivers/pci/pci.h         |  5 ++
 include/linux/acpi.h      | 14 ++++--
 5 files changed, 171 insertions(+), 75 deletions(-)


base-commit: 13b9372068660fe4f7023f43081067376582ef3c
-- 
2.34.1

[PATCH v11 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

[PATCH v11 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_ACPI_SLEEP.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
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..13a0fca3539f0 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_ACPI_SLEEP) && 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_ACPI_SLEEP && CONFIG_X86 */
 #ifndef CONFIG_IA64
 void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
 #else
-- 
2.34.1

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

[Bjorn Helgaas]

On Wed, Aug 09, 2023 at 01:54:46PM -0500, 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_ACPI_SLEEP.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 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..13a0fca3539f0 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_ACPI_SLEEP) && defined(CONFIG_X86)

What's the connection to CONFIG_ACPI_SLEEP?

The acpi_register_lps0_dev() implementation in
drivers/acpi/x86/s2idle.c is under #ifdef CONFIG_SUSPEND (and
obviously s2idle.c is only compiled at all if CONFIG_X86).

Both callers (amd_pmc_probe() and thinkpad_acpi_module_init()) are
under #ifdef CONFIG_SUSPEND.

>  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_ACPI_SLEEP && CONFIG_X86 */
>  #ifndef CONFIG_IA64
>  void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
>  #else
> -- 
> 2.34.1
> 

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

[Mario Limonciello]

On 8/15/2023 13:28, Bjorn Helgaas wrote:
> On Wed, Aug 09, 2023 at 01:54:46PM -0500, 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_ACPI_SLEEP.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> 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..13a0fca3539f0 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_ACPI_SLEEP) && defined(CONFIG_X86)
> 
> What's the connection to CONFIG_ACPI_SLEEP?
> 
> The acpi_register_lps0_dev() implementation in
> drivers/acpi/x86/s2idle.c is under #ifdef CONFIG_SUSPEND (and
> obviously s2idle.c is only compiled at all if CONFIG_X86).
> 
> Both callers (amd_pmc_probe() and thinkpad_acpi_module_init()) are
> under #ifdef CONFIG_SUSPEND.
> 

My thought process was that s2idle.c is from drivers/acpi/x86 and only 
can be used in the context of ACPI enabled sleep.

But I could see the argument for CONFIG_SUSPEND being stronger.  I'll 
adjust and make sure the rest of the series works with CONFIG_SUSPEND.

Thanks,

>>   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_ACPI_SLEEP && CONFIG_X86 */
>>   #ifndef CONFIG_IA64
>>   void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
>>   #else
>> -- 
>> 2.34.1
>>

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

[Bjorn Helgaas]

On Tue, Aug 15, 2023 at 01:32:05PM -0500, Mario Limonciello wrote:
> On 8/15/2023 13:28, Bjorn Helgaas wrote:
> > On Wed, Aug 09, 2023 at 01:54:46PM -0500, 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_ACPI_SLEEP.
> > > 
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > 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..13a0fca3539f0 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_ACPI_SLEEP) && defined(CONFIG_X86)
> > 
> > What's the connection to CONFIG_ACPI_SLEEP?
> > 
> > The acpi_register_lps0_dev() implementation in
> > drivers/acpi/x86/s2idle.c is under #ifdef CONFIG_SUSPEND (and
> > obviously s2idle.c is only compiled at all if CONFIG_X86).
> > 
> > Both callers (amd_pmc_probe() and thinkpad_acpi_module_init()) are
> > under #ifdef CONFIG_SUSPEND.
> 
> My thought process was that s2idle.c is from drivers/acpi/x86 and only can
> be used in the context of ACPI enabled sleep.
> 
> But I could see the argument for CONFIG_SUSPEND being stronger.  I'll adjust
> and make sure the rest of the series works with CONFIG_SUSPEND.

It's very hard to verify that it's correct if the declaration is under
a different #ifdef than the implementation, whereas it's trivial if it
uses the same #ifdef.

Bjorn

[PATCH v11 3/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.

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

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index ce62e61a9605e..b566b3aa09388 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -129,12 +129,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;
@@ -149,26 +149,25 @@ 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;
-					}
+				if (list->min_dstate < 0) {
+					acpi_handle_debug(lps0_device_handle,
+							  "Incomplete constraint defined\n");
+					continue;
 				}
 				lpi_constraints_table_size++;
 			}
-- 
2.34.1

Re: [PATCH v11 3/9] ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table

[Bjorn Helgaas]

On Wed, Aug 09, 2023 at 01:54:47PM -0500, Mario Limonciello wrote:
> The constraints table should be resetting the `list` object
> after running through all of `info_obj` iterations.

This *looks* like it should fix a real problem (see below), but not
the one mentioned here.  But maybe I'm missing something because the
code that looks broken to me has been there since 146f1ed852a8 ("ACPI:
PM: s2idle: Add AMD support to handle _DSM"), which appeared in v5.11
in 2021.

> This adjusts whitespace as well as less code will now be included
> with each loop.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v9->v10:
>  * split from other patches
> ---
>  drivers/acpi/x86/s2idle.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index ce62e61a9605e..b566b3aa09388 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -129,12 +129,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;

I really have no idea what's going on here, but the code still looks
weird:

  1) Moving the "list" update:

       for (j = 0; j < package->package.count; ++j) {
     +   list = &lpi_constraints_table[lpi_constraints_table_size];
         for (k = 0; k < info_obj->package.count; ++k) {
     -     list = &lpi_constraints_table[lpi_constraints_table_size];
           ...
         }
         lpi_constraints_table_size++;
       }

     looks fine, but lpi_constraints_table_size isn't updated inside
     the "k" loop, and "list" isn't otherwise updated, so it shouldn't
     make any functional difference.

     HOWEVER, this patch also moves all the
     dev_info.enabled/name/min_dstate tests outside the "k" loop, so
     they're only done after the "k" loop has completed and they've
     all been set, which looks like it DOES fix a problem and is not
     mentioned in the commit log.

  2) Both lpi_device_get_constraints_amd() and
     lpi_device_get_constraints() overwrite the global
     lpi_constraints_table for each PNP0D80 device.  I assume there's
     some higher-level constraint that there can only be one such
     device, but the code doesn't enforce that.

  3) It's obvious that lpi_device_get_constraints() can only allocate
     lpi_constraints_table once per call.  It's NOT obvious for
     lpi_device_get_constraints_amd(), because the alloc is inside a
     loop:

       for (i = 0; i < out_obj->package.count; i++) {
         lpi_constraints_table = kcalloc(...);

     If the AMD _DSM returns more than one package, we'll leak all but
     the last one.

  4) Both lpi_device_get_constraints_amd() and
     lpi_device_get_constraints() use pre- and post-increment in the
     "for" loops for no apparent reason:

       for (i = 0; i < out_obj->package.count; i++)
         for (j = 0; j < package->package.count; ++j)
           for (k = 0; k < info_obj->package.count; ++k)  # AMD only

     I'd say they should all use the same (I vote for post-increment).

>  				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;
> @@ -149,26 +149,25 @@ 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;
> -					}
> +				if (list->min_dstate < 0) {
> +					acpi_handle_debug(lps0_device_handle,
> +							  "Incomplete constraint defined\n");
> +					continue;
>  				}
>  				lpi_constraints_table_size++;
>  			}
> -- 
> 2.34.1
> 

[PATCH v11 4/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 b566b3aa09388..91cd6f8b8ade0 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -160,7 +160,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 v11 5/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 | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 91cd6f8b8ade0..0c8101acc92ef 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 */
@@ -151,8 +152,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,6 +173,7 @@ static void lpi_device_get_constraints_amd(void)
 							  "Incomplete constraint defined\n");
 					continue;
 				}
+				list->enabled = dev_info.enabled;
 				lpi_constraints_table_size++;
 			}
 		}
@@ -235,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];
@@ -247,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];
@@ -284,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 v11 6/9] 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>
---
v9->v10:
 * split from other patches
 * kerneldoc fixes
 * move debug statement to this function
---
 drivers/acpi/x86/s2idle.c | 29 +++++++++++++++++++++++++++++
 include/linux/acpi.h      |  6 ++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 0c8101acc92ef..2a1a482f4803a 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -295,6 +295,35 @@ 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:
+ *  - If the constraint is enabled, the value for constraint.
+ *  - If the constraint is disabled, 0.
+ *  - Otherwise, -ENODEV.
+ */
+int acpi_get_lps0_constraint(struct device *dev)
+{
+	int i;
+
+	for (i = 0; i < lpi_constraints_table_size; ++i) {
+		static struct lpi_constraints *entry;
+		int val;
+
+		entry = &lpi_constraints_table[i];
+		if (!device_match_acpi_handle(dev, entry->handle))
+			continue;
+		val = entry->enabled ? entry->min_dstate : 0;
+		acpi_handle_debug(entry->handle,
+				  "ACPI device constraint: %d\n", val);
+		return val;
+	}
+
+	return -ENODEV;
+}
+
 static void lpi_check_constraints(void)
 {
 	int i;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 13a0fca3539f0..99458502a7510 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_ACPI_SLEEP && CONFIG_X86 */
+static inline int acpi_get_lps0_constraint(struct device *dev)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_ACPI_SLEEP && CONFIG_X86 */
 #ifndef CONFIG_IA64
 void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
-- 
2.34.1

Re: [PATCH v11 6/9] ACPI: x86: s2idle: Add a function to get constraints for a device

[Andy Shevchenko]

On Wed, Aug 09, 2023 at 01:54:50PM -0500, Mario Limonciello wrote:
> Other parts of the kernel may use constraints information to make
> decisions on what power state to put a device into.

...

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

> +	int i;
> +
> +	for (i = 0; i < lpi_constraints_table_size; ++i) {
> +		static struct lpi_constraints *entry;

static?!

Seems to me with the code in lpi_check_constraints() this actually can be first
(separate patch maybe with conversion of the mentioned user) transformed to

#define for_each_lpi_constraint(entry)
	for (int i = 0;
	     entry = &lpi_constraints_table[i], i < lpi_constraints_table_size;
	     i++)

> +		int val;
> +
> +		entry = &lpi_constraints_table[i];
> +		if (!device_match_acpi_handle(dev, entry->handle))
> +			continue;
> +		val = entry->enabled ? entry->min_dstate : 0;
> +		acpi_handle_debug(entry->handle,
> +				  "ACPI device constraint: %d\n", val);
> +		return val;
> +	}

So will become

	struct lpi_constraints *entry;
	int val;

	for_each_lpi_constraint(entry) {
		if (!device_match_acpi_handle(dev, entry->handle))
			continue;
		val = entry->enabled ? entry->min_dstate : 0;
		acpi_handle_debug(entry->handle,
				  "ACPI device constraint: %d\n", val);
		return val;
	}

> +	return -ENODEV;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 6/9] ACPI: x86: s2idle: Add a function to get constraints for a device

[Limonciello, Mario]

On 8/10/2023 10:47 AM, Andy Shevchenko wrote:
> On Wed, Aug 09, 2023 at 01:54:50PM -0500, Mario Limonciello wrote:
>> Other parts of the kernel may use constraints information to make
>> decisions on what power state to put a device into.
> 
> ...
> 
>> +int acpi_get_lps0_constraint(struct device *dev)
>> +{
> 
>> +	int i;
>> +
>> +	for (i = 0; i < lpi_constraints_table_size; ++i) {
>> +		static struct lpi_constraints *entry;
> 
> static?!

Whoops!  Good catch!

> 
> Seems to me with the code in lpi_check_constraints() this actually can be first
> (separate patch maybe with conversion of the mentioned user) transformed to
> 
> #define for_each_lpi_constraint(entry)
> 	for (int i = 0;
> 	     entry = &lpi_constraints_table[i], i < lpi_constraints_table_size;
> 	     i++)
> 
>> +		int val;
>> +
>> +		entry = &lpi_constraints_table[i];
>> +		if (!device_match_acpi_handle(dev, entry->handle))
>> +			continue;
>> +		val = entry->enabled ? entry->min_dstate : 0;
>> +		acpi_handle_debug(entry->handle,
>> +				  "ACPI device constraint: %d\n", val);
>> +		return val;
>> +	}
> 
> So will become
> 
> 	struct lpi_constraints *entry;
> 	int val;
> 
> 	for_each_lpi_constraint(entry) {
> 		if (!device_match_acpi_handle(dev, entry->handle))
> 			continue;
> 		val = entry->enabled ? entry->min_dstate : 0;
> 		acpi_handle_debug(entry->handle,
> 				  "ACPI device constraint: %d\n", val);
> 		return val;
> 	}
> 
>> +	return -ENODEV;
>> +}
> 

Much appreciated suggestion.  I'll incorporate this in the next version.

Re: [PATCH v11 6/9] ACPI: x86: s2idle: Add a function to get constraints for a device

[Andy Shevchenko]

On Thu, Aug 10, 2023 at 10:54:08AM -0500, Limonciello, Mario wrote:
> On 8/10/2023 10:47 AM, Andy Shevchenko wrote:
> > On Wed, Aug 09, 2023 at 01:54:50PM -0500, Mario Limonciello wrote:

...

> Much appreciated suggestion.  I'll incorporate this in the next version.

I just sent a patch into this thread. Feel free to incorporate.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v11 7/9] 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 v11 8/9] PCI: Split PME state selection into a local static function

[Mario Limonciello]

When a device is not power manageable by the platform, and not already
in a low power state pci_target_state() will find the deepest state
that PME is supported and use this to select the wakeup state.

Simplify this logic and split it out to a local function. No intended
functional changes.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10->v11:
 * New patch split from existing patch in v10
---
 drivers/pci/pci.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0c..693f4ca90452b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2660,6 +2660,20 @@ int pci_wake_from_d3(struct pci_dev *dev, bool enable)
 }
 EXPORT_SYMBOL(pci_wake_from_d3);
 
+/*
+ * Find the deepest state from which the device can generate
+ * PME#.
+ */
+static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
+{
+	pci_power_t state = PCI_D3hot;
+
+	while (state && !(dev->pme_support & (1 << state)))
+		state--;
+
+	return state;
+}
+
 /**
  * pci_target_state - find an appropriate low power state for a given PCI dev
  * @dev: PCI device
@@ -2701,21 +2715,8 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 	else if (!dev->pm_cap)
 		return PCI_D0;
 
-	if (wakeup && dev->pme_support) {
-		pci_power_t state = PCI_D3hot;
-
-		/*
-		 * Find the deepest state from which the device can generate
-		 * PME#.
-		 */
-		while (state && !(dev->pme_support & (1 << state)))
-			state--;
-
-		if (state)
-			return state;
-		else if (dev->pme_support & 1)
-			return PCI_D0;
-	}
+	if (wakeup && dev->pme_support)
+		return pci_get_wake_pme_state(dev);
 
 	return PCI_D3hot;
 }
-- 
2.34.1

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

[Andy Shevchenko]

On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:
> When a device is not power manageable by the platform, and not already
> in a low power state pci_target_state() will find the deepest state
> that PME is supported and use this to select the wakeup state.
> 
> Simplify this logic and split it out to a local function. No intended
> functional changes.

...

> +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
> +{
> +	pci_power_t state = PCI_D3hot;
> +
> +	while (state && !(dev->pme_support & (1 << state)))
> +		state--;
> +
> +	return state;

Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).

Basically it's something like

	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));

(but double check and test the logic).

> +}

...

Yeah, I see that is the existing code, perhaps amend it first?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

[Limonciello, Mario]

On 8/10/2023 11:21 AM, Andy Shevchenko wrote:
> On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:
>> When a device is not power manageable by the platform, and not already
>> in a low power state pci_target_state() will find the deepest state
>> that PME is supported and use this to select the wakeup state.
>>
>> Simplify this logic and split it out to a local function. No intended
>> functional changes.
> 
> ...
> 
>> +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
>> +{
>> +	pci_power_t state = PCI_D3hot;
>> +
>> +	while (state && !(dev->pme_support & (1 << state)))
>> +		state--;
>> +
>> +	return state;
> 
> Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).
> 
> Basically it's something like
> 
> 	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));
> 
> (but double check and test the logic).
> 
>> +}
> 
> ...
> 
> Yeah, I see that is the existing code, perhaps amend it first?
> 

Are you sure?  I actually double checked the sparse output using this 
command before I sent it.

make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' drivers/pci/pci.o

I didn't see anything related to the line numbers for this function.

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

[Andy Shevchenko]

On Thu, Aug 10, 2023 at 11:29:49AM -0500, Limonciello, Mario wrote:
> On 8/10/2023 11:21 AM, Andy Shevchenko wrote:
> > On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:

...

> > > +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
> > > +{
> > > +	pci_power_t state = PCI_D3hot;
> > > +
> > > +	while (state && !(dev->pme_support & (1 << state)))
> > > +		state--;
> > > +
> > > +	return state;
> > 
> > Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).
> > 
> > Basically it's something like
> > 
> > 	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));
> > 
> > (but double check and test the logic).
> > 
> > > +}
> > 
> > ...
> > 
> > Yeah, I see that is the existing code, perhaps amend it first?
> > 
> 
> Are you sure?

Yes.

Just the original code

drivers/pci/pci.c:2711:60: warning: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2712:30: warning: restricted pci_power_t degrades to integer

                /*
                 * Find the deepest state from which the device can generate
                 * PME#.
                 */
2711 ==>         while (state && !(dev->pme_support & (1 << state)))
2712 ==>                state--;

How is yours different?

> I actually double checked the sparse output using this
> command before I sent it.
> 
> make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' drivers/pci/pci.o
> 
> I didn't see anything related to the line numbers for this function.

Just in case...

$ gcc --version
gcc (Debian 12.3.0-5) 12.3.0

$ sparse --version
0.6.4 (Debian: 0.6.4-3)

$ make --version
GNU Make 4.3
Built for x86_64-pc-linux-gnu

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

[Limonciello, Mario]

On 8/11/2023 3:55 AM, Andy Shevchenko wrote:
> On Thu, Aug 10, 2023 at 11:29:49AM -0500, Limonciello, Mario wrote:
>> On 8/10/2023 11:21 AM, Andy Shevchenko wrote:
>>> On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:
> 
> ...
> 
>>>> +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
>>>> +{
>>>> +	pci_power_t state = PCI_D3hot;
>>>> +
>>>> +	while (state && !(dev->pme_support & (1 << state)))
>>>> +		state--;
>>>> +
>>>> +	return state;
>>>
>>> Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).
>>>
>>> Basically it's something like
>>>
>>> 	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));
>>>
>>> (but double check and test the logic).
>>>
>>>> +}
>>>
>>> ...
>>>
>>> Yeah, I see that is the existing code, perhaps amend it first?
>>>
>>
>> Are you sure?
> 
> Yes.
> 
> Just the original code
> 
> drivers/pci/pci.c:2711:60: warning: restricted pci_power_t degrades to integer
> drivers/pci/pci.c:2712:30: warning: restricted pci_power_t degrades to integer
> 
>                  /*
>                   * Find the deepest state from which the device can generate
>                   * PME#.
>                   */
> 2711 ==>         while (state && !(dev->pme_support & (1 << state)))
> 2712 ==>                state--;
> 
> How is yours different?
> 
>> I actually double checked the sparse output using this
>> command before I sent it.
>>
>> make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' drivers/pci/pci.o
>>
>> I didn't see anything related to the line numbers for this function.
> 
> Just in case...
> 
> $ gcc --version
> gcc (Debian 12.3.0-5) 12.3.0
> 
> $ sparse --version
> 0.6.4 (Debian: 0.6.4-3)
> 
> $ make --version
> GNU Make 4.3
> Built for x86_64-pc-linux-gnu
> 

Oh, I see why I wasn't observing it now.  Because it's inline code it's 
processed separately and isn't with the rest of the grouped output from 
2416 through 2922.

drivers/pci/pci.c:2679:52: sparse: warning: restricted pci_power_t 
degrades to integer
drivers/pci/pci.c:2680:22: sparse: warning: restricted pci_power_t 
degrades to integer

[PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

[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.

When devices are not considered power manageable; specs are ambiguous as
to what should happen.  In this situation Windows 11 leaves PCIe
ports in D0 while Linux puts them into D3 due to the above mentioned
commit.

In Windows systems that support Modern Standby specify hardware
pre-conditions for the SoC to achieve the lowest power state by device
constraints in a SOC specific "Power Engine Plugin" (PEP) [2] [3].
They can be marked as disabled or enabled and when enabled can specify
the minimum power state required for an ACPI device.

When it is ambiguous what should happen, adjust the logic for
pci_target_state() to check whether a device constraint is present
and enabled.
* If power manageable by ACPI use this to get to select target state
* If a device constraint is present but disabled then choose D0
* If a device constraint is present and enabled then use it
* If a device constraint is not present, then continue to existing
logic (if marked for wakeup use deepest state that PME works)
* If not marked for wakeup choose D3hot

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]
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>
---
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 | 12 ++++++++++++
 drivers/pci/pci.c      | 17 ++++++++++++++++-
 drivers/pci/pci.h      |  5 +++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b5b65cdfa3b8b..9f418ec87b6a5 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1081,6 +1081,18 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
 	return false;
 }
 
+/**
+ * acpi_pci_device_constraint - return any PCI power state constraints
+ * @dev: PCI device to check
+ *
+ * Returns: Any valid constraint specified by platform for a device.
+ * Otherwise PCI_POWER_ERROR.
+ */
+pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
+{
+	return map_acpi_to_pci(acpi_get_lps0_constraint(&dev->dev));
+}
+
 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 693f4ca90452b..567443726974b 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 pci_power_t platform_get_constraint(struct pci_dev *dev)
+{
+	if (pci_use_mid_pm())
+		return PCI_POWER_ERROR;
+
+	return acpi_pci_device_constraint(dev);
+}
+
 /**
  * pci_update_current_state - Read power state of given device and cache it
  * @dev: PCI device to handle.
@@ -2685,11 +2693,13 @@ static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
  */
 static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 {
+	pci_power_t state;
+
 	if (platform_pci_power_manageable(dev)) {
 		/*
 		 * Call the platform to find the target state for the device.
 		 */
-		pci_power_t state = platform_pci_choose_state(dev);
+		state = platform_pci_choose_state(dev);
 
 		switch (state) {
 		case PCI_POWER_ERROR:
@@ -2715,6 +2725,11 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 	else if (!dev->pm_cap)
 		return PCI_D0;
 
+	/* if platform indicates preferred state device constraint, use it */
+	state = platform_get_constraint(dev);
+	if (state != PCI_POWER_ERROR)
+		return state;
+
 	if (wakeup && dev->pme_support)
 		return pci_get_wake_pme_state(dev);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a4c3974340576..410fca4b88837 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);
+pci_power_t acpi_pci_device_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 pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
+{
+	return PCI_POWER_ERROR;
+}
 static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 {
 	return -ENODEV;
-- 
2.34.1

Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

[Bjorn Helgaas]

On Wed, Aug 09, 2023 at 01:54:53PM -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.
> 
> 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.
> 
> When devices are not considered power manageable; specs are ambiguous as
> to what should happen.  In this situation Windows 11 leaves PCIe
> ports in D0 while Linux puts them into D3 due to the above mentioned
> commit.

Why would we not use the same policy as Windows 11?

> In Windows systems that support Modern Standby specify hardware
> pre-conditions for the SoC to achieve the lowest power state by device
> constraints in a SOC specific "Power Engine Plugin" (PEP) [2] [3].
> They can be marked as disabled or enabled and when enabled can specify
> the minimum power state required for an ACPI device.
> 
> When it is ambiguous what should happen, adjust the logic for
> pci_target_state() to check whether a device constraint is present
> and enabled.
> * If power manageable by ACPI use this to get to select target state
> * If a device constraint is present but disabled then choose D0
> * If a device constraint is present and enabled then use it
> * If a device constraint is not present, then continue to existing
> logic (if marked for wakeup use deepest state that PME works)
> * If not marked for wakeup choose D3hot

Apparently this is based on constraints from the _DSM method of a
PNP0D80 device (and Intel x86 and AMD have different _DSM methods)?

Isn't this something that needs to be part of the ACPI spec?  I don't
see PNP0D80 mentioned there.  Obviously we also have ARM64 and other
arches now using ACPI, so I'm not really comfortable with these bits
scrounged from Microsoft and Intel white papers.  That seems hard to
maintain in the future, and this is hard enough now.

> 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]
> 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>
> ---
> 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 | 12 ++++++++++++
>  drivers/pci/pci.c      | 17 ++++++++++++++++-
>  drivers/pci/pci.h      |  5 +++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b5b65cdfa3b8b..9f418ec87b6a5 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1081,6 +1081,18 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>  	return false;
>  }
>  
> +/**
> + * acpi_pci_device_constraint - return any PCI power state constraints
> + * @dev: PCI device to check
> + *
> + * Returns: Any valid constraint specified by platform for a device.
> + * Otherwise PCI_POWER_ERROR.
> + */
> +pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
> +{
> +	return map_acpi_to_pci(acpi_get_lps0_constraint(&dev->dev));
> +}
> +
>  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 693f4ca90452b..567443726974b 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 pci_power_t platform_get_constraint(struct pci_dev *dev)
> +{
> +	if (pci_use_mid_pm())
> +		return PCI_POWER_ERROR;
> +
> +	return acpi_pci_device_constraint(dev);
> +}
> +
>  /**
>   * pci_update_current_state - Read power state of given device and cache it
>   * @dev: PCI device to handle.
> @@ -2685,11 +2693,13 @@ static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
>   */
>  static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>  {
> +	pci_power_t state;
> +
>  	if (platform_pci_power_manageable(dev)) {
>  		/*
>  		 * Call the platform to find the target state for the device.
>  		 */
> -		pci_power_t state = platform_pci_choose_state(dev);
> +		state = platform_pci_choose_state(dev);
>  
>  		switch (state) {
>  		case PCI_POWER_ERROR:
> @@ -2715,6 +2725,11 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>  	else if (!dev->pm_cap)
>  		return PCI_D0;
>  
> +	/* if platform indicates preferred state device constraint, use it */
> +	state = platform_get_constraint(dev);
> +	if (state != PCI_POWER_ERROR)
> +		return state;
> +
>  	if (wakeup && dev->pme_support)
>  		return pci_get_wake_pme_state(dev);
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index a4c3974340576..410fca4b88837 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);
> +pci_power_t acpi_pci_device_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 pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
> +{
> +	return PCI_POWER_ERROR;
> +}
>  static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>  {
>  	return -ENODEV;
> -- 
> 2.34.1
> 

Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

[Limonciello, Mario]

On 8/15/2023 6:48 PM, Bjorn Helgaas wrote:
> On Wed, Aug 09, 2023 at 01:54:53PM -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.
>>
>> 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.
>>
>> When devices are not considered power manageable; specs are ambiguous as
>> to what should happen.  In this situation Windows 11 leaves PCIe
>> ports in D0 while Linux puts them into D3 due to the above mentioned
>> commit.
> 
> Why would we not use the same policy as Windows 11?

That's what I'm aiming to do with my patch series.

> 
>> In Windows systems that support Modern Standby specify hardware
>> pre-conditions for the SoC to achieve the lowest power state by device
>> constraints in a SOC specific "Power Engine Plugin" (PEP) [2] [3].
>> They can be marked as disabled or enabled and when enabled can specify
>> the minimum power state required for an ACPI device.
>>
>> When it is ambiguous what should happen, adjust the logic for
>> pci_target_state() to check whether a device constraint is present
>> and enabled.
>> * If power manageable by ACPI use this to get to select target state
>> * If a device constraint is present but disabled then choose D0
>> * If a device constraint is present and enabled then use it
>> * If a device constraint is not present, then continue to existing
>> logic (if marked for wakeup use deepest state that PME works)
>> * If not marked for wakeup choose D3hot
> 
> Apparently this is based on constraints from the _DSM method of a
> PNP0D80 device (and Intel x86 and AMD have different _DSM methods)?
> 
> Isn't this something that needs to be part of the ACPI spec?  
 > I don't
 > see PNP0D80 mentioned there.

So PNP0D80 is a "Windows-compatible system power management controller"
See 
https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-management-namespace-objects#compatible-id-_cid

> Obviously we also have ARM64 and other
> arches now using ACPI

Let me quote a few select sections of [4] that I want to draw attention to:

"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.
...
For peripheral devices that aren't integrated into the SoC, Windows uses 
ACPI Device Power Management.
...
It is possible to, and some device stacks do, use ACPI Device Power 
Management alone, or in combination with the microPEP for on-SoC device 
power management.
"

[4] 
https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-power-management#device-power-management-in-windows

In Linux we call the device that supports PNP0D80 the "LPS0 device", but 
in Windows they call it uPEP.  In Windows the SOC vendor provides this 
driver and it supports the _DSM calls for the vendor.

As part of going into Modern Standby on Windows that uPEP driver 
orchestrates the state of the devices mentioned by constraints.

I'd like to think that's exactly what this patch is doing; it's giving 
the ability to select the target state for SOC specified constraints to 
the LPS0 driver.

Looking at the intertwined function calls again, I wonder if maybe it's 
better to move the constraint checking into pci_prepare_to_sleep().

That makes it explicitly clear that LPS0 constraints shouldn't be used 
for anything other than suspend rather than also having implications 
into runtime PM.

As for your comment for ARM64, I think if they use Windows uPEP 
constraints we can add an PNP0D80 compatible LPS0 driver for them too 
just the same.

 > so I'm not really comfortable with these bits
 > scrounged from Microsoft and Intel white papers.  That seems hard to
 > maintain in the future, and this is hard enough now.

Remember this all comes back to a PCI root port in the SOC being put 
into an unexpected D-state over suspend.  The device is not ACPI power 
manageable so *that behavior* is up to the OSPM and is not grounded in 
any ACPI or PCI spec.

TBH I think the Microsoft documentation is the best we're going to get 
here for this case.  To be compatible with UEFI firmware designed for 
Windows machines you need to adopt some Windows-isms.

If this was coreboot, I would tell the coreboot developers to mark this
root port as ACPI power manageable and adjust the _SxD and _SxW objects 
so that this root port couldn't get into D3.

>> 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]
>> 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>
>> ---
>> 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 | 12 ++++++++++++
>>   drivers/pci/pci.c      | 17 ++++++++++++++++-
>>   drivers/pci/pci.h      |  5 +++++
>>   3 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index b5b65cdfa3b8b..9f418ec87b6a5 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1081,6 +1081,18 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>>   	return false;
>>   }
>>   
>> +/**
>> + * acpi_pci_device_constraint - return any PCI power state constraints
>> + * @dev: PCI device to check
>> + *
>> + * Returns: Any valid constraint specified by platform for a device.
>> + * Otherwise PCI_POWER_ERROR.
>> + */
>> +pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
>> +{
>> +	return map_acpi_to_pci(acpi_get_lps0_constraint(&dev->dev));
>> +}
>> +
>>   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 693f4ca90452b..567443726974b 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 pci_power_t platform_get_constraint(struct pci_dev *dev)
>> +{
>> +	if (pci_use_mid_pm())
>> +		return PCI_POWER_ERROR;
>> +
>> +	return acpi_pci_device_constraint(dev);
>> +}
>> +
>>   /**
>>    * pci_update_current_state - Read power state of given device and cache it
>>    * @dev: PCI device to handle.
>> @@ -2685,11 +2693,13 @@ static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
>>    */
>>   static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>>   {
>> +	pci_power_t state;
>> +
>>   	if (platform_pci_power_manageable(dev)) {
>>   		/*
>>   		 * Call the platform to find the target state for the device.
>>   		 */
>> -		pci_power_t state = platform_pci_choose_state(dev);
>> +		state = platform_pci_choose_state(dev);
>>   
>>   		switch (state) {
>>   		case PCI_POWER_ERROR:
>> @@ -2715,6 +2725,11 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>>   	else if (!dev->pm_cap)
>>   		return PCI_D0;
>>   
>> +	/* if platform indicates preferred state device constraint, use it */
>> +	state = platform_get_constraint(dev);
>> +	if (state != PCI_POWER_ERROR)
>> +		return state;
>> +
>>   	if (wakeup && dev->pme_support)
>>   		return pci_get_wake_pme_state(dev);
>>   
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index a4c3974340576..410fca4b88837 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);
>> +pci_power_t acpi_pci_device_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 pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
>> +{
>> +	return PCI_POWER_ERROR;
>> +}
>>   static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>>   {
>>   	return -ENODEV;
>> -- 
>> 2.34.1
>>

Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

[Bjorn Helgaas]

[I see that you just posted a v12 that doesn't touch drivers/pci at
all.  I haven't looked at it yet, so maybe my questions/comments below
are no longer relevant.]

On Wed, Aug 16, 2023 at 07:57:52AM -0500, Limonciello, Mario wrote:
> On 8/15/2023 6:48 PM, Bjorn Helgaas wrote:
> > On Wed, Aug 09, 2023 at 01:54:53PM -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.
> > > 
> > > 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.
> > > 
> > > When devices are not considered power manageable; specs are ambiguous as
> > > to what should happen.  In this situation Windows 11 leaves PCIe
> > > ports in D0 while Linux puts them into D3 due to the above mentioned
> > > commit.
> > 
> > Why would we not use the same policy as Windows 11?
> 
> That's what I'm aiming to do with my patch series.

OK, help me out because I think I have a hint of how this works, but
I'm still really confused.  Here's the sort of commit log I envision
(but I know it's all wrong, so help me out):

  Iain reported that the Lenovo Thinkpad Z13 suspends but activity on
  plugged-in USB devices will not cause a resume.  The resume failed
  because the Root Port leading to the USB adapter was in D3hot, and
  ... maybe firmware can't identify the wakeup source?  I dunno?

  The Z13 supplies an ACPI PNP0D80 System Power Management Controller
  with a "Get Device Constraints" _DSM method that says the Root Port
  should not be put into a lower power state than D0 ??

[This is one thing that seems completely wrong.  The _DSM spec says
the device must be in the specified or DEEPER D-state to achieve the
lowest power platform idle state.  That seems backwards.]

  For power-manageable devices (those with _PS0 or _PR0, per ACPI
  r6.5, sec 7.3), platform_pci_choose_state() selects the low-power
  state based on the device's _SxW, _SxD, and _PRW methods.

  For non-power manageable devices (those without _PS0 or _PR0), ACPI
  doesn't help decide the low-power state.

[This confuses me too.  Even here pci_set_power_state() uses
pci_platform_power_transition(), i.e., ACPI, but I guess this only
uses _OFF/_ON and doesn't count as "power managed"?

Maybe that makes sense since _OFF/_ON are part of sec 7.2 ("Declaring
a Power Resource Object", which is separate from *7.3* ("Device Power
Management Objects") where _PR0, _PS0, _PRW, _SxD, _SxW, etc are.]

  However, extensions like the ACPI PNP0D80 System Power Management
  Controller device can provide constraints on the allowable low-power
  states.

  Add platform_get_constraint() so pci_target_state() can discover
  constraints from these extensions and avoid choosing a deeper
  low-power state than the platform allows.

[Again, this *seems* backwards to how that _DSM is documented.]

> > > In Windows systems that support Modern Standby specify hardware
> > > pre-conditions for the SoC to achieve the lowest power state by device
> > > constraints in a SOC specific "Power Engine Plugin" (PEP) [2] [3].
> > > They can be marked as disabled or enabled and when enabled can specify
> > > the minimum power state required for an ACPI device.
> > > 
> > > When it is ambiguous what should happen, adjust the logic for
> > > pci_target_state() to check whether a device constraint is present
> > > and enabled.
> > > * If power manageable by ACPI use this to get to select target state
> > > * If a device constraint is present but disabled then choose D0
> > > * If a device constraint is present and enabled then use it
> > > * If a device constraint is not present, then continue to existing
> > > logic (if marked for wakeup use deepest state that PME works)
> > > * If not marked for wakeup choose D3hot
> > 
> > Apparently this is based on constraints from the _DSM method of a
> > PNP0D80 device (and Intel x86 and AMD have different _DSM methods)?
> > 
> > Isn't this something that needs to be part of the ACPI spec?  I
> > don't see PNP0D80 mentioned there.
> 
> So PNP0D80 is a "Windows-compatible system power management controller"
> See https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-management-namespace-objects#compatible-id-_cid
> 
> > Obviously we also have ARM64 and other arches now using ACPI
> 
> Let me quote a few select sections of [4] that I want to draw attention to:
> 
> "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.
> ...
> For peripheral devices that aren't integrated into the SoC, Windows uses
> ACPI Device Power Management.
> ...
> It is possible to, and some device stacks do, use ACPI Device Power
> Management alone, or in combination with the microPEP for on-SoC device
> power management.
> "
> 
> [4] https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-power-management#device-power-management-in-windows
> 
> In Linux we call the device that supports PNP0D80 the "LPS0 device", but in
> Windows they call it uPEP.  In Windows the SOC vendor provides this driver
> and it supports the _DSM calls for the vendor.
> 
> As part of going into Modern Standby on Windows that uPEP driver
> orchestrates the state of the devices mentioned by constraints.
> 
> I'd like to think that's exactly what this patch is doing; it's giving the
> ability to select the target state for SOC specified constraints to the LPS0
> driver.
> 
> Looking at the intertwined function calls again, I wonder if maybe it's
> better to move the constraint checking into pci_prepare_to_sleep().
> 
> That makes it explicitly clear that LPS0 constraints shouldn't be used for
> anything other than suspend rather than also having implications into
> runtime PM.
> 
> As for your comment for ARM64, I think if they use Windows uPEP constraints
> we can add an PNP0D80 compatible LPS0 driver for them too just the same.
> 
> > so I'm not really comfortable with these bits scrounged from
> > Microsoft and Intel white papers.  That seems hard to maintain in
> > the future, and this is hard enough now.
> 
> Remember this all comes back to a PCI root port in the SOC being put
> into an unexpected D-state over suspend.  The device is not ACPI
> power manageable so *that behavior* is up to the OSPM and is not
> grounded in any ACPI or PCI spec.
> 
> TBH I think the Microsoft documentation is the best we're going to
> get here for this case.  To be compatible with UEFI firmware
> designed for Windows machines you need to adopt some Windows-isms.
> 
> If this was coreboot, I would tell the coreboot developers to mark
> this root port as ACPI power manageable and adjust the _SxD and _SxW
> objects so that this root port couldn't get into D3.
> 
> > > 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]

This covers the Intel PNP0D80 _DSM.  But
lpi_device_get_constraints_amd() implies that there's a similar but
different AMD version?  Is there a published spec for the AMD _DSM?

> > > 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>
> > > ---
> > > 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 | 12 ++++++++++++
> > >   drivers/pci/pci.c      | 17 ++++++++++++++++-
> > >   drivers/pci/pci.h      |  5 +++++
> > >   3 files changed, 33 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index b5b65cdfa3b8b..9f418ec87b6a5 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -1081,6 +1081,18 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
> > >   	return false;
> > >   }
> > > +/**
> > > + * acpi_pci_device_constraint - return any PCI power state constraints
> > > + * @dev: PCI device to check
> > > + *
> > > + * Returns: Any valid constraint specified by platform for a device.
> > > + * Otherwise PCI_POWER_ERROR.
> > > + */
> > > +pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
> > > +{
> > > +	return map_acpi_to_pci(acpi_get_lps0_constraint(&dev->dev));

The non-x86 acpi_get_lps0_constraint() stub returns -ENODEV, which
doesn't seem quite right because map_acpi_to_pci() assumes it gets
ACPI_STATE_D0, etc.

I think it happens to work out fine because if it gets something
that's not ACPI_STATE_*, map_acpi_to_pci() returns PCI_POWER_ERROR,
but that's unholy knowledge of the ACPI_STATE_* values.

> > > +}
> > > +
> > >   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 693f4ca90452b..567443726974b 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 pci_power_t platform_get_constraint(struct pci_dev *dev)
> > > +{
> > > +	if (pci_use_mid_pm())
> > > +		return PCI_POWER_ERROR;
> > > +
> > > +	return acpi_pci_device_constraint(dev);
> > > +}
> > > +
> > >   /**
> > >    * pci_update_current_state - Read power state of given device and cache it
> > >    * @dev: PCI device to handle.
> > > @@ -2685,11 +2693,13 @@ static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
> > >    */
> > >   static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
> > >   {
> > > +	pci_power_t state;
> > > +
> > >   	if (platform_pci_power_manageable(dev)) {
> > >   		/*
> > >   		 * Call the platform to find the target state for the device.
> > >   		 */
> > > -		pci_power_t state = platform_pci_choose_state(dev);
> > > +		state = platform_pci_choose_state(dev);
> > >   		switch (state) {
> > >   		case PCI_POWER_ERROR:
> > > @@ -2715,6 +2725,11 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
> > >   	else if (!dev->pm_cap)
> > >   		return PCI_D0;
> > > +	/* if platform indicates preferred state device constraint, use it */
> > > +	state = platform_get_constraint(dev);
> > > +	if (state != PCI_POWER_ERROR)
> > > +		return state;
> > > +
> > >   	if (wakeup && dev->pme_support)
> > >   		return pci_get_wake_pme_state(dev);
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index a4c3974340576..410fca4b88837 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);
> > > +pci_power_t acpi_pci_device_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 pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
> > > +{
> > > +	return PCI_POWER_ERROR;
> > > +}
> > >   static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> > >   {
> > >   	return -ENODEV;
> > > -- 
> > > 2.34.1
> > > 

Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

[Limonciello, Mario]

On 8/16/2023 5:38 PM, Bjorn Helgaas wrote:
> [I see that you just posted a v12 that doesn't touch drivers/pci at
> all.  I haven't looked at it yet, so maybe my questions/comments below
> are no longer relevant.]

I'm not married to either approach but I think that you'll like the v12 
approach better.

Let me try to answer your questions anyway though because I think 
they're still applicable for understanding of this issue.

> 
> On Wed, Aug 16, 2023 at 07:57:52AM -0500, Limonciello, Mario wrote:
>> On 8/15/2023 6:48 PM, Bjorn Helgaas wrote:
>>> On Wed, Aug 09, 2023 at 01:54:53PM -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.
>>>>
>>>> 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.
>>>>
>>>> When devices are not considered power manageable; specs are ambiguous as
>>>> to what should happen.  In this situation Windows 11 leaves PCIe
>>>> ports in D0 while Linux puts them into D3 due to the above mentioned
>>>> commit.
>>>
>>> Why would we not use the same policy as Windows 11?
>>
>> That's what I'm aiming to do with my patch series.
> 
> OK, help me out because I think I have a hint of how this works, but
> I'm still really confused.  Here's the sort of commit log I envision
> (but I know it's all wrong, so help me out):

I was intentionally trying to leave the actual problem out of the commit 
from your earlier feedback and just put it in the cover letter.

But if it's better to keep in the commit message I'll return those details.

> 
>    Iain reported that the Lenovo Thinkpad Z13 suspends but activity on
>    plugged-in USB devices will not cause a resume.  The resume failed
>    because the Root Port leading to the USB adapter was in D3hot, and
>    ... maybe firmware can't identify the wakeup source?  I dunno?
> 

I would just say due to the platform behavior.

>    The Z13 supplies an ACPI PNP0D80 System Power Management Controller
>    with a "Get Device Constraints" _DSM method that says the Root Port
>    should not be put into a lower power state than D0 ??
> 
> [This is one thing that seems completely wrong.  The _DSM spec says
> the device must be in the specified or DEEPER D-state to achieve the
> lowest power platform idle state.  That seems backwards.]
> 

Yeah I see the confusion here and this is why it all ties back the 
policy that Linux uses for deciding if something can use D3hot or not.

The constraint is "not enabled" which means the OS shouldn't be putting 
the device it into a low power state.

Windows without ACPI and without the uPEP constraints saying what to do 
just leaves the device alone at suspend.  I believe that's why we get D0 
in Windows.

But Linux decides that anything >=2015 it should support D3hot, and 
anything that isn't ACPI power manageable should use D3hot.

We can't just rip that out because there are a number of Intel platforms 
that don't ship with Windows and rely upon this specific behavior to get 
the SoC into a low power state.


So what I do instead is look at the constraint not being enabled and use 
that to decide to return it to D0 at suspend.


>    For power-manageable devices (those with _PS0 or _PR0, per ACPI
>    r6.5, sec 7.3), platform_pci_choose_state() selects the low-power
>    state based on the device's _SxW, _SxD, and _PRW methods.
> 
>    For non-power manageable devices (those without _PS0 or _PR0), ACPI
>    doesn't help decide the low-power state.
> 
> [This confuses me too.  Even here pci_set_power_state() uses
> pci_platform_power_transition(), i.e., ACPI, but I guess this only
> uses _OFF/_ON and doesn't count as "power managed"?
> 
> Maybe that makes sense since _OFF/_ON are part of sec 7.2 ("Declaring
> a Power Resource Object", which is separate from *7.3* ("Device Power
> Management Objects") where _PR0, _PS0, _PRW, _SxD, _SxW, etc are.]
> 
>    However, extensions like the ACPI PNP0D80 System Power Management
>    Controller device can provide constraints on the allowable low-power
>    states.
> 
>    Add platform_get_constraint() so pci_target_state() can discover
>    constraints from these extensions and avoid choosing a deeper
>    low-power state than the platform allows.
> 
> [Again, this *seems* backwards to how that _DSM is documented.]
> 
>>>> In Windows systems that support Modern Standby specify hardware
>>>> pre-conditions for the SoC to achieve the lowest power state by device
>>>> constraints in a SOC specific "Power Engine Plugin" (PEP) [2] [3].
>>>> They can be marked as disabled or enabled and when enabled can specify
>>>> the minimum power state required for an ACPI device.
>>>>
>>>> When it is ambiguous what should happen, adjust the logic for
>>>> pci_target_state() to check whether a device constraint is present
>>>> and enabled.
>>>> * If power manageable by ACPI use this to get to select target state
>>>> * If a device constraint is present but disabled then choose D0
>>>> * If a device constraint is present and enabled then use it
>>>> * If a device constraint is not present, then continue to existing
>>>> logic (if marked for wakeup use deepest state that PME works)
>>>> * If not marked for wakeup choose D3hot
>>>
>>> Apparently this is based on constraints from the _DSM method of a
>>> PNP0D80 device (and Intel x86 and AMD have different _DSM methods)?
>>>
>>> Isn't this something that needs to be part of the ACPI spec?  I
>>> don't see PNP0D80 mentioned there.
>>
>> So PNP0D80 is a "Windows-compatible system power management controller"
>> See https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-management-namespace-objects#compatible-id-_cid
>>
>>> Obviously we also have ARM64 and other arches now using ACPI
>>
>> Let me quote a few select sections of [4] that I want to draw attention to:
>>
>> "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.
>> ...
>> For peripheral devices that aren't integrated into the SoC, Windows uses
>> ACPI Device Power Management.
>> ...
>> It is possible to, and some device stacks do, use ACPI Device Power
>> Management alone, or in combination with the microPEP for on-SoC device
>> power management.
>> "
>>
>> [4] https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-power-management#device-power-management-in-windows
>>
>> In Linux we call the device that supports PNP0D80 the "LPS0 device", but in
>> Windows they call it uPEP.  In Windows the SOC vendor provides this driver
>> and it supports the _DSM calls for the vendor.
>>
>> As part of going into Modern Standby on Windows that uPEP driver
>> orchestrates the state of the devices mentioned by constraints.
>>
>> I'd like to think that's exactly what this patch is doing; it's giving the
>> ability to select the target state for SOC specified constraints to the LPS0
>> driver.
>>
>> Looking at the intertwined function calls again, I wonder if maybe it's
>> better to move the constraint checking into pci_prepare_to_sleep().
>>
>> That makes it explicitly clear that LPS0 constraints shouldn't be used for
>> anything other than suspend rather than also having implications into
>> runtime PM.
>>
>> As for your comment for ARM64, I think if they use Windows uPEP constraints
>> we can add an PNP0D80 compatible LPS0 driver for them too just the same.
>>
>>> so I'm not really comfortable with these bits scrounged from
>>> Microsoft and Intel white papers.  That seems hard to maintain in
>>> the future, and this is hard enough now.
>>
>> Remember this all comes back to a PCI root port in the SOC being put
>> into an unexpected D-state over suspend.  The device is not ACPI
>> power manageable so *that behavior* is up to the OSPM and is not
>> grounded in any ACPI or PCI spec.
>>
>> TBH I think the Microsoft documentation is the best we're going to
>> get here for this case.  To be compatible with UEFI firmware
>> designed for Windows machines you need to adopt some Windows-isms.
>>
>> If this was coreboot, I would tell the coreboot developers to mark
>> this root port as ACPI power manageable and adjust the _SxD and _SxW
>> objects so that this root port couldn't get into D3.
>>
>>>> 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]
> 
> This covers the Intel PNP0D80 _DSM.  But
> lpi_device_get_constraints_amd() implies that there's a similar but
> different AMD version?  Is there a published spec for the AMD _DSM?

AMD's _DSM doesn't have a public facing document today.

I'll try to get that solved, but for now referencing the Intel and 
Microsoft ones conveys the necessary message about how this stuff works.

> 
>>>> 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>
>>>> ---
>>>> 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 | 12 ++++++++++++
>>>>    drivers/pci/pci.c      | 17 ++++++++++++++++-
>>>>    drivers/pci/pci.h      |  5 +++++
>>>>    3 files changed, 33 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>>>> index b5b65cdfa3b8b..9f418ec87b6a5 100644
>>>> --- a/drivers/pci/pci-acpi.c
>>>> +++ b/drivers/pci/pci-acpi.c
>>>> @@ -1081,6 +1081,18 @@ bool acpi_pci_bridge_d3(struct pci_dev *dev)
>>>>    	return false;
>>>>    }
>>>> +/**
>>>> + * acpi_pci_device_constraint - return any PCI power state constraints
>>>> + * @dev: PCI device to check
>>>> + *
>>>> + * Returns: Any valid constraint specified by platform for a device.
>>>> + * Otherwise PCI_POWER_ERROR.
>>>> + */
>>>> +pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
>>>> +{
>>>> +	return map_acpi_to_pci(acpi_get_lps0_constraint(&dev->dev));
> 
> The non-x86 acpi_get_lps0_constraint() stub returns -ENODEV, which
> doesn't seem quite right because map_acpi_to_pci() assumes it gets
> ACPI_STATE_D0, etc.
> 
> I think it happens to work out fine because if it gets something
> that's not ACPI_STATE_*, map_acpi_to_pci() returns PCI_POWER_ERROR,
> but that's unholy knowledge of the ACPI_STATE_* values.

If the consensus is to back to patching pci.c/pci-acpi.c in a future 
revision I'll change this a bit.

Yes I was relying upon PCI_POWER_ERROR for non-x86/non-s2idle cases.

> 
>>>> +}
>>>> +
>>>>    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 693f4ca90452b..567443726974b 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 pci_power_t platform_get_constraint(struct pci_dev *dev)
>>>> +{
>>>> +	if (pci_use_mid_pm())
>>>> +		return PCI_POWER_ERROR;
>>>> +
>>>> +	return acpi_pci_device_constraint(dev);
>>>> +}
>>>> +
>>>>    /**
>>>>     * pci_update_current_state - Read power state of given device and cache it
>>>>     * @dev: PCI device to handle.
>>>> @@ -2685,11 +2693,13 @@ static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
>>>>     */
>>>>    static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>>>>    {
>>>> +	pci_power_t state;
>>>> +
>>>>    	if (platform_pci_power_manageable(dev)) {
>>>>    		/*
>>>>    		 * Call the platform to find the target state for the device.
>>>>    		 */
>>>> -		pci_power_t state = platform_pci_choose_state(dev);
>>>> +		state = platform_pci_choose_state(dev);
>>>>    		switch (state) {
>>>>    		case PCI_POWER_ERROR:
>>>> @@ -2715,6 +2725,11 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>>>>    	else if (!dev->pm_cap)
>>>>    		return PCI_D0;
>>>> +	/* if platform indicates preferred state device constraint, use it */
>>>> +	state = platform_get_constraint(dev);
>>>> +	if (state != PCI_POWER_ERROR)
>>>> +		return state;
>>>> +
>>>>    	if (wakeup && dev->pme_support)
>>>>    		return pci_get_wake_pme_state(dev);
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index a4c3974340576..410fca4b88837 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);
>>>> +pci_power_t acpi_pci_device_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 pci_power_t acpi_pci_device_constraint(struct pci_dev *dev)
>>>> +{
>>>> +	return PCI_POWER_ERROR;
>>>> +}
>>>>    static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>>>>    {
>>>>    	return -ENODEV;
>>>> -- 
>>>> 2.34.1
>>>>

Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

[Rafael J. Wysocki]

On Thu, Aug 17, 2023 at 3:26 AM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
>
> On 8/16/2023 5:38 PM, Bjorn Helgaas wrote:
> > [I see that you just posted a v12 that doesn't touch drivers/pci at
> > all.  I haven't looked at it yet, so maybe my questions/comments below
> > are no longer relevant.]
>
> I'm not married to either approach but I think that you'll like the v12
> approach better.
>
> Let me try to answer your questions anyway though because I think
> they're still applicable for understanding of this issue.
>
> >
> > On Wed, Aug 16, 2023 at 07:57:52AM -0500, Limonciello, Mario wrote:
> >> On 8/15/2023 6:48 PM, Bjorn Helgaas wrote:
> >>> On Wed, Aug 09, 2023 at 01:54:53PM -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.
> >>>>
> >>>> 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.
> >>>>
> >>>> When devices are not considered power manageable; specs are ambiguous as
> >>>> to what should happen.  In this situation Windows 11 leaves PCIe
> >>>> ports in D0 while Linux puts them into D3 due to the above mentioned
> >>>> commit.
> >>>
> >>> Why would we not use the same policy as Windows 11?
> >>
> >> That's what I'm aiming to do with my patch series.
> >
> > OK, help me out because I think I have a hint of how this works, but
> > I'm still really confused.  Here's the sort of commit log I envision
> > (but I know it's all wrong, so help me out):
>
> I was intentionally trying to leave the actual problem out of the commit
> from your earlier feedback and just put it in the cover letter.
>
> But if it's better to keep in the commit message I'll return those details.

It is.

If you make a change in order to address a specific problem, that
problem needs to be described in the changelog of the patch making
that change.

Anything else is more or less confusing IMO.

Re: [PATCH v11 9/9] PCI: ACPI: Use device constraints to decide PCI target state fallback policy

[Limonciello, Mario]

>> I was intentionally trying to leave the actual problem out of the commit
>> from your earlier feedback and just put it in the cover letter.
>>
>> But if it's better to keep in the commit message I'll return those details.
> 
> It is.
> 
> If you make a change in order to address a specific problem, that
> problem needs to be described in the changelog of the patch making
> that change.
> 
> Anything else is more or less confusing IMO.

Thanks. I'll write up a new commit message for v13 once I have some 
feedback on the approach of 'v11' vs the 'approach of 'v12'.