* [PATCH 0/1] acpi_get_sleep_type_data(): accept package with one element @ 2012-12-20 17:31 Laszlo Ersek 2012-12-20 17:31 ` [PATCH 1/1] " Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2012-12-20 17:31 UTC (permalink / raw) To: linux-acpi, Len Brown, Rafael J. Wysocki, lersek I'm trying to follow the guidelines set forth under <https://lesswatts.org/projects/acpi/> and in the MAINTAINERS file. Apologies if I'm getting wrong any posting requirement. The patch is against Linus's tree @ commit f01af9f8. I'm not subscribed to linux-acpi; please CC me on any followup. "Please apply". Thanks for considering. Laszlo Ersek (1): acpi_get_sleep_type_data(): accept package with one element drivers/acpi/acpica/hwxface.c | 60 ++++++++++++++++++++++------------------- 1 files changed, 32 insertions(+), 28 deletions(-) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] acpi_get_sleep_type_data(): accept package with one element 2012-12-20 17:31 [PATCH 0/1] acpi_get_sleep_type_data(): accept package with one element Laszlo Ersek @ 2012-12-20 17:31 ` Laszlo Ersek 2012-12-20 21:22 ` Rafael J. Wysocki 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2012-12-20 17:31 UTC (permalink / raw) To: linux-acpi, Len Brown, Rafael J. Wysocki, lersek Citation from "ACPIspec50.pdf": 7.3.4 System \_Sx states [...] Return Value: A Package containing an Integer containing register values for sleeping Hence accept one-element packages as well when parsing sleep states, in addition to the currently accepted multi-element packages. See the ACPICA upstream posting under [1]. This change is prompted by OVMF svn rev 14003 [2] that creates the \_S3 and \_S4 packages with a single DWORD in each: Name (\_S3, Package (0x01) { 0x00000001 }) Name (\_S4, Package (0x01) { 0x00000002 }) Tested with a RHEL-6 kernel (patch applies verbatim) in virtual machines. Without the patch, - on OVMF, errors like ACPI Error: Sleep State return package does not have at least two elements (20120711/hwxface-511) ACPI Exception: AE_AML_NO_OPERAND, While evaluating SleepState [\_S3_], bad Sleep object ffff880119a33cf0 type Package (20120711/hwxface-543) are logged and suspend doesn't work, - on SeaBIOS, which provides multi-element packages, suspend works. With the patch, the messages are gone on OVMF, and the VM can be suspended with both boot firmwares. [1] http://lists.acpica.org/pipermail/devel/2012-December/000405.html [2] http://tianocore.git.sourceforge.net/git/gitweb.cgi?p=tianocore/edk2;a=commitdiff;h=14430c55c8d0e9a8487c2c74155e63299632ef5e Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- drivers/acpi/acpica/hwxface.c | 60 ++++++++++++++++++++++------------------- 1 files changed, 32 insertions(+), 28 deletions(-) diff --git a/drivers/acpi/acpica/hwxface.c b/drivers/acpi/acpica/hwxface.c index 05a154c..6cd5bb0 100644 --- a/drivers/acpi/acpica/hwxface.c +++ b/drivers/acpi/acpica/hwxface.c @@ -499,39 +499,43 @@ acpi_get_sleep_type_data(u8 sleep_state, u8 *sleep_type_a, u8 *sleep_type_b) } /* - * The package must have at least two elements. NOTE (March 2005): This - * goes against the current ACPI spec which defines this object as a - * package with one encoded DWORD element. However, existing practice - * by BIOS vendors seems to be to have 2 or more elements, at least - * one per sleep type (A/B). + * The current ACPI spec (v 5.0) defines this object as a package with + * one encoded DWORD element. + * + * Existing practice by BIOS vendors seems to be to have 2 or more + * elements, at least one per sleep type (A/B). */ - else if (info->return_object->package.count < 2) { + else if (info->return_object->package.count == 0) { ACPI_ERROR((AE_INFO, - "Sleep State return package does not have at least two elements")); + "Sleep State return package does not have elements")); status = AE_AML_NO_OPERAND; - } - - /* The first two elements must both be of type Integer */ - - else if (((info->return_object->package.elements[0])->common.type - != ACPI_TYPE_INTEGER) || - ((info->return_object->package.elements[1])->common.type - != ACPI_TYPE_INTEGER)) { - ACPI_ERROR((AE_INFO, - "Sleep State return package elements are not both Integers " - "(%s, %s)", - acpi_ut_get_object_type_name(info->return_object-> - package.elements[0]), - acpi_ut_get_object_type_name(info->return_object-> - package.elements[1]))); - status = AE_AML_OPERAND_TYPE; } else { - /* Valid _Sx_ package size, type, and value */ + union acpi_operand_object **elements; - *sleep_type_a = (u8) - (info->return_object->package.elements[0])->integer.value; - *sleep_type_b = (u8) - (info->return_object->package.elements[1])->integer.value; + elements = info->return_object->package.elements; + + if (elements[0]->common.type != ACPI_TYPE_INTEGER) { + ACPI_ERROR((AE_INFO, + "1st element in Sleep State return package is not Integer " + "(%s)", + acpi_ut_get_object_type_name(elements[0]))); + status = AE_AML_OPERAND_TYPE; + } + else if (info->return_object->package.count == 1) { + *sleep_type_a = (u8) elements[0]->integer.value; + *sleep_type_b = (u8)(elements[0]->integer.value >> 8); + } + else if (elements[1]->common.type != ACPI_TYPE_INTEGER) { + ACPI_ERROR((AE_INFO, + "2nd element in Sleep State return package is not Integer " + "(%s)", + acpi_ut_get_object_type_name(elements[1]))); + status = AE_AML_OPERAND_TYPE; + } + else { + *sleep_type_a = (u8)elements[0]->integer.value; + *sleep_type_b = (u8)elements[1]->integer.value; + } } if (ACPI_FAILURE(status)) { -- 1.7.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] acpi_get_sleep_type_data(): accept package with one element 2012-12-20 17:31 ` [PATCH 1/1] " Laszlo Ersek @ 2012-12-20 21:22 ` Rafael J. Wysocki 2012-12-20 21:39 ` Moore, Robert 0 siblings, 1 reply; 4+ messages in thread From: Rafael J. Wysocki @ 2012-12-20 21:22 UTC (permalink / raw) To: Laszlo Ersek; +Cc: linux-acpi, Len Brown, Moore, Robert On Thursday, December 20, 2012 06:31:23 PM Laszlo Ersek wrote: > Citation from "ACPIspec50.pdf": > > 7.3.4 System \_Sx states > > [...] > > Return Value: > > A Package containing an Integer containing register values for > sleeping > > Hence accept one-element packages as well when parsing sleep states, in > addition to the currently accepted multi-element packages. > > See the ACPICA upstream posting under [1]. > > This change is prompted by OVMF svn rev 14003 [2] that creates the \_S3 > and \_S4 packages with a single DWORD in each: > > Name (\_S3, Package (0x01) > { > 0x00000001 > }) > Name (\_S4, Package (0x01) > { > 0x00000002 > }) > > Tested with a RHEL-6 kernel (patch applies verbatim) in virtual machines. > Without the patch, > - on OVMF, errors like > > ACPI Error: Sleep State return package does not have at least two > elements (20120711/hwxface-511) > ACPI Exception: AE_AML_NO_OPERAND, While evaluating SleepState [\_S3_], > bad Sleep object ffff880119a33cf0 type Package > (20120711/hwxface-543) > > are logged and suspend doesn't work, > > - on SeaBIOS, which provides multi-element packages, suspend works. > > With the patch, the messages are gone on OVMF, and the VM can be suspended > with both boot firmwares. > > [1] http://lists.acpica.org/pipermail/devel/2012-December/000405.html > [2] http://tianocore.git.sourceforge.net/git/gitweb.cgi?p=tianocore/edk2;a=commitdiff;h=14430c55c8d0e9a8487c2c74155e63299632ef5e Well, the patch modifies ACPICA, so it would need to go through the ACPICA upstream for us to take it to the kernel. Adding a CC to Bob Moore, who's the upstream ACPICA maintainer. Thanks, Rafael > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > drivers/acpi/acpica/hwxface.c | 60 ++++++++++++++++++++++------------------- > 1 files changed, 32 insertions(+), 28 deletions(-) > > diff --git a/drivers/acpi/acpica/hwxface.c b/drivers/acpi/acpica/hwxface.c > index 05a154c..6cd5bb0 100644 > --- a/drivers/acpi/acpica/hwxface.c > +++ b/drivers/acpi/acpica/hwxface.c > @@ -499,39 +499,43 @@ acpi_get_sleep_type_data(u8 sleep_state, u8 *sleep_type_a, u8 *sleep_type_b) > } > > /* > - * The package must have at least two elements. NOTE (March 2005): This > - * goes against the current ACPI spec which defines this object as a > - * package with one encoded DWORD element. However, existing practice > - * by BIOS vendors seems to be to have 2 or more elements, at least > - * one per sleep type (A/B). > + * The current ACPI spec (v 5.0) defines this object as a package with > + * one encoded DWORD element. > + * > + * Existing practice by BIOS vendors seems to be to have 2 or more > + * elements, at least one per sleep type (A/B). > */ > - else if (info->return_object->package.count < 2) { > + else if (info->return_object->package.count == 0) { > ACPI_ERROR((AE_INFO, > - "Sleep State return package does not have at least two elements")); > + "Sleep State return package does not have elements")); > status = AE_AML_NO_OPERAND; > - } > - > - /* The first two elements must both be of type Integer */ > - > - else if (((info->return_object->package.elements[0])->common.type > - != ACPI_TYPE_INTEGER) || > - ((info->return_object->package.elements[1])->common.type > - != ACPI_TYPE_INTEGER)) { > - ACPI_ERROR((AE_INFO, > - "Sleep State return package elements are not both Integers " > - "(%s, %s)", > - acpi_ut_get_object_type_name(info->return_object-> > - package.elements[0]), > - acpi_ut_get_object_type_name(info->return_object-> > - package.elements[1]))); > - status = AE_AML_OPERAND_TYPE; > } else { > - /* Valid _Sx_ package size, type, and value */ > + union acpi_operand_object **elements; > > - *sleep_type_a = (u8) > - (info->return_object->package.elements[0])->integer.value; > - *sleep_type_b = (u8) > - (info->return_object->package.elements[1])->integer.value; > + elements = info->return_object->package.elements; > + > + if (elements[0]->common.type != ACPI_TYPE_INTEGER) { > + ACPI_ERROR((AE_INFO, > + "1st element in Sleep State return package is not Integer " > + "(%s)", > + acpi_ut_get_object_type_name(elements[0]))); > + status = AE_AML_OPERAND_TYPE; > + } > + else if (info->return_object->package.count == 1) { > + *sleep_type_a = (u8) elements[0]->integer.value; > + *sleep_type_b = (u8)(elements[0]->integer.value >> 8); > + } > + else if (elements[1]->common.type != ACPI_TYPE_INTEGER) { > + ACPI_ERROR((AE_INFO, > + "2nd element in Sleep State return package is not Integer " > + "(%s)", > + acpi_ut_get_object_type_name(elements[1]))); > + status = AE_AML_OPERAND_TYPE; > + } > + else { > + *sleep_type_a = (u8)elements[0]->integer.value; > + *sleep_type_b = (u8)elements[1]->integer.value; > + } > } > > if (ACPI_FAILURE(status)) { > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 1/1] acpi_get_sleep_type_data(): accept package with one element 2012-12-20 21:22 ` Rafael J. Wysocki @ 2012-12-20 21:39 ` Moore, Robert 0 siblings, 0 replies; 4+ messages in thread From: Moore, Robert @ 2012-12-20 21:39 UTC (permalink / raw) To: Rafael J. Wysocki, Laszlo Ersek; +Cc: linux-acpi@vger.kernel.org, Len Brown And while we are at it, we can make sure that a lone integer (without a surrounding package) gets wrapped with a package before handing it off to a driver. > -----Original Message----- > From: Rafael J. Wysocki [mailto:rjw@sisk.pl] > Sent: Thursday, December 20, 2012 1:23 PM > To: Laszlo Ersek > Cc: linux-acpi@vger.kernel.org; Len Brown; Moore, Robert > Subject: Re: [PATCH 1/1] acpi_get_sleep_type_data(): accept package with > one element > > On Thursday, December 20, 2012 06:31:23 PM Laszlo Ersek wrote: > > Citation from "ACPIspec50.pdf": > > > > 7.3.4 System \_Sx states > > > > [...] > > > > Return Value: > > > > A Package containing an Integer containing register values for > > sleeping > > > > Hence accept one-element packages as well when parsing sleep states, > > in addition to the currently accepted multi-element packages. > > > > See the ACPICA upstream posting under [1]. > > > > This change is prompted by OVMF svn rev 14003 [2] that creates the > > \_S3 and \_S4 packages with a single DWORD in each: > > > > Name (\_S3, Package (0x01) > > { > > 0x00000001 > > }) > > Name (\_S4, Package (0x01) > > { > > 0x00000002 > > }) > > > > Tested with a RHEL-6 kernel (patch applies verbatim) in virtual > machines. > > Without the patch, > > - on OVMF, errors like > > > > ACPI Error: Sleep State return package does not have at least two > > elements (20120711/hwxface-511) > > ACPI Exception: AE_AML_NO_OPERAND, While evaluating SleepState > [\_S3_], > > bad Sleep object ffff880119a33cf0 type Package > > (20120711/hwxface-543) > > > > are logged and suspend doesn't work, > > > > - on SeaBIOS, which provides multi-element packages, suspend works. > > > > With the patch, the messages are gone on OVMF, and the VM can be > > suspended with both boot firmwares. > > > > [1] http://lists.acpica.org/pipermail/devel/2012-December/000405.html > > [2] > > http://tianocore.git.sourceforge.net/git/gitweb.cgi?p=tianocore/edk2;a > > =commitdiff;h=14430c55c8d0e9a8487c2c74155e63299632ef5e > > Well, the patch modifies ACPICA, so it would need to go through the ACPICA > upstream for us to take it to the kernel. > > Adding a CC to Bob Moore, who's the upstream ACPICA maintainer. > > Thanks, > Rafael > > > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > drivers/acpi/acpica/hwxface.c | 60 ++++++++++++++++++++++------------ > ------- > > 1 files changed, 32 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/acpi/acpica/hwxface.c > > b/drivers/acpi/acpica/hwxface.c index 05a154c..6cd5bb0 100644 > > --- a/drivers/acpi/acpica/hwxface.c > > +++ b/drivers/acpi/acpica/hwxface.c > > @@ -499,39 +499,43 @@ acpi_get_sleep_type_data(u8 sleep_state, u8 > *sleep_type_a, u8 *sleep_type_b) > > } > > > > /* > > - * The package must have at least two elements. NOTE (March 2005): > This > > - * goes against the current ACPI spec which defines this object as a > > - * package with one encoded DWORD element. However, existing > practice > > - * by BIOS vendors seems to be to have 2 or more elements, at least > > - * one per sleep type (A/B). > > + * The current ACPI spec (v 5.0) defines this object as a package > with > > + * one encoded DWORD element. > > + * > > + * Existing practice by BIOS vendors seems to be to have 2 or more > > + * elements, at least one per sleep type (A/B). > > */ > > - else if (info->return_object->package.count < 2) { > > + else if (info->return_object->package.count == 0) { > > ACPI_ERROR((AE_INFO, > > - "Sleep State return package does not have at least > two elements")); > > + "Sleep State return package does not have > elements")); > > status = AE_AML_NO_OPERAND; > > - } > > - > > - /* The first two elements must both be of type Integer */ > > - > > - else if (((info->return_object->package.elements[0])->common.type > > - != ACPI_TYPE_INTEGER) || > > - ((info->return_object->package.elements[1])->common.type > > - != ACPI_TYPE_INTEGER)) { > > - ACPI_ERROR((AE_INFO, > > - "Sleep State return package elements are not both > Integers " > > - "(%s, %s)", > > - acpi_ut_get_object_type_name(info->return_object-> > > - package.elements[0]), > > - acpi_ut_get_object_type_name(info->return_object-> > > - package.elements[1]))); > > - status = AE_AML_OPERAND_TYPE; > > } else { > > - /* Valid _Sx_ package size, type, and value */ > > + union acpi_operand_object **elements; > > > > - *sleep_type_a = (u8) > > - (info->return_object->package.elements[0])->integer.value; > > - *sleep_type_b = (u8) > > - (info->return_object->package.elements[1])->integer.value; > > + elements = info->return_object->package.elements; > > + > > + if (elements[0]->common.type != ACPI_TYPE_INTEGER) { > > + ACPI_ERROR((AE_INFO, > > + "1st element in Sleep State return package is > not Integer " > > + "(%s)", > > + acpi_ut_get_object_type_name(elements[0]))); > > + status = AE_AML_OPERAND_TYPE; > > + } > > + else if (info->return_object->package.count == 1) { > > + *sleep_type_a = (u8) elements[0]->integer.value; > > + *sleep_type_b = (u8)(elements[0]->integer.value >> 8); > > + } > > + else if (elements[1]->common.type != ACPI_TYPE_INTEGER) { > > + ACPI_ERROR((AE_INFO, > > + "2nd element in Sleep State return package is > not Integer " > > + "(%s)", > > + acpi_ut_get_object_type_name(elements[1]))); > > + status = AE_AML_OPERAND_TYPE; > > + } > > + else { > > + *sleep_type_a = (u8)elements[0]->integer.value; > > + *sleep_type_b = (u8)elements[1]->integer.value; > > + } > > } > > > > if (ACPI_FAILURE(status)) { > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-12-20 21:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-20 17:31 [PATCH 0/1] acpi_get_sleep_type_data(): accept package with one element Laszlo Ersek 2012-12-20 17:31 ` [PATCH 1/1] " Laszlo Ersek 2012-12-20 21:22 ` Rafael J. Wysocki 2012-12-20 21:39 ` Moore, Robert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).