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@xxxxxxx] > Sent: Thursday, December 20, 2012 1:23 PM > To: Laszlo Ersek > Cc: linux-acpi@xxxxxxxxxxxxxxx; 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@xxxxxxxxxx> > > --- > > 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. ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
