[PATCH v2 0/2] ACPI: Low power idle constraints check

[PATCH v2 0/2] ACPI: Low power idle constraints check

[Srinivas Pandruvada]

This patchset enables debugging of low power idle. This is done by checking
all the required constraints on wake.

v2
- Changes as suggested by Lukas Wunner.
- Using pm_debug_messages_on attribute to prevent constraints check to
save some cycles on wake.

Srinivas Pandruvada (2):
  PM / sleep: Export the setting of pm_debug_messages_on
  ACPI / Sleep: Check low power idle constraints for debug only

 drivers/acpi/sleep.c    | 162 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/suspend.h |   6 ++
 kernel/power/main.c     |   5 ++
 3 files changed, 173 insertions(+)

-- 
2.7.5

[PATCH v2 1/2] PM / sleep: Export the setting of pm_debug_messages_on

[Srinivas Pandruvada]

Added a function to export the value of pm_debug_messages_on, so that
other parts of the system can use this flag to enable/disable executing
PM debug code.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 include/linux/suspend.h | 6 ++++++
 kernel/power/main.c     | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 8c3b0b1..5d4b3a3 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -496,6 +496,10 @@ static inline void unlock_system_sleep(void) {}
 #ifdef CONFIG_PM_SLEEP_DEBUG
 extern bool pm_print_times_enabled;
 extern __printf(2, 3) void __pm_pr_dbg(bool defer, const char *fmt, ...);
+
+/* Return the value of pm_debug_messages_on */
+bool pm_debug_messages_enabled(void);
+
 #else
 #define pm_print_times_enabled	(false)
 
@@ -503,6 +507,8 @@ extern __printf(2, 3) void __pm_pr_dbg(bool defer, const char *fmt, ...);
 
 #define __pm_pr_dbg(defer, fmt, ...) \
 	no_printk(KERN_DEBUG fmt, ##__VA_ARGS__)
+
+static inline bool pm_debug_messages_enabled(void) { return false; }
 #endif
 
 #define pm_pr_dbg(fmt, ...) \
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 3074ea4..afb7f00 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -363,6 +363,11 @@ power_attr_ro(pm_wakeup_irq);
 
 static bool pm_debug_messages_on __read_mostly;
 
+bool pm_debug_messages_enabled(void)
+{
+	return pm_debug_messages_on;
+}
+
 static ssize_t pm_debug_messages_show(struct kobject *kobj,
 				      struct kobj_attribute *attr, char *buf)
 {
-- 
2.7.5

[PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Srinivas Pandruvada]

For SoC to achieve its lowest power platform idle state a set of hardware
preconditions must be met. These preconditions or constraints can be
obtained by issuing a device specific method (_DSM) with function "1".
Refer to the document provided in the link below.

Here during initialization (from attach() callback of LPS0 device), invoke
function 1 to get the device constraints. Each enabled constraint is
stored in a table.

The devices in this table are used to check whether they were in required
minimum state, while entering suspend. This check is done from platform
freeze wake() callback, only when /sys/power/pm_debug_messages attribute
is non zero.

If any constraint is not met and device is ACPI power managed then it
prints the device name to kernel logs.

Also if debug is enabled in acpi/sleep.c, the constraint table and state
of each device on wake is dumped in kernel logs.

Link: http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/acpi/sleep.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 162 insertions(+)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 2b881de..b3ef577 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -669,6 +669,7 @@ static const struct acpi_device_id lps0_device_ids[] = {
 
 #define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
 
+#define ACPI_LPS0_GET_DEVICE_CONSTRAINTS	1
 #define ACPI_LPS0_SCREEN_OFF	3
 #define ACPI_LPS0_SCREEN_ON	4
 #define ACPI_LPS0_ENTRY		5
@@ -680,6 +681,162 @@ static acpi_handle lps0_device_handle;
 static guid_t lps0_dsm_guid;
 static char lps0_dsm_func_mask;
 
+/* Device constraint entry structure */
+struct lpi_device_info {
+	char *name;
+	int enabled;
+	union acpi_object *package;
+};
+
+/* Constraint package structure */
+struct lpi_device_constraint {
+	int uid;
+	int min_dstate;
+	int function_states;
+};
+
+struct lpi_constraints {
+	char *name;
+	int min_dstate;
+};
+
+static struct lpi_constraints *lpi_constraints_table;
+static int lpi_constraints_table_size;
+
+static void lpi_device_get_constraints(void)
+{
+	union acpi_object *out_obj;
+	int i;
+
+	out_obj = acpi_evaluate_dsm_typed(lps0_device_handle, &lps0_dsm_guid,
+					  1, ACPI_LPS0_GET_DEVICE_CONSTRAINTS,
+					  NULL, ACPI_TYPE_PACKAGE);
+
+	acpi_handle_debug(lps0_device_handle, "_DSM function 1 eval %s\n",
+			  out_obj ? "successful" : "failed");
+
+	if (!out_obj)
+		return;
+
+	lpi_constraints_table = kcalloc(out_obj->package.count,
+					sizeof(*lpi_constraints_table),
+					GFP_KERNEL);
+	if (!lpi_constraints_table)
+		goto free_acpi_buffer;
+
+	pr_debug("LPI: constraints dump begin\n");
+	for (i = 0; i < out_obj->package.count; i++) {
+		union acpi_object *package = &out_obj->package.elements[i];
+		struct lpi_device_info info = { };
+		int package_count = 0, j;
+
+		if (!package)
+			continue;
+
+		for (j = 0; j < package->package.count; ++j) {
+			union acpi_object *element =
+					&(package->package.elements[j]);
+
+			switch (element->type) {
+			case ACPI_TYPE_INTEGER:
+				info.enabled = element->integer.value;
+				break;
+			case ACPI_TYPE_STRING:
+				info.name = element->string.pointer;
+				break;
+			case ACPI_TYPE_PACKAGE:
+				package_count = element->package.count;
+				info.package = element->package.elements;
+				break;
+			}
+		}
+
+		if (!info.enabled || !info.package || !info.name)
+			continue;
+
+		lpi_constraints_table[lpi_constraints_table_size].name =
+						kstrdup(info.name, GFP_KERNEL);
+		if (!lpi_constraints_table[lpi_constraints_table_size].name)
+			goto free_constraints;
+
+		pr_debug("index:%d Name:%s\n", i, info.name);
+
+		for (j = 0; j < package_count; ++j) {
+			union acpi_object *info_obj = &info.package[j];
+			union acpi_object *cnstr_pkg;
+			union acpi_object *obj;
+			struct lpi_device_constraint dev_info;
+
+			switch (info_obj->type) {
+			case ACPI_TYPE_INTEGER:
+				/* version */
+				break;
+			case ACPI_TYPE_PACKAGE:
+				if (info_obj->package.count < 2)
+					break;
+
+				cnstr_pkg = info_obj->package.elements;
+				obj = &cnstr_pkg[0];
+				dev_info.uid = obj->integer.value;
+				obj = &cnstr_pkg[1];
+				dev_info.min_dstate = obj->integer.value;
+				pr_debug("uid %d min_dstate %d\n",
+					 dev_info.uid,
+					 dev_info.min_dstate);
+				lpi_constraints_table[
+					lpi_constraints_table_size].min_dstate =
+						dev_info.min_dstate;
+				break;
+			}
+		}
+
+		lpi_constraints_table_size++;
+	}
+
+	pr_debug("LPI: constraints dump end\n");
+free_acpi_buffer:
+	ACPI_FREE(out_obj);
+	return;
+
+free_constraints:
+	ACPI_FREE(out_obj);
+	for (i = 0; i < lpi_constraints_table_size; ++i)
+		kfree(lpi_constraints_table[i].name);
+	kfree(lpi_constraints_table);
+	lpi_constraints_table_size = 0;
+}
+
+static void lpi_check_constraints(void)
+{
+	int i;
+
+	for (i = 0; i < lpi_constraints_table_size; ++i) {
+		acpi_handle handle;
+		struct acpi_device *adev;
+		int state, ret;
+
+		if (ACPI_FAILURE(acpi_get_handle(NULL,
+						 lpi_constraints_table[i].name,
+						 &handle)))
+			continue;
+
+		if (acpi_bus_get_device(handle, &adev))
+			continue;
+
+		ret = acpi_device_get_power(adev, &state);
+		if (!ret)
+			pr_debug("LPI: %s required min power state %d, current power state %d, real power state %d\n",
+				 lpi_constraints_table[i].name,
+				 lpi_constraints_table[i].min_dstate,
+				 adev->power.state, state);
+
+		if (adev->flags.power_manageable && adev->power.state <
+					lpi_constraints_table[i].min_dstate)
+			pr_info("LPI: Constraint [%s] not matched\n",
+				 lpi_constraints_table[i].name);
+	}
+}
+
 static void acpi_sleep_run_lps0_dsm(unsigned int func)
 {
 	union acpi_object *out_obj;
@@ -729,6 +886,9 @@ static int lps0_device_attach(struct acpi_device *adev,
 				  "_DSM function 0 evaluation failed\n");
 	}
 	ACPI_FREE(out_obj);
+
+	lpi_device_get_constraints();
+
 	return 0;
 }
 
@@ -773,6 +933,8 @@ static void acpi_freeze_wake(void)
 	 */
 	if (acpi_sci_irq_valid() &&
 	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) {
+		if (pm_debug_messages_enabled())
+			lpi_check_constraints();
 		pm_system_cancel_wakeup();
 		s2idle_wakeup = true;
 	}
-- 
2.7.5

Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Rafael J. Wysocki]

On Wednesday, August 9, 2017 12:41:15 AM CEST Srinivas Pandruvada wrote:
> For SoC to achieve its lowest power platform idle state a set of hardware
> preconditions must be met. These preconditions or constraints can be
> obtained by issuing a device specific method (_DSM) with function "1".
> Refer to the document provided in the link below.
> 
> Here during initialization (from attach() callback of LPS0 device), invoke
> function 1 to get the device constraints. Each enabled constraint is
> stored in a table.
> 
> The devices in this table are used to check whether they were in required
> minimum state, while entering suspend. This check is done from platform
> freeze wake() callback, only when /sys/power/pm_debug_messages attribute
> is non zero.
> 
> If any constraint is not met and device is ACPI power managed then it
> prints the device name to kernel logs.
> 
> Also if debug is enabled in acpi/sleep.c, the constraint table and state
> of each device on wake is dumped in kernel logs.
> 
> Link: http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/acpi/sleep.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 162 insertions(+)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 2b881de..b3ef577 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -669,6 +669,7 @@ static const struct acpi_device_id lps0_device_ids[] = {

[cut]

>  
> @@ -773,6 +933,8 @@ static void acpi_freeze_wake(void)
>  	 */
>  	if (acpi_sci_irq_valid() &&
>  	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) {
> +		if (pm_debug_messages_enabled())
> +			lpi_check_constraints();

I'm not sure why you only want to check the constraints when we do the
_cancel_wakeup() thing.

IMO the check is relevant regardless of whether or not the wakeup was
via ACPI.

>  		pm_system_cancel_wakeup();
>  		s2idle_wakeup = true;
>  	}
> 

Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Srinivas Pandruvada]

On Thu, 2017-08-10 at 01:17 +0200, Rafael J. Wysocki wrote:
> On Wednesday, August 9, 2017 12:41:15 AM CEST Srinivas Pandruvada
> wrote:
> > 
> > For SoC to achieve its lowest power platform idle state a set of
> > hardware
> > preconditions must be met. These preconditions or constraints can
> > be
> > obtained by issuing a device specific method (_DSM) with function
> > "1".
> > Refer to the document provided in the link below.
> > 
> > Here during initialization (from attach() callback of LPS0 device),
> > invoke
> > function 1 to get the device constraints. Each enabled constraint
> > is
> > stored in a table.
> > 
> > The devices in this table are used to check whether they were in
> > required
> > minimum state, while entering suspend. This check is done from
> > platform
> > freeze wake() callback, only when /sys/power/pm_debug_messages
> > attribute
> > is non zero.
> > 
> > If any constraint is not met and device is ACPI power managed then
> > it
> > prints the device name to kernel logs.
> > 
> > Also if debug is enabled in acpi/sleep.c, the constraint table and
> > state
> > of each device on wake is dumped in kernel logs.
> > 
> > Link: http://www.uefi.org/sites/default/files/resources/Intel_ACPI_
> > Low_Power_S0_Idle.pdf
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > .com>
> > ---
> >  drivers/acpi/sleep.c | 162
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 162 insertions(+)
> > 
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 2b881de..b3ef577 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -669,6 +669,7 @@ static const struct acpi_device_id
> > lps0_device_ids[] = {
> [cut]
> 
> > 
> >  
> > @@ -773,6 +933,8 @@ static void acpi_freeze_wake(void)
> >  	 */
> >  	if (acpi_sci_irq_valid() &&
> >  	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
> > {
> > +		if (pm_debug_messages_enabled())
> > +			lpi_check_constraints();
> I'm not sure why you only want to check the constraints when we do
> the
> _cancel_wakeup() thing.
> 
> IMO the check is relevant regardless of whether or not the wakeup was
> via ACPI.
OK. I will move out of this if block.

Thanks,
Srinivas
> 
> > 
> >  		pm_system_cancel_wakeup();
> >  		s2idle_wakeup = true;
> >  	}
> > 
> 

RE: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Mario.Limonciello]

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> Sent: Tuesday, August 8, 2017 5:41 PM
> To: rjw@rjwysocki.net; lenb@kernel.org
> Cc: linux-pm@vger.kernel.org; Limonciello, Mario <Mario_Limonciello@Dell.com>;
> linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; lukas@wunner.de;
> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Subject: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug
> only
> 
> For SoC to achieve its lowest power platform idle state a set of hardware
> preconditions must be met. These preconditions or constraints can be
> obtained by issuing a device specific method (_DSM) with function "1".
> Refer to the document provided in the link below.
> 
> Here during initialization (from attach() callback of LPS0 device), invoke
> function 1 to get the device constraints. Each enabled constraint is
> stored in a table.
> 
> The devices in this table are used to check whether they were in required
> minimum state, while entering suspend. This check is done from platform
> freeze wake() callback, only when /sys/power/pm_debug_messages attribute
> is non zero.
> 
> If any constraint is not met and device is ACPI power managed then it
> prints the device name to kernel logs.
> 
> Also if debug is enabled in acpi/sleep.c, the constraint table and state
> of each device on wake is dumped in kernel logs.
> 
> Link:
> http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.
> pdf
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/acpi/sleep.c | 162
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 162 insertions(+)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 2b881de..b3ef577 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -669,6 +669,7 @@ static const struct acpi_device_id lps0_device_ids[] = {
> 
>  #define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
> 
> +#define ACPI_LPS0_GET_DEVICE_CONSTRAINTS	1
>  #define ACPI_LPS0_SCREEN_OFF	3
>  #define ACPI_LPS0_SCREEN_ON	4
>  #define ACPI_LPS0_ENTRY		5
> @@ -680,6 +681,162 @@ static acpi_handle lps0_device_handle;
>  static guid_t lps0_dsm_guid;
>  static char lps0_dsm_func_mask;
> 
> +/* Device constraint entry structure */
> +struct lpi_device_info {
> +	char *name;
> +	int enabled;
> +	union acpi_object *package;
> +};
> +
> +/* Constraint package structure */
> +struct lpi_device_constraint {
> +	int uid;
> +	int min_dstate;
> +	int function_states;
> +};
> +
> +struct lpi_constraints {
> +	char *name;
> +	int min_dstate;
> +};
> +
> +static struct lpi_constraints *lpi_constraints_table;
> +static int lpi_constraints_table_size;
> +
> +static void lpi_device_get_constraints(void)
> +{
> +	union acpi_object *out_obj;
> +	int i;
> +
> +	out_obj = acpi_evaluate_dsm_typed(lps0_device_handle, &lps0_dsm_guid,
> +					  1,
> ACPI_LPS0_GET_DEVICE_CONSTRAINTS,
> +					  NULL, ACPI_TYPE_PACKAGE);
> +
> +	acpi_handle_debug(lps0_device_handle, "_DSM function 1 eval %s\n",
> +			  out_obj ? "successful" : "failed");
> +
> +	if (!out_obj)
> +		return;
> +
> +	lpi_constraints_table = kcalloc(out_obj->package.count,
> +					sizeof(*lpi_constraints_table),
> +					GFP_KERNEL);
> +	if (!lpi_constraints_table)
> +		goto free_acpi_buffer;
> +
> +	pr_debug("LPI: constraints dump begin\n");
> +	for (i = 0; i < out_obj->package.count; i++) {
> +		union acpi_object *package = &out_obj->package.elements[i];
> +		struct lpi_device_info info = { };
> +		int package_count = 0, j;
> +
> +		if (!package)
> +			continue;
> +
> +		for (j = 0; j < package->package.count; ++j) {
> +			union acpi_object *element =
> +					&(package->package.elements[j]);
> +
> +			switch (element->type) {
> +			case ACPI_TYPE_INTEGER:
> +				info.enabled = element->integer.value;
> +				break;
> +			case ACPI_TYPE_STRING:
> +				info.name = element->string.pointer;
> +				break;
> +			case ACPI_TYPE_PACKAGE:
> +				package_count = element->package.count;
> +				info.package = element->package.elements;
> +				break;
> +			}
> +		}
> +
> +		if (!info.enabled || !info.package || !info.name)
> +			continue;
> +
> +		lpi_constraints_table[lpi_constraints_table_size].name =
> +						kstrdup(info.name, GFP_KERNEL);
> +		if (!lpi_constraints_table[lpi_constraints_table_size].name)
> +			goto free_constraints;
> +
> +		pr_debug("index:%d Name:%s\n", i, info.name);
> +
> +		for (j = 0; j < package_count; ++j) {
> +			union acpi_object *info_obj = &info.package[j];
> +			union acpi_object *cnstr_pkg;
> +			union acpi_object *obj;
> +			struct lpi_device_constraint dev_info;
> +
> +			switch (info_obj->type) {
> +			case ACPI_TYPE_INTEGER:
> +				/* version */
> +				break;
> +			case ACPI_TYPE_PACKAGE:
> +				if (info_obj->package.count < 2)
> +					break;
> +
> +				cnstr_pkg = info_obj->package.elements;
> +				obj = &cnstr_pkg[0];
> +				dev_info.uid = obj->integer.value;
> +				obj = &cnstr_pkg[1];
> +				dev_info.min_dstate = obj->integer.value;
> +				pr_debug("uid %d min_dstate %d\n",
> +					 dev_info.uid,
> +					 dev_info.min_dstate);
> +				lpi_constraints_table[
> +					lpi_constraints_table_size].min_dstate =
> +						dev_info.min_dstate;
> +				break;
> +			}
> +		}
> +
> +		lpi_constraints_table_size++;
> +	}
> +
> +	pr_debug("LPI: constraints dump end\n");
> +free_acpi_buffer:
> +	ACPI_FREE(out_obj);
> +	return;
> +
> +free_constraints:
> +	ACPI_FREE(out_obj);
> +	for (i = 0; i < lpi_constraints_table_size; ++i)
> +		kfree(lpi_constraints_table[i].name);
> +	kfree(lpi_constraints_table);
> +	lpi_constraints_table_size = 0;
> +}
> +
> +static void lpi_check_constraints(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < lpi_constraints_table_size; ++i) {
> +		acpi_handle handle;
> +		struct acpi_device *adev;
> +		int state, ret;
> +
> +		if (ACPI_FAILURE(acpi_get_handle(NULL,
> +						 lpi_constraints_table[i].name,
> +						 &handle)))
> +			continue;
> +
> +		if (acpi_bus_get_device(handle, &adev))
> +			continue;
> +
> +		ret = acpi_device_get_power(adev, &state);
> +		if (!ret)
> +			pr_debug("LPI: %s required min power state %d, current
> power state %d, real power state %d\n",
> +				 lpi_constraints_table[i].name,
> +				 lpi_constraints_table[i].min_dstate,
> +				 adev->power.state, state);
Isn't this superfluous to be showing the state returned from acpi_device_get_power and
also probing directly at the state? You can't just rely on the information you got
back from apci_device_get_power?

> +
> +		if (adev->flags.power_manageable && adev->power.state <
> +					lpi_constraints_table[i].min_dstate)
> +			pr_info("LPI: Constraint [%s] not matched\n",
> +				 lpi_constraints_table[i].name);
Similarly here, can't you just compare against &state instead?

> +	}
> +}
> +
>  static void acpi_sleep_run_lps0_dsm(unsigned int func)
>  {
>  	union acpi_object *out_obj;
> @@ -729,6 +886,9 @@ static int lps0_device_attach(struct acpi_device *adev,
>  				  "_DSM function 0 evaluation failed\n");
>  	}
>  	ACPI_FREE(out_obj);
> +
> +	lpi_device_get_constraints();
> +
>  	return 0;
>  }
> 
> @@ -773,6 +933,8 @@ static void acpi_freeze_wake(void)
>  	 */
>  	if (acpi_sci_irq_valid() &&
>  	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) {
> +		if (pm_debug_messages_enabled())
> +			lpi_check_constraints();
>  		pm_system_cancel_wakeup();
>  		s2idle_wakeup = true;
>  	}
> --
> 2.7.5

Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Srinivas Pandruvada]

On Thu, 2017-08-10 at 22:07 +0000, Mario.Limonciello@dell.com wrote:
> 
> > 
> > 
[...]

> > +
> > +		ret = acpi_device_get_power(adev, &state);
> > +		if (!ret)
> > +			pr_debug("LPI: %s required min power state
> > %d, current
> > power state %d, real power state %d\n",
> > +				 lpi_constraints_table[i].name,
> > +				 lpi_constraints_table[i].min_dsta
> > te,
> > +				 adev->power.state, state);
> Isn't this superfluous to be showing the state returned from
> acpi_device_get_power and
> also probing directly at the state? You can't just rely on the
> information you got
> back from apci_device_get_power?
They can be different as one is real power state and the other is what
was set.
For example on Dell 9365 it shows

[ 1924.393653] LPI: \_SB.PCI0.XHC required min power state 3, current
power state 3, real power state 255 

> 
> > 
> > +
> > +		if (adev->flags.power_manageable && adev-
> > >power.state <
> > +					lpi_constraints_table[i].m
> > in_dstate)
> > +			pr_info("LPI: Constraint [%s] not
> > matched\n",
> > +				 lpi_constraints_table[i].name);
> Similarly here, can't you just compare against &state instead?
> 
The problem then the check will fail for XHCI on Dell 9365. So not
using "state".

Thanks,
Srinivas
> > 
> > +	}
> > +}
> > +
> >  static void acpi_sleep_run_lps0_dsm(unsigned int func)
> >  {
> >  	union acpi_object *out_obj;
> > @@ -729,6 +886,9 @@ static int lps0_device_attach(struct
> > acpi_device *adev,
> >  				  "_DSM function 0 evaluation
> > failed\n");
> >  	}
> >  	ACPI_FREE(out_obj);
> > +
> > +	lpi_device_get_constraints();
> > +
> >  	return 0;
> >  }
> > 
> > @@ -773,6 +933,8 @@ static void acpi_freeze_wake(void)
> >  	 */
> >  	if (acpi_sci_irq_valid() &&
> >  	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
> > {
> > +		if (pm_debug_messages_enabled())
> > +			lpi_check_constraints();
> >  		pm_system_cancel_wakeup();
> >  		s2idle_wakeup = true;
> >  	}
> > --
> > 2.7.5

RE: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Mario.Limonciello]

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> Sent: Thursday, August 10, 2017 5:54 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; rjw@rjwysocki.net;
> lenb@kernel.org
> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> acpi@vger.kernel.org; lukas@wunner.de
> Subject: Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for
> debug only
> 
> On Thu, 2017-08-10 at 22:07 +0000, Mario.Limonciello@dell.com wrote:
> >
> > >
> > >
> [...]
> 
> > > +
> > > +		ret = acpi_device_get_power(adev, &state);
> > > +		if (!ret)
> > > +			pr_debug("LPI: %s required min power state
> > > %d, current
> > > power state %d, real power state %d\n",
> > > +				 lpi_constraints_table[i].name,
> > > +				 lpi_constraints_table[i].min_dsta
> > > te,
> > > +				 adev->power.state, state);
> > Isn't this superfluous to be showing the state returned from
> > acpi_device_get_power and
> > also probing directly at the state? You can't just rely on the
> > information you got
> > back from apci_device_get_power?
> They can be different as one is real power state and the other is what
> was set.
> For example on Dell 9365 it shows
> 
> [ 1924.393653] LPI: \_SB.PCI0.XHC required min power state 3, current
> power state 3, real power state 255
> 

Isn't 255 ACPI_STATE_UNKNOWN?  That makes it seem like it
is a logic problem in acpi_device_get_power (or somewhere down the chain)
doesn't it?

> >
> > >
> > > +
> > > +		if (adev->flags.power_manageable && adev-
> > > >power.state <
> > > +					lpi_constraints_table[i].m
> > > in_dstate)
> > > +			pr_info("LPI: Constraint [%s] not
> > > matched\n",
> > > +				 lpi_constraints_table[i].name);
> > Similarly here, can't you just compare against &state instead?
> >
> The problem then the check will fail for XHCI on Dell 9365. So not
> using "state".
> 
> Thanks,
> Srinivas
> > >
> > > +	}
> > > +}
> > > +
> > >  static void acpi_sleep_run_lps0_dsm(unsigned int func)
> > >  {
> > >  	union acpi_object *out_obj;
> > > @@ -729,6 +886,9 @@ static int lps0_device_attach(struct
> > > acpi_device *adev,
> > >  				  "_DSM function 0 evaluation
> > > failed\n");
> > >  	}
> > >  	ACPI_FREE(out_obj);
> > > +
> > > +	lpi_device_get_constraints();
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -773,6 +933,8 @@ static void acpi_freeze_wake(void)
> > >  	 */
> > >  	if (acpi_sci_irq_valid() &&
> > >  	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
> > > {
> > > +		if (pm_debug_messages_enabled())
> > > +			lpi_check_constraints();
> > >  		pm_system_cancel_wakeup();
> > >  		s2idle_wakeup = true;
> > >  	}
> > > --
> > > 2.7.5

Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Srinivas Pandruvada]

On Fri, 2017-08-11 at 14:43 +0000, Mario.Limonciello@dell.com wrote:
> > 
> > -----Original Message-----
> > From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.c
> > om]
> > Sent: Thursday, August 10, 2017 5:54 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; rjw@rjwysocki.
> > net;
> > lenb@kernel.org
> > Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > acpi@vger.kernel.org; lukas@wunner.de
> > Subject: Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle
> > constraints for
> > debug only
> > 
> > On Thu, 2017-08-10 at 22:07 +0000, Mario.Limonciello@dell.com
> > wrote:
> > > 
> > > 
> > > > 
> > > > 
> > > > 
> > [...]
> > 
> > > 
> > > > 
> > > > +
> > > > +		ret = acpi_device_get_power(adev, &state);
> > > > +		if (!ret)
> > > > +			pr_debug("LPI: %s required min power
> > > > state
> > > > %d, current
> > > > power state %d, real power state %d\n",
> > > > +				 lpi_constraints_table[i].name
> > > > ,
> > > > +				 lpi_constraints_table[i].min_
> > > > dsta
> > > > te,
> > > > +				 adev->power.state, state);
> > > Isn't this superfluous to be showing the state returned from
> > > acpi_device_get_power and
> > > also probing directly at the state? You can't just rely on the
> > > information you got
> > > back from apci_device_get_power?
> > They can be different as one is real power state and the other is
> > what
> > was set.
> > For example on Dell 9365 it shows
> > 
> > [ 1924.393653] LPI: \_SB.PCI0.XHC required min power state 3,
> > current
> > power state 3, real power state 255
> > 
> Isn't 255 ACPI_STATE_UNKNOWN?  That makes it seem like it
> is a logic problem in acpi_device_get_power (or somewhere down the
> chain)
> doesn't it?
There is no _PSC for XHC device. So it will return unknown. This is an
optional object, so I think that dumping the status is fine, but
matching with output of acpi_device_get_power() as it relies on _PSC is
not correct for the constraint.

Thanks,
Srinivas

> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +
> > > > +		if (adev->flags.power_manageable && adev-
> > > > > 
> > > > > power.state <
> > > > +					lpi_constraints_table[
> > > > i].m
> > > > in_dstate)
> > > > +			pr_info("LPI: Constraint [%s] not
> > > > matched\n",
> > > > +				 lpi_constraints_table[i].name
> > > > );
> > > Similarly here, can't you just compare against &state instead?
> > > 
> > The problem then the check will fail for XHCI on Dell 9365. So not
> > using "state".
> > 
> > Thanks,
> > Srinivas
> > > 
> > > > 
> > > > 
> > > > +	}
> > > > +}
> > > > +
> > > >  static void acpi_sleep_run_lps0_dsm(unsigned int func)
> > > >  {
> > > >  	union acpi_object *out_obj;
> > > > @@ -729,6 +886,9 @@ static int lps0_device_attach(struct
> > > > acpi_device *adev,
> > > >  				  "_DSM function 0 evaluation
> > > > failed\n");
> > > >  	}
> > > >  	ACPI_FREE(out_obj);
> > > > +
> > > > +	lpi_device_get_constraints();
> > > > +
> > > >  	return 0;
> > > >  }
> > > > 
> > > > @@ -773,6 +933,8 @@ static void acpi_freeze_wake(void)
> > > >  	 */
> > > >  	if (acpi_sci_irq_valid() &&
> > > >  	    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_ir
> > > > q)))
> > > > {
> > > > +		if (pm_debug_messages_enabled())
> > > > +			lpi_check_constraints();
> > > >  		pm_system_cancel_wakeup();
> > > >  		s2idle_wakeup = true;
> > > >  	}
> > > > --
> > > > 2.7.5

Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Rafael J. Wysocki]

On Friday, August 11, 2017 6:18:50 PM CEST Srinivas Pandruvada wrote:
> On Fri, 2017-08-11 at 14:43 +0000, Mario.Limonciello@dell.com wrote:
> > > 
> > > -----Original Message-----
> > > From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.c
> > > om]
> > > Sent: Thursday, August 10, 2017 5:54 PM
> > > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; rjw@rjwysocki.
> > > net;
> > > lenb@kernel.org
> > > Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > > acpi@vger.kernel.org; lukas@wunner.de
> > > Subject: Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle
> > > constraints for
> > > debug only
> > > 
> > > On Thu, 2017-08-10 at 22:07 +0000, Mario.Limonciello@dell.com
> > > wrote:
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > [...]
> > > 
> > > > 
> > > > > 
> > > > > +
> > > > > +		ret = acpi_device_get_power(adev, &state);
> > > > > +		if (!ret)
> > > > > +			pr_debug("LPI: %s required min power
> > > > > state
> > > > > %d, current
> > > > > power state %d, real power state %d\n",
> > > > > +				 lpi_constraints_table[i].name
> > > > > ,
> > > > > +				 lpi_constraints_table[i].min_
> > > > > dsta
> > > > > te,
> > > > > +				 adev->power.state, state);
> > > > Isn't this superfluous to be showing the state returned from
> > > > acpi_device_get_power and
> > > > also probing directly at the state? You can't just rely on the
> > > > information you got
> > > > back from apci_device_get_power?
> > > They can be different as one is real power state and the other is
> > > what
> > > was set.
> > > For example on Dell 9365 it shows
> > > 
> > > [ 1924.393653] LPI: \_SB.PCI0.XHC required min power state 3,
> > > current
> > > power state 3, real power state 255
> > > 
> > Isn't 255 ACPI_STATE_UNKNOWN?  That makes it seem like it
> > is a logic problem in acpi_device_get_power (or somewhere down the
> > chain)
> > doesn't it?
> There is no _PSC for XHC device. So it will return unknown. This is an
> optional object, so I think that dumping the status is fine, but
> matching with output of acpi_device_get_power() as it relies on _PSC is
> not correct for the constraint.

Right.  Moreover, acpi_device_get_power() is basically mostly intended for
initialization, so the constraint should be matched against power.state,
as that's the current ACPI power state of the device as far as the kernel
can say.

Thanks,
Rafael