| Message ID | 1502232075-23832-3-git-send-email-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > > } > > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBTcmluaXZhcyBQYW5kcnV2YWRh IFttYWlsdG86c3Jpbml2YXMucGFuZHJ1dmFkYUBsaW51eC5pbnRlbC5jb21dDQo+IFNlbnQ6IFRo dXJzZGF5LCBBdWd1c3QgMTAsIDIwMTcgNTo1NCBQTQ0KPiBUbzogTGltb25jaWVsbG8sIE1hcmlv IDxNYXJpb19MaW1vbmNpZWxsb0BEZWxsLmNvbT47IHJqd0Byand5c29ja2kubmV0Ow0KPiBsZW5i QGtlcm5lbC5vcmcNCj4gQ2M6IGxpbnV4LXBtQHZnZXIua2VybmVsLm9yZzsgbGludXgta2VybmVs QHZnZXIua2VybmVsLm9yZzsgbGludXgtDQo+IGFjcGlAdmdlci5rZXJuZWwub3JnOyBsdWthc0B3 dW5uZXIuZGUNCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2MiAyLzJdIEFDUEkgLyBTbGVlcDogQ2hl Y2sgbG93IHBvd2VyIGlkbGUgY29uc3RyYWludHMgZm9yDQo+IGRlYnVnIG9ubHkNCj4gDQo+IE9u IFRodSwgMjAxNy0wOC0xMCBhdCAyMjowNyArMDAwMCwgTWFyaW8uTGltb25jaWVsbG9AZGVsbC5j b20gd3JvdGU6DQo+ID4NCj4gPiA+DQo+ID4gPg0KPiBbLi4uXQ0KPiANCj4gPiA+ICsNCj4gPiA+ ICsJCXJldCA9IGFjcGlfZGV2aWNlX2dldF9wb3dlcihhZGV2LCAmc3RhdGUpOw0KPiA+ID4gKwkJ aWYgKCFyZXQpDQo+ID4gPiArCQkJcHJfZGVidWcoIkxQSTogJXMgcmVxdWlyZWQgbWluIHBvd2Vy IHN0YXRlDQo+ID4gPiAlZCwgY3VycmVudA0KPiA+ID4gcG93ZXIgc3RhdGUgJWQsIHJlYWwgcG93 ZXIgc3RhdGUgJWRcbiIsDQo+ID4gPiArCQkJCcKgbHBpX2NvbnN0cmFpbnRzX3RhYmxlW2ldLm5h bWUsDQo+ID4gPiArCQkJCcKgbHBpX2NvbnN0cmFpbnRzX3RhYmxlW2ldLm1pbl9kc3RhDQo+ID4g PiB0ZSwNCj4gPiA+ICsJCQkJwqBhZGV2LT5wb3dlci5zdGF0ZSwgc3RhdGUpOw0KPiA+IElzbid0 IHRoaXMgc3VwZXJmbHVvdXMgdG8gYmUgc2hvd2luZyB0aGUgc3RhdGUgcmV0dXJuZWQgZnJvbQ0K PiA+IGFjcGlfZGV2aWNlX2dldF9wb3dlciBhbmQNCj4gPiBhbHNvIHByb2JpbmcgZGlyZWN0bHkg YXQgdGhlIHN0YXRlPyBZb3UgY2FuJ3QganVzdCByZWx5IG9uIHRoZQ0KPiA+IGluZm9ybWF0aW9u IHlvdSBnb3QNCj4gPiBiYWNrIGZyb20gYXBjaV9kZXZpY2VfZ2V0X3Bvd2VyPw0KPiBUaGV5IGNh biBiZSBkaWZmZXJlbnQgYXMgb25lIGlzIHJlYWwgcG93ZXIgc3RhdGUgYW5kIHRoZSBvdGhlciBp cyB3aGF0DQo+IHdhcyBzZXQuDQo+IEZvciBleGFtcGxlIG9uIERlbGwgOTM2NSBpdCBzaG93cw0K PiANCj4gWyAxOTI0LjM5MzY1M10gTFBJOiBcX1NCLlBDSTAuWEhDIHJlcXVpcmVkIG1pbiBwb3dl ciBzdGF0ZSAzLCBjdXJyZW50DQo+IHBvd2VyIHN0YXRlIDMsIHJlYWwgcG93ZXIgc3RhdGUgMjU1 DQo+IA0KDQpJc24ndCAyNTUgQUNQSV9TVEFURV9VTktOT1dOPyAgVGhhdCBtYWtlcyBpdCBzZWVt IGxpa2UgaXQNCmlzIGEgbG9naWMgcHJvYmxlbSBpbiBhY3BpX2RldmljZV9nZXRfcG93ZXIgKG9y IHNvbWV3aGVyZSBkb3duIHRoZSBjaGFpbikNCmRvZXNuJ3QgaXQ/DQoNCj4gPg0KPiA+ID4NCj4g PiA+ICsNCj4gPiA+ICsJCWlmIChhZGV2LT5mbGFncy5wb3dlcl9tYW5hZ2VhYmxlICYmIGFkZXYt DQo+ID4gPiA+cG93ZXIuc3RhdGUgPA0KPiA+ID4gKwkJCQkJbHBpX2NvbnN0cmFpbnRzX3RhYmxl W2ldLm0NCj4gPiA+IGluX2RzdGF0ZSkNCj4gPiA+ICsJCQlwcl9pbmZvKCJMUEk6IENvbnN0cmFp bnQgWyVzXSBub3QNCj4gPiA+IG1hdGNoZWRcbiIsDQo+ID4gPiArCQkJCcKgbHBpX2NvbnN0cmFp bnRzX3RhYmxlW2ldLm5hbWUpOw0KPiA+IFNpbWlsYXJseSBoZXJlLCBjYW4ndCB5b3UganVzdCBj b21wYXJlIGFnYWluc3QgJnN0YXRlIGluc3RlYWQ/DQo+ID4NCj4gVGhlIHByb2JsZW0gdGhlbiB0 aGUgY2hlY2sgd2lsbCBmYWlsIGZvciBYSENJIG9uIERlbGwgOTM2NS4gU28gbm90DQo+IHVzaW5n ICJzdGF0ZSIuDQo+IA0KPiBUaGFua3MsDQo+IFNyaW5pdmFzDQo+ID4gPg0KPiA+ID4gKwl9DQo+ ID4gPiArfQ0KPiA+ID4gKw0KPiA+ID4gwqBzdGF0aWMgdm9pZCBhY3BpX3NsZWVwX3J1bl9scHMw X2RzbSh1bnNpZ25lZCBpbnQgZnVuYykNCj4gPiA+IMKgew0KPiA+ID4gwqAJdW5pb24gYWNwaV9v YmplY3QgKm91dF9vYmo7DQo+ID4gPiBAQCAtNzI5LDYgKzg4Niw5IEBAIHN0YXRpYyBpbnQgbHBz MF9kZXZpY2VfYXR0YWNoKHN0cnVjdA0KPiA+ID4gYWNwaV9kZXZpY2UgKmFkZXYsDQo+ID4gPiDC oAkJCQnCoMKgIl9EU00gZnVuY3Rpb24gMCBldmFsdWF0aW9uDQo+ID4gPiBmYWlsZWRcbiIpOw0K PiA+ID4gwqAJfQ0KPiA+ID4gwqAJQUNQSV9GUkVFKG91dF9vYmopOw0KPiA+ID4gKw0KPiA+ID4g KwlscGlfZGV2aWNlX2dldF9jb25zdHJhaW50cygpOw0KPiA+ID4gKw0KPiA+ID4gwqAJcmV0dXJu IDA7DQo+ID4gPiDCoH0NCj4gPiA+DQo+ID4gPiBAQCAtNzczLDYgKzkzMyw4IEBAIHN0YXRpYyB2 b2lkIGFjcGlfZnJlZXplX3dha2Uodm9pZCkNCj4gPiA+IMKgCcKgKi8NCj4gPiA+IMKgCWlmIChh Y3BpX3NjaV9pcnFfdmFsaWQoKSAmJg0KPiA+ID4gwqAJwqDCoMKgwqAhaXJxZF9pc193YWtldXBf YXJtZWQoaXJxX2dldF9pcnFfZGF0YShhY3BpX3NjaV9pcnEpKSkNCj4gPiA+IHsNCj4gPiA+ICsJ CWlmIChwbV9kZWJ1Z19tZXNzYWdlc19lbmFibGVkKCkpDQo+ID4gPiArCQkJbHBpX2NoZWNrX2Nv bnN0cmFpbnRzKCk7DQo+ID4gPiDCoAkJcG1fc3lzdGVtX2NhbmNlbF93YWtldXAoKTsNCj4gPiA+ IMKgCQlzMmlkbGVfd2FrZXVwID0gdHJ1ZTsNCj4gPiA+IMKgCX0NCj4gPiA+IC0tDQo+ID4gPiAy LjcuNQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(+)