[PATCH 0/1] acpi_get_sleep_type_data(): accept package with one element

[PATCH 0/1] acpi_get_sleep_type_data(): accept package with one element

[Laszlo Ersek]

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(-)

[PATCH 1/1] acpi_get_sleep_type_data(): accept package with one element

[Laszlo Ersek]

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

Re: [PATCH 1/1] acpi_get_sleep_type_data(): accept package with one element

[Rafael J. Wysocki]

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.

RE: [PATCH 1/1] acpi_get_sleep_type_data(): accept package with one element

[Moore, Robert]

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.