[PATCH v14.b 0/7] Export LPS0 constraints

[PATCH v14.b 0/7] Export LPS0 constraints

[Mario Limonciello]

LPS0 constraints can be useful to other parts of the kernel to make
decisions what state to put devices into.

In v14 this series has been split into 3 parts.
 part A: Immediate fix for AMD issue.
 part B: LPS0 export improvements
 part C: Long term solution for all vendors

This is part B, it can be applied and reviewed independently from part A.

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

Mario Limonciello (6):
  ACPI: Adjust #ifdef for *_lps0_dev use
  ACPI: x86: s2idle: Post-increment variables when getting constraints
  ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects
  ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table
  ACPI: x86: s2idle: Add more debugging for AMD constraints parsing
  ACPI: x86: s2idle: Add a function to get constraints for a device

 drivers/acpi/x86/s2idle.c | 96 +++++++++++++++++++++++++--------------
 include/linux/acpi.h      | 10 +++-
 2 files changed, 70 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH v14.b 1/7] ACPI: Adjust #ifdef for *_lps0_dev use

[Mario Limonciello]

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

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

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

[PATCH v14.b 2/7] ACPI: x86: s2idle: Post-increment variables when getting constraints

[Mario Limonciello]

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

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

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

[PATCH v14.b 3/7] ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects

[Mario Limonciello]

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

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

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

[PATCH v14.b 4/7] ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table

[Mario Limonciello]

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

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

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

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

[PATCH v14.b 5/7] ACPI: x86: s2idle: Add more debugging for AMD constraints parsing

[Mario Limonciello]

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

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

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

[PATCH v14.b 6/7] ACPI: x86: s2idle: Add for_each_lpi_constraint() helper

[Mario Limonciello]

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

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

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

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

[PATCH v14.b 7/7] 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>
---
v13->v14:
 * Use acpi_device instead
 * Drop debugging statements (will be used by caller instead)
 * Return ACPI_STATE_UNKNOWN on no enabled constraints
v12->v13:
 * Drop checking for enabled, just return constraints
v11->v12:
 * Use for_each_lpi_constraint instead
 * use CONFIG_SUSPEND instead of CONFIG_ACPI_SLEEP
v9->v10:
 * split from other patches
 * kerneldoc fixes
 * move debug statement to this function
---
 drivers/acpi/x86/s2idle.c | 21 +++++++++++++++++++++
 include/linux/acpi.h      |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 1aa3cd5677bd8..3019ca760ac1b 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -299,6 +299,27 @@ static void lpi_device_get_constraints(void)
 	ACPI_FREE(out_obj);
 }
 
+/**
+ * acpi_get_lps0_constraint - get the LPS0 constraint for a device
+ * @dev: device to get constraints for
+ *
+ * Returns:
+ *  - ACPI state value for constraint.
+ *  - Otherwise, ACPI_STATE_UNKNOWN.
+ */
+int acpi_get_lps0_constraint(struct acpi_device *adev)
+{
+	struct lpi_constraints *entry;
+
+	for_each_lpi_constraint(entry) {
+		if (adev->handle != entry->handle)
+			continue;
+		return entry->min_dstate;
+	}
+
+	return ACPI_STATE_UNKNOWN;
+}
+
 static void lpi_check_constraints(void)
 {
 	struct lpi_constraints *entry;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index f1552c04a2856..2212668ce60b7 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 acpi_device *adev);
+#else /* CONFIG_SUSPEND && CONFIG_X86 */
+static inline int acpi_get_lps0_constraint(struct device *dev)
+{
+	return ACPI_STATE_UNKNOWN;
+}
 #endif /* CONFIG_SUSPEND && CONFIG_X86 */
 #ifndef CONFIG_IA64
 void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
-- 
2.34.1

Re: [PATCH v14.b 7/7] ACPI: x86: s2idle: Add a function to get constraints for a device

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.5-rc7]
[cannot apply to next-20230824]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/ACPI-Adjust-ifdef-for-_lps0_dev-use/20230821-113922
base:   linus/master
patch link:    https://lore.kernel.org/r/20230818194007.27410-8-mario.limonciello%40amd.com
patch subject: [PATCH v14.b 7/7] ACPI: x86: s2idle: Add a function to get constraints for a device
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230825/202308251347.C2sdb28f-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308251347.C2sdb28f-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

>> drivers/acpi/x86/s2idle.c:311: warning: Function parameter or member 'adev' not described in 'acpi_get_lps0_constraint'
>> drivers/acpi/x86/s2idle.c:311: warning: Excess function parameter 'dev' description in 'acpi_get_lps0_constraint'


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

   301	
   302	/**
   303	 * acpi_get_lps0_constraint - get the LPS0 constraint for a device
   304	 * @dev: device to get constraints for
   305	 *
   306	 * Returns:
   307	 *  - ACPI state value for constraint.
   308	 *  - Otherwise, ACPI_STATE_UNKNOWN.
   309	 */
   310	int acpi_get_lps0_constraint(struct acpi_device *adev)
 > 311	{
   312		struct lpi_constraints *entry;
   313	
   314		for_each_lpi_constraint(entry) {
   315			if (adev->handle != entry->handle)
   316				continue;
   317			return entry->min_dstate;
   318		}
   319	
   320		return ACPI_STATE_UNKNOWN;
   321	}
   322	

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

Re: [PATCH v14.b 0/7] Export LPS0 constraints

[Rafael J. Wysocki]

On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> LPS0 constraints can be useful to other parts of the kernel to make
> decisions what state to put devices into.
>
> In v14 this series has been split into 3 parts.
>  part A: Immediate fix for AMD issue.
>  part B: LPS0 export improvements
>  part C: Long term solution for all vendors
>
> This is part B, it can be applied and reviewed independently from part A.
>
> Andy Shevchenko (1):
>   ACPI: x86: s2idle: Add for_each_lpi_constraint() helper
>
> Mario Limonciello (6):
>   ACPI: Adjust #ifdef for *_lps0_dev use
>   ACPI: x86: s2idle: Post-increment variables when getting constraints
>   ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects
>   ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table
>   ACPI: x86: s2idle: Add more debugging for AMD constraints parsing
>   ACPI: x86: s2idle: Add a function to get constraints for a device
>
>  drivers/acpi/x86/s2idle.c | 96 +++++++++++++++++++++++++--------------
>  include/linux/acpi.h      | 10 +++-
>  2 files changed, 70 insertions(+), 36 deletions(-)
>
> --

All applied as 6.6 material, but I rewrote the last patch my way, so
please see the result in the bleeding-edge branch and let me know if
there's anything wrong with it.

Thanks!

Re: [PATCH v14.b 0/7] Export LPS0 constraints

[Limonciello, Mario]

On 8/21/2023 1:31 PM, Rafael J. Wysocki wrote:
> On Fri, Aug 18, 2023 at 9:40 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>> LPS0 constraints can be useful to other parts of the kernel to make
>> decisions what state to put devices into.
>>
>> In v14 this series has been split into 3 parts.
>>   part A: Immediate fix for AMD issue.
>>   part B: LPS0 export improvements
>>   part C: Long term solution for all vendors
>>
>> This is part B, it can be applied and reviewed independently from part A.
>>
>> Andy Shevchenko (1):
>>    ACPI: x86: s2idle: Add for_each_lpi_constraint() helper
>>
>> Mario Limonciello (6):
>>    ACPI: Adjust #ifdef for *_lps0_dev use
>>    ACPI: x86: s2idle: Post-increment variables when getting constraints
>>    ACPI: x86: s2idle: Catch multiple ACPI_TYPE_PACKAGE objects
>>    ACPI: x86: s2idle: Fix a logic error parsing AMD constraints table
>>    ACPI: x86: s2idle: Add more debugging for AMD constraints parsing
>>    ACPI: x86: s2idle: Add a function to get constraints for a device
>>
>>   drivers/acpi/x86/s2idle.c | 96 +++++++++++++++++++++++++--------------
>>   include/linux/acpi.h      | 10 +++-
>>   2 files changed, 70 insertions(+), 36 deletions(-)
>>
>> --
> All applied as 6.6 material, but I rewrote the last patch my way, so
> please see the result in the bleeding-edge branch and let me know if
> there's anything wrong with it.
>
> Thanks!
Looks great, thanks!