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

2017-08-12 Thread Rafael J. Wysocki 
On Sun, Aug 13, 2017 at 2:00 AM, Srinivas Pandruvada
 wrote:
> On Sun, 2017-08-13 at 00:37 +0200, Rafael J. Wysocki wrote:
>> On Sat, Aug 12, 2017 at 5:59 PM, Srinivas Pandruvada
>>  wrote:
>> >
>> > On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
>> > [...]
>> >
>> > >
>> > > >
>> > > > +
>> > > > +struct lpi_constraints {
>> > > > +   char *name;
>> > > > +   int min_dstate;
>> > > If you store the handle here as well, you won't need to
>> > > look it up every time _check_constraints() is called.
>> > The reason I didn't keep handle here, I thought handle can be stale
>> > or
>> > change for PnP device on plug in and out. Is this not true?
>> The handles don't go away on hot remove as a rule.  That may only
>> happen if tables get unloaded, but basically the constraints should
>> not point to anything in a table that may go away.
>
> So we don't need to worry about this case where tables gets unloaded
> and replaced?

I wouldn't. :-)

We generally don't support that case entirely, so why bother?

> This is in a debug path, so additional overhead of path
> to handle conversion may not be significant.

Well, it still is wasted cycles ...

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

2017-08-12 Thread Rafael J. Wysocki 
On Sun, Aug 13, 2017 at 2:00 AM, Srinivas Pandruvada
 wrote:
> On Sun, 2017-08-13 at 00:37 +0200, Rafael J. Wysocki wrote:
>> On Sat, Aug 12, 2017 at 5:59 PM, Srinivas Pandruvada
>>  wrote:
>> >
>> > On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
>> > [...]
>> >
>> > >
>> > > >
>> > > > +
>> > > > +struct lpi_constraints {
>> > > > +   char *name;
>> > > > +   int min_dstate;
>> > > If you store the handle here as well, you won't need to
>> > > look it up every time _check_constraints() is called.
>> > The reason I didn't keep handle here, I thought handle can be stale
>> > or
>> > change for PnP device on plug in and out. Is this not true?
>> The handles don't go away on hot remove as a rule.  That may only
>> happen if tables get unloaded, but basically the constraints should
>> not point to anything in a table that may go away.
>
> So we don't need to worry about this case where tables gets unloaded
> and replaced?

I wouldn't. :-)

We generally don't support that case entirely, so why bother?

> This is in a debug path, so additional overhead of path
> to handle conversion may not be significant.

Well, it still is wasted cycles ...

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

2017-08-12 Thread Srinivas Pandruvada 
On Sun, 2017-08-13 at 00:37 +0200, Rafael J. Wysocki wrote:
> On Sat, Aug 12, 2017 at 5:59 PM, Srinivas Pandruvada
>  wrote:
> > 
> > On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
> > [...]
> > 
> > > 
> > > > 
> > > > +
> > > > +struct lpi_constraints {
> > > > +   char *name;
> > > > +   int min_dstate;
> > > If you store the handle here as well, you won't need to
> > > look it up every time _check_constraints() is called.
> > The reason I didn't keep handle here, I thought handle can be stale
> > or
> > change for PnP device on plug in and out. Is this not true?
> The handles don't go away on hot remove as a rule.  That may only
> happen if tables get unloaded, but basically the constraints should
> not point to anything in a table that may go away.
So we don't need to worry about this case where tables gets unloaded
and replaced? This is in a debug path, so additional overhead of path
to handle conversion may not be significant.

Thanks,
Srinivas

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

2017-08-12 Thread Srinivas Pandruvada 
On Sun, 2017-08-13 at 00:37 +0200, Rafael J. Wysocki wrote:
> On Sat, Aug 12, 2017 at 5:59 PM, Srinivas Pandruvada
>  wrote:
> > 
> > On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
> > [...]
> > 
> > > 
> > > > 
> > > > +
> > > > +struct lpi_constraints {
> > > > +   char *name;
> > > > +   int min_dstate;
> > > If you store the handle here as well, you won't need to
> > > look it up every time _check_constraints() is called.
> > The reason I didn't keep handle here, I thought handle can be stale
> > or
> > change for PnP device on plug in and out. Is this not true?
> The handles don't go away on hot remove as a rule.  That may only
> happen if tables get unloaded, but basically the constraints should
> not point to anything in a table that may go away.
So we don't need to worry about this case where tables gets unloaded
and replaced? This is in a debug path, so additional overhead of path
to handle conversion may not be significant.

Thanks,
Srinivas

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

2017-08-12 Thread Rafael J. Wysocki 
On Sat, Aug 12, 2017 at 5:59 PM, Srinivas Pandruvada
 wrote:
> On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
> [...]
>
>> > +
>> > +struct lpi_constraints {
>> > +   char *name;
>> > +   int min_dstate;
>> If you store the handle here as well, you won't need to
>> look it up every time _check_constraints() is called.
>
> The reason I didn't keep handle here, I thought handle can be stale or
> change for PnP device on plug in and out. Is this not true?

The handles don't go away on hot remove as a rule.  That may only
happen if tables get unloaded, but basically the constraints should
not point to anything in a table that may go away.

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

2017-08-12 Thread Rafael J. Wysocki 
On Sat, Aug 12, 2017 at 5:59 PM, Srinivas Pandruvada
 wrote:
> On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
> [...]
>
>> > +
>> > +struct lpi_constraints {
>> > +   char *name;
>> > +   int min_dstate;
>> If you store the handle here as well, you won't need to
>> look it up every time _check_constraints() is called.
>
> The reason I didn't keep handle here, I thought handle can be stale or
> change for PnP device on plug in and out. Is this not true?

The handles don't go away on hot remove as a rule.  That may only
happen if tables get unloaded, but basically the constraints should
not point to anything in a table that may go away.

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

2017-08-12 Thread Srinivas Pandruvada 
On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
[...]

> > +
> > +struct lpi_constraints {
> > +   char *name;
> > +   int min_dstate;
> If you store the handle here as well, you won't need to
> look it up every time _check_constraints() is called.

The reason I didn't keep handle here, I thought handle can be stale or
change for PnP device on plug in and out. Is this not true?

> > 
> > +};
> > +
> > +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,
> > _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_ta
> > ble),
> > +   GFP_KERNEL);
> > +   if (!lpi_constraints_table)
> > +   goto free_acpi_buffer;
> > +
> > +   pr_debug("LPI: constraints dump begin\n");
> Please add an empty line after this.  Also something like
> "constraints
> list begin" would sound better IMO.
> 
OK.

> > 
> > +   for (i = 0; i < out_obj->package.count; i++) {
> > +   union acpi_object *package = _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;
> > +
> I would evaluate acpi_get_handle() here and store the handle in the
> constraints table (if persent).  And you can skip the entry
> altogether
> if not present, because you won't be printing it anyway.
> 
> > 
> > +   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 =
> > [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 = _pkg[0];
> > +   dev_info.uid = obj->integer.value;
> > +   obj = _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_dstat
> > e;
> > +   break;
> > +   }
> > +   }
> > +
> > +   lpi_constraints_table_size++;
> > +   }
> > +
> > +   pr_debug("LPI: constraints dump end\n");
> > +free_acpi_buffer:
> > +   ACPI_FREE(out_obj);
> > +   return;
> >

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

2017-08-12 Thread Srinivas Pandruvada 
On Sat, 2017-08-12 at 16:27 +0200, Rafael J. Wysocki wrote:
[...]

> > +
> > +struct lpi_constraints {
> > +   char *name;
> > +   int min_dstate;
> If you store the handle here as well, you won't need to
> look it up every time _check_constraints() is called.

The reason I didn't keep handle here, I thought handle can be stale or
change for PnP device on plug in and out. Is this not true?

> > 
> > +};
> > +
> > +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,
> > _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_ta
> > ble),
> > +   GFP_KERNEL);
> > +   if (!lpi_constraints_table)
> > +   goto free_acpi_buffer;
> > +
> > +   pr_debug("LPI: constraints dump begin\n");
> Please add an empty line after this.  Also something like
> "constraints
> list begin" would sound better IMO.
> 
OK.

> > 
> > +   for (i = 0; i < out_obj->package.count; i++) {
> > +   union acpi_object *package = _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;
> > +
> I would evaluate acpi_get_handle() here and store the handle in the
> constraints table (if persent).  And you can skip the entry
> altogether
> if not present, because you won't be printing it anyway.
> 
> > 
> > +   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 =
> > [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 = _pkg[0];
> > +   dev_info.uid = obj->integer.value;
> > +   obj = _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_dstat
> > e;
> > +   break;
> > +   }
> > +   }
> > +
> > +   lpi_constraints_table_size++;
> > +   }
> > +
> > +   pr_debug("LPI: constraints dump end\n");
> > +free_acpi_buffer:
> > +   ACPI_FREE(out_obj);
> > +   return;
> >

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

2017-08-12 Thread Rafael J. Wysocki 
On Friday, August 11, 2017 8:23:55 PM 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.
> 
> Since pm_debug_messages_on setting is used as condition to check
> constraints outside kernel/power/main.c, pm_debug_messages_on is changed
> to a global variable.
> 
> Link: 
> http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
> Signed-off-by: Srinivas Pandruvada 
> ---
> 
> v3
> - Removed the patch 0001 for exporting pm_debug_messages_on via a function.
> As suggested by Rafael, made the pm_debug_messages_on a global variable 
> instead.
> - Moved the constraint check outside if() block and valid for all calls to
> acpi_freeze_wakeup()
> - Dumping irrespective of return value of acpi_device_get_power().
> - Rebased to linux-next as of today
>  
> v2
> - Changes as suggested by Lukas Wunner.
> - Using pm_debug_messages_on attribute to prevent constraints check to
> save some cycles on wake.
> 
>  drivers/acpi/sleep.c| 169 
> 
>  include/linux/suspend.h |   2 +
>  kernel/power/main.c |   2 +-
>  3 files changed, 172 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 0a16435..df513e7 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,167 @@ 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;

If you store the handle here as well, you won't need to
look it up every time _check_constraints() is called.

> +};
> +
> +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, _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");

Please add an empty line after this.  Also something like "constraints
list begin" would sound better IMO.

> + for (i = 0; i < out_obj->package.count; i++) {
> + union acpi_object *package = _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:
> +

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

2017-08-12 Thread Rafael J. Wysocki 
On Friday, August 11, 2017 8:23:55 PM 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.
> 
> Since pm_debug_messages_on setting is used as condition to check
> constraints outside kernel/power/main.c, pm_debug_messages_on is changed
> to a global variable.
> 
> Link: 
> http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
> Signed-off-by: Srinivas Pandruvada 
> ---
> 
> v3
> - Removed the patch 0001 for exporting pm_debug_messages_on via a function.
> As suggested by Rafael, made the pm_debug_messages_on a global variable 
> instead.
> - Moved the constraint check outside if() block and valid for all calls to
> acpi_freeze_wakeup()
> - Dumping irrespective of return value of acpi_device_get_power().
> - Rebased to linux-next as of today
>  
> v2
> - Changes as suggested by Lukas Wunner.
> - Using pm_debug_messages_on attribute to prevent constraints check to
> save some cycles on wake.
> 
>  drivers/acpi/sleep.c| 169 
> 
>  include/linux/suspend.h |   2 +
>  kernel/power/main.c |   2 +-
>  3 files changed, 172 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 0a16435..df513e7 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,167 @@ 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;

If you store the handle here as well, you won't need to
look it up every time _check_constraints() is called.

> +};
> +
> +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, _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");

Please add an empty line after this.  Also something like "constraints
list begin" would sound better IMO.

> + for (i = 0; i < out_obj->package.count; i++) {
> + union acpi_object *package = _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

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

2017-08-11 Thread 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.

Since pm_debug_messages_on setting is used as condition to check
constraints outside kernel/power/main.c, pm_debug_messages_on is changed
to a global variable.

Link: 
http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
Signed-off-by: Srinivas Pandruvada 
---

v3
- Removed the patch 0001 for exporting pm_debug_messages_on via a function.
As suggested by Rafael, made the pm_debug_messages_on a global variable instead.
- Moved the constraint check outside if() block and valid for all calls to
acpi_freeze_wakeup()
- Dumping irrespective of return value of acpi_device_get_power().
- Rebased to linux-next as of today
 
v2
- Changes as suggested by Lukas Wunner.
- Using pm_debug_messages_on attribute to prevent constraints check to
save some cycles on wake.

 drivers/acpi/sleep.c| 169 
 include/linux/suspend.h |   2 +
 kernel/power/main.c |   2 +-
 3 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 0a16435..df513e7 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_ON4
 #define ACPI_LPS0_ENTRY5
@@ -680,6 +681,167 @@ 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, _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 = _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)
+

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

2017-08-11 Thread 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.

Since pm_debug_messages_on setting is used as condition to check
constraints outside kernel/power/main.c, pm_debug_messages_on is changed
to a global variable.

Link: 
http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
Signed-off-by: Srinivas Pandruvada 
---

v3
- Removed the patch 0001 for exporting pm_debug_messages_on via a function.
As suggested by Rafael, made the pm_debug_messages_on a global variable instead.
- Moved the constraint check outside if() block and valid for all calls to
acpi_freeze_wakeup()
- Dumping irrespective of return value of acpi_device_get_power().
- Rebased to linux-next as of today
 
v2
- Changes as suggested by Lukas Wunner.
- Using pm_debug_messages_on attribute to prevent constraints check to
save some cycles on wake.

 drivers/acpi/sleep.c| 169 
 include/linux/suspend.h |   2 +
 kernel/power/main.c |   2 +-
 3 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 0a16435..df513e7 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_ON4
 #define ACPI_LPS0_ENTRY5
@@ -680,6 +681,167 @@ 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, _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 = _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;
+
+