Re: PROBLEM: Calling ObjectType on buffer field reports type integer

Re: PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz]

On 3/22/19 10:28 PM, Schmauss, Erik wrote:
> I've been discussing this with Bob. I've decided to file a bug against Microsoft
> internally. I would like to wait and see what they say... I've never done such
> things so I don't know how long the process takes to get a response form them.
> 
> I'll post updates when I get them but feel free to ping us again in a few
> weeks if you don't hear back. We're still investigating their operation region
> behavior as well...
> 	
> Erik

Hi,

I assume there hasn't been any response from Microsoft?

Maximilian

RE: PROBLEM: Calling ObjectType on buffer field reports type integer

[Schmauss, Erik]

> -----Original Message-----
> From: Maximilian Luz [mailto:luzmaximilian@gmail.com]
> Sent: Saturday, July 20, 2019 1:45 PM
> To: Schmauss, Erik <erik.schmauss@intel.com>; Moore, Robert
> <robert.moore@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org; devel@acpica.org
> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type integer
> 
> On 3/22/19 10:28 PM, Schmauss, Erik wrote:
> > I've been discussing this with Bob. I've decided to file a bug against
> > Microsoft internally. I would like to wait and see what they say...
> > I've never done such things so I don't know how long the process takes to get
> a response form them.
> >
> > I'll post updates when I get them but feel free to ping us again in a
> > few weeks if you don't hear back. We're still investigating their
> > operation region behavior as well...
> >
> > Erik
> 
> Hi,
> 
> I assume there hasn't been any response from Microsoft?

Sorry about the late response. This slipped through the cracks.
I've sent them an email just now and I'll keep you informed

Thanks,
Erik
> 
> Maximilian

Re: PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz]

On 7/23/19 1:01 AM, Schmauss, Erik wrote:
> Sorry about the late response. This slipped through the cracks.
> I've sent them an email just now and I'll keep you informed

No worries. Thank you.

Maximilian

Re: PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz]

On 7/23/19 1:01 AM, Schmauss, Erik wrote:
> Sorry about the late response. This slipped through the cracks.
> I've sent them an email just now and I'll keep you informed

Hi again,

is there any update on this?

Regards,

Maximilian

RE: PROBLEM: Calling ObjectType on buffer field reports type integer

[Moore, Robert]

We will probably make this change, depending on what Windows does.
Bob


-----Original Message-----
From: Maximilian Luz <luzmaximilian@gmail.com> 
Sent: Sunday, November 10, 2019 1:30 PM
To: Schmauss, Erik <erik.schmauss@intel.com>; Moore, Robert <robert.moore@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>
Cc: linux-acpi@vger.kernel.org; devel@acpica.org
Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type integer


On 7/23/19 1:01 AM, Schmauss, Erik wrote:
> Sorry about the late response. This slipped through the cracks.
> I've sent them an email just now and I'll keep you informed

Hi again,

is there any update on this?

Regards,

Maximilian

RE: PROBLEM: Calling ObjectType on buffer field reports type integer

[Moore, Robert]

I'm not seeing this problem. For example:
    Method (DS01)
    {
        Name(BUFZ, Buffer(48){})
        CreateField(BUFZ, 192, 69, DST0)
        Store (ObjectType (DST0), Debug)
    }

Acpiexec dsdt.aml
Eval DS01
Evaluating \DS01
ACPI Debug:  0x000000000000000E

0x0E is in fact type "buffer field".


-----Original Message-----
From: Moore, Robert <robert.moore@intel.com> 
Sent: Friday, November 22, 2019 9:12 AM
To: Maximilian Luz <luzmaximilian@gmail.com>; Schmauss, Erik <erik.schmauss@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>
Cc: linux-acpi@vger.kernel.org; devel@acpica.org
Subject: [Devel] Re: PROBLEM: Calling ObjectType on buffer field reports type integer

We will probably make this change, depending on what Windows does.
Bob


-----Original Message-----
From: Maximilian Luz <luzmaximilian@gmail.com> 
Sent: Sunday, November 10, 2019 1:30 PM
To: Schmauss, Erik <erik.schmauss@intel.com>; Moore, Robert <robert.moore@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>
Cc: linux-acpi@vger.kernel.org; devel@acpica.org
Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type integer


On 7/23/19 1:01 AM, Schmauss, Erik wrote:
> Sorry about the late response. This slipped through the cracks.
> I've sent them an email just now and I'll keep you informed

Hi again,

is there any update on this?

Regards,

Maximilian
_______________________________________________
Devel mailing list -- devel@acpica.org
To unsubscribe send an email to devel-leave@acpica.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

Re: PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz]

On 11/22/19 11:07 PM, Moore, Robert wrote:
> I'm not seeing this problem. For example:
>      Method (DS01)
>      {
>          Name(BUFZ, Buffer(48){})
>          CreateField(BUFZ, 192, 69, DST0)
>          Store (ObjectType (DST0), Debug)
>      }
> 
> Acpiexec dsdt.aml
> Eval DS01
> Evaluating \DS01
> ACPI Debug:  0x000000000000000E
> 
> 0x0E is in fact type "buffer field".

Hi,

unfortunately, the bug is a bit more convoluted, here's a minimal
example:

     Method (DS01)
     {
         Name(BUFZ, Buffer(48){})
         CreateField(BUFZ, 192, 32, DST0)
         Local0 = DST0
         Store (ObjectType (Local0), Debug)
     }

It is based on the combination of CreateField with length smaller or
equal to the maximum integer size and the assignment of the field to a
local variable. For me, this yields

Eval DS01
Evaluating \DS01
ACPI Debug:  0x0000000000000001

As already iterated, this should be the correct behavior according to
spec, but causes failures on Microsoft Surface devices.

For reference, I've added two links to the relevant parts in the DSDT of
the MS Surface Book 2: In short, MS identifies errors by the type of a
local variable which is supposed to either contain a payload (buffer) or
an error value (integer). This local variable (Local4) is previously
assigned from a buffer field [1] containing some payload data. However
on Linux, if the payload data is smaller than or equal to 64 bits, the
type of the variable is integer and the transaction gets misidentified
as having failed [2].

[1]: https://github.com/qzed/surfacebook2-dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L15396-L15397
[2]: https://github.com/qzed/surfacebook2-dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L15427

In hindsight, the subject line is not describing the actual problem very
well, I apologize for that.

Thanks,
Maximilian

RE: PROBLEM: Calling ObjectType on buffer field reports type integer

[Schmauss, Erik]

Bob and I started debugging this and we found the issue:

Let's say that we have this code:

Name (BUF1, Buffer (0x10) {})
Method (M001)
{
    CreateField (BUF1, 1, 72, FLD0)
    local0 = FLD0 // BUG: store operator (aka =)  converts FLD0 into an integer
    return (ObjectType (local0)) // Integer is returned
}

Although FLD0's value is small enough to fit in an integer, the bit length of FLD0 exceeds 64 bits so local0 should actually be a Buffer type.

This is likely an issue in the implicit object conversion rules implemented in the store operator. I'll take a look at this next week or the week after...

Thanks for your patience,
Erik

> -----Original Message-----
> From: Moore, Robert <robert.moore@intel.com>
> Sent: Friday, November 22, 2019 2:07 PM
> To: Moore, Robert <robert.moore@intel.com>; Maximilian Luz
> <luzmaximilian@gmail.com>; Schmauss, Erik <erik.schmauss@intel.com>;
> Wysocki, Rafael J <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org; devel@acpica.org
> Subject: RE: PROBLEM: Calling ObjectType on buffer field reports type integer
> 
> I'm not seeing this problem. For example:
>     Method (DS01)
>     {
>         Name(BUFZ, Buffer(48){})
>         CreateField(BUFZ, 192, 69, DST0)
>         Store (ObjectType (DST0), Debug)
>     }
> 
> Acpiexec dsdt.aml
> Eval DS01
> Evaluating \DS01
> ACPI Debug:  0x000000000000000E
> 
> 0x0E is in fact type "buffer field".
> 
> 
> -----Original Message-----
> From: Moore, Robert <robert.moore@intel.com>
> Sent: Friday, November 22, 2019 9:12 AM
> To: Maximilian Luz <luzmaximilian@gmail.com>; Schmauss, Erik
> <erik.schmauss@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org; devel@acpica.org
> Subject: [Devel] Re: PROBLEM: Calling ObjectType on buffer field reports type
> integer
> 
> We will probably make this change, depending on what Windows does.
> Bob
> 
> 
> -----Original Message-----
> From: Maximilian Luz <luzmaximilian@gmail.com>
> Sent: Sunday, November 10, 2019 1:30 PM
> To: Schmauss, Erik <erik.schmauss@intel.com>; Moore, Robert
> <robert.moore@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org; devel@acpica.org
> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type integer
> 
> 
> On 7/23/19 1:01 AM, Schmauss, Erik wrote:
> > Sorry about the late response. This slipped through the cracks.
> > I've sent them an email just now and I'll keep you informed
> 
> Hi again,
> 
> is there any update on this?
> 
> Regards,
> 
> Maximilian
> _______________________________________________
> Devel mailing list -- devel@acpica.org
> To unsubscribe send an email to devel-leave@acpica.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

Re: PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz]

On 11/23/19 12:29 AM, Schmauss, Erik wrote:
> Bob and I started debugging this and we found the issue:
> 
> Let's say that we have this code:
> 
> Name (BUF1, Buffer (0x10) {})
> Method (M001)
> {
>      CreateField (BUF1, 1, 72, FLD0)
>      local0 = FLD0 // BUG: store operator (aka =)  converts FLD0 into an integer
>      return (ObjectType (local0)) // Integer is returned
> }
> 
> Although FLD0's value is small enough to fit in an integer, the bit length of FLD0 exceeds 64 bits so local0 should actually be a Buffer type.
> 
> This is likely an issue in the implicit object conversion rules implemented in the store operator. I'll take a look at this next week or the week after...

This looks like a separate problem to me. On the SB2 there is this piece
of code, simplified:

     Name(RQBF, Buffer (0xFF) {})
     CreateByteField (RQBF, 0x03, ALEN)

     // ...
     // GenericSerialBus/AttribRawProcessBytes access to fill RQBF
     // ...

     If (/* success and everything is valid */)
     {
         Local3 = (ALEN * 0x08)
         CreateField (RQBF, 0x20, Local3, ARB)
         Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
     }
     Else
     {
         Local4 = 0x01   // or some other error code as integer
     }

     // ...
     // some more stuff
     // ...

     If ((ObjectType (Local4) == One /* Integer */))
     {
         // notify that an error has occurred
     }
     Else
     {
         // success and actually use data from Local4
     }

The code in question basically unpacks a payload from some other
management stuff sent over the OperationRegion.

Here, ALEN is the length of a dynamically sized payload in bytes, which
is obtained from the data returned by the OperationRegion access. This
can for example be 4, making the field length 32 bit. So this is not an
issue of the field length being larger than intmax bits, it actually is
sometimes only 32 bits, or 8 bits, depending on the response of the
driver connected to the OperationRegion. Also the DSDT depends on that,
see the example below.

Just to reiterate, the code works fine for payloads with ALEN > 8 (so
more than 8 bytes), but fails for anything less.

Also note that this is not something that can be fixed by just telling
the GenericSerialBus/OperationRegion driver to just return 9 bytes
instead: There are length-checks on Local4 further down the line to
validate it actually contains what was requested.

An example of how this piece of code is actually used, if that helps
(again simplified):

     Method (RQST, 1)
     {
         // pretty much the code above
         Return (Local4)     // either payload or integer error code
     }

     Scope (_SB.BAT1)
     {
         Method (_STA, 0)
         {
             Local0 = RQST(0x01)     // request battery status

             If ((ObjectType (Local0) == 0x03))      // is buffer type
             {
                 If ((SizeOf (Local0) == 0x04))      // has length 4
                 {
                     CreateDWordField (Local0, 0, BAST)
                     Return (BAST)
                 }
             }

             Return (0x00)           // return default value
         }
     }


Regards,
Maximilian

RE: PROBLEM: Calling ObjectType on buffer field reports type integer

[Schmauss, Erik]

> 
> This looks like a separate problem to me. On the SB2 there is this piece of code,
> simplified:
> 
>      Name(RQBF, Buffer (0xFF) {})
>      CreateByteField (RQBF, 0x03, ALEN)
> 
>      // ...
>      // GenericSerialBus/AttribRawProcessBytes access to fill RQBF
>      // ...
> 
>      If (/* success and everything is valid */)
>      {
>          Local3 = (ALEN * 0x08)
>          CreateField (RQBF, 0x20, Local3, ARB)
>          Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
>      }
>      Else
>      {
>          Local4 = 0x01   // or some other error code as integer
>      }
> 
>      // ...
>      // some more stuff
>      // ...
> 
>      If ((ObjectType (Local4) == One /* Integer */))
>      {
>          // notify that an error has occurred
>      }
>      Else
>      {
>          // success and actually use data from Local4
>      }
> 
> The code in question basically unpacks a payload from some other
> management stuff sent over the OperationRegion.
> 
> Here, ALEN is the length of a dynamically sized payload in bytes, which is
> obtained from the data returned by the OperationRegion access. This can for
> example be 4, making the field length 32 bit. So this is not an issue of the field
> length being larger than intmax bits, it actually is sometimes only 32 bits, or 8
> bits, depending on the response of the driver connected to the
> OperationRegion. Also the DSDT depends on that, see the example below.
> 
> Just to reiterate, the code works fine for payloads with ALEN > 8 (so more than
> 8 bytes), but fails for anything less.

Forget what I said in the previous email. You are correct.

They treat bufferfields that are small enough to fit inside of an integer as a buffer or bufferfield.

I did a bunch of testing on windows and determined that, objects created by Create(Bit|Byte|Word|DWord|Qword)Field
work the same way on both interpreters. They get converted to Integers as outlined in the spec.

We simply need to stop perform the BufferField->Integer conversion for buffer fields created by the ASL CreateField() operators.
I'll get a solution hopefully sometime next week..

Thanks,
Erik

> 
> Also note that this is not something that can be fixed by just telling the
> GenericSerialBus/OperationRegion driver to just return 9 bytes
> instead: There are length-checks on Local4 further down the line to validate it
> actually contains what was requested.
> 
> An example of how this piece of code is actually used, if that helps (again
> simplified):
> 
>      Method (RQST, 1)
>      {
>          // pretty much the code above
>          Return (Local4)     // either payload or integer error code
>      }
> 
>      Scope (_SB.BAT1)
>      {
>          Method (_STA, 0)
>          {
>              Local0 = RQST(0x01)     // request battery status
> 
>              If ((ObjectType (Local0) == 0x03))      // is buffer type
>              {
>                  If ((SizeOf (Local0) == 0x04))      // has length 4
>                  {
>                      CreateDWordField (Local0, 0, BAST)
>                      Return (BAST)
>                  }
>              }
> 
>              Return (0x00)           // return default value
>          }
>      }
> 
> 
> Regards,
> Maximilian

RE: PROBLEM: Calling ObjectType on buffer field reports type integer

[Kaneda, Erik]

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org <linux-acpi-
> owner@vger.kernel.org> On Behalf Of Schmauss, Erik
> Sent: Tuesday, November 26, 2019 4:26 PM
> To: Maximilian Luz <luzmaximilian@gmail.com>; Moore, Robert
> <robert.moore@intel.com>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org; devel@acpica.org
> Subject: RE: PROBLEM: Calling ObjectType on buffer field reports
> type integer
> 
> >
> > This looks like a separate problem to me. On the SB2 there is
> this
> > piece of code,
> > simplified:
> >
> >      Name(RQBF, Buffer (0xFF) {})
> >      CreateByteField (RQBF, 0x03, ALEN)
> >
> >      // ...
> >      // GenericSerialBus/AttribRawProcessBytes access to fill RQBF
> >      // ...
> >
> >      If (/* success and everything is valid */)
> >      {
> >          Local3 = (ALEN * 0x08)
> >          CreateField (RQBF, 0x20, Local3, ARB)
> >          Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
> >      }
> >      Else
> >      {
> >          Local4 = 0x01   // or some other error code as integer
> >      }
> >
> >      // ...
> >      // some more stuff
> >      // ...
> >
> >      If ((ObjectType (Local4) == One /* Integer */))
> >      {
> >          // notify that an error has occurred
> >      }
> >      Else
> >      {
> >          // success and actually use data from Local4
> >      }
> >
> > The code in question basically unpacks a payload from some other
> > management stuff sent over the OperationRegion.
> >
> > Here, ALEN is the length of a dynamically sized payload in bytes,
> > which is obtained from the data returned by the
> OperationRegion
> > access. This can for example be 4, making the field length 32 bit.
> So
> > this is not an issue of the field length being larger than intmax
> > bits, it actually is sometimes only 32 bits, or 8 bits, depending
> on
> > the response of the driver connected to the OperationRegion.
> Also the DSDT depends on that, see the example below.
> >
> > Just to reiterate, the code works fine for payloads with ALEN > 8
> (so
> > more than
> > 8 bytes), but fails for anything less.
> 
> Forget what I said in the previous email. You are correct.
> 
> They treat bufferfields that are small enough to fit inside of an
> integer as a buffer or bufferfield.
> 
> I did a bunch of testing on windows and determined that, objects
> created by Create(Bit|Byte|Word|DWord|Qword)Field
> work the same way on both interpreters. They get converted to
> Integers as outlined in the spec.
> 
> We simply need to stop perform the BufferField->Integer
> conversion for buffer fields created by the ASL CreateField()
> operators.
> I'll get a solution hopefully sometime next week..
> 
Hi Maximilian,

Please try the patch below:

From 9a949c05253e36f69451c0e1af4f862d76820c8a Mon Sep 17 00:00:00 2001
From: Erik Schmauss <erik.schmauss@intel.com>
Date: Tue, 3 Dec 2019 12:42:51 -0800
From: Erik Schmauss <erik.schmauss@intel.com>
Subject: [PATCH 1] ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator

According to table 19-419 of the ACPI 6.3 specification, buffer_fields
created using the ASL create_field() Operator have been treated as
integers if the buffer_field is small enough to fit inside of an ASL
integer (32-bits or 64-bits depending on the definition block
revision). If they are larger, buffer fields are treated as ASL
Buffer objects. However, this is not true for other AML interpreter
implementations.

It has been discovered that other AML interpreters always treat
buffer fields created by create_field() as a buffer regardless of the
length of the buffer field.

This change prohibits the previously mentioned integer conversion to
match other AML interpreter implementations.

Reported-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Erik Schmauss <erik.schmauss@intel.com>
---
 acobject.h |    3 ++-
 dsutils.c  |    3 +++
 exfield.c  |   10 ++++++++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff -Nurp linux.before_name/drivers/acpi/acpica/acobject.h linux.after_name/drivers/acpi/acpica/acobject.h
--- linux.before_name/drivers/acpi/acpica/acobject.h	2019-12-03 13:36:21.802578716 -0800
+++ linux.after_name/drivers/acpi/acpica/acobject.h	2019-12-03 13:36:18.267579061 -0800
@@ -259,7 +259,8 @@ struct acpi_object_index_field {
 /* The buffer_field is different in that it is part of a Buffer, not an op_region */
 
 struct acpi_object_buffer_field {
-	ACPI_OBJECT_COMMON_HEADER ACPI_COMMON_FIELD_INFO union acpi_operand_object *buffer_obj;	/* Containing Buffer object */
+	ACPI_OBJECT_COMMON_HEADER ACPI_COMMON_FIELD_INFO u8 is_create_field;	/* Special case for objects created by create_field() */
+	union acpi_operand_object *buffer_obj;	/* Containing Buffer object */
 };
 
 /******************************************************************************
diff -Nurp linux.before_name/drivers/acpi/acpica/dsutils.c linux.after_name/drivers/acpi/acpica/dsutils.c
--- linux.before_name/drivers/acpi/acpica/dsutils.c	2019-12-03 13:36:21.782578718 -0800
+++ linux.after_name/drivers/acpi/acpica/dsutils.c	2019-12-03 13:36:17.765579110 -0800
@@ -470,6 +470,9 @@ acpi_ds_create_operand(struct acpi_walk_
 			    ACPI_CAST_PTR(union acpi_operand_object,
 					  walk_state->deferred_node);
 			status = AE_OK;
+			if (walk_state->opcode == AML_CREATE_FIELD_OP) {
+				obj_desc->buffer_field.is_create_field = TRUE;
+			}
 		} else {	/* All other opcodes */

 			/*
diff -Nurp linux.before_name/drivers/acpi/acpica/exfield.c linux.after_name/drivers/acpi/acpica/exfield.c
--- linux.before_name/drivers/acpi/acpica/exfield.c	2019-12-03 13:36:21.791578717 -0800
+++ linux.after_name/drivers/acpi/acpica/exfield.c	2019-12-03 13:36:18.257579062 -0800
@@ -95,7 +95,8 @@ acpi_ex_get_protocol_buffer_length(u32 p
  * RETURN:      Status
  *
  * DESCRIPTION: Read from a named field. Returns either an Integer or a
- *              Buffer, depending on the size of the field.
+ *              Buffer, depending on the size of the field and whether if a
+ *              field is created by the create_field() operator.
  *
  ******************************************************************************/
 
@@ -153,12 +154,17 @@ acpi_ex_read_data_from_field(struct acpi
 	 * the use of arithmetic operators on the returned value if the
 	 * field size is equal or smaller than an Integer.
 	 *
+	 * However, all buffer fields created by create_field operator needs to
+	 * remain as a buffer to match other AML interpreter implementations.
+	 *
 	 * Note: Field.length is in bits.
 	 */
 	buffer_length =
 	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc->field.bit_length);
 
-	if (buffer_length > acpi_gbl_integer_byte_width) {
+	if (buffer_length > acpi_gbl_integer_byte_width ||
+	    (obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD &&
+	     obj_desc->buffer_field.is_create_field)) {
 
 		/* Field is too large for an Integer, create a Buffer instead */

> Thanks,
> Erik
> 

Re: PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz]

On 12/3/19 11:21 PM, Kaneda, Erik wrote:
> Hi Maximilian,
> 
> Please try the patch below:

Hi,

I've tested this on 5.4.1 and the current upstream master now.
Unfortunately it doesn't seem to fix the issue.

Specifically, it seems like the flag doesn't get set here
(printk-debugging indicates that the code in the if never runs):

>   /******************************************************************************
> diff -Nurp linux.before_name/drivers/acpi/acpica/dsutils.c linux.after_name/drivers/acpi/acpica/dsutils.c
> --- linux.before_name/drivers/acpi/acpica/dsutils.c	2019-12-03 13:36:21.782578718 -0800
> +++ linux.after_name/drivers/acpi/acpica/dsutils.c	2019-12-03 13:36:17.765579110 -0800
> @@ -470,6 +470,9 @@ acpi_ds_create_operand(struct acpi_walk_
>   			    ACPI_CAST_PTR(union acpi_operand_object,
>   					  walk_state->deferred_node);
>   			status = AE_OK;
> +			if (walk_state->opcode == AML_CREATE_FIELD_OP) {
> +				obj_desc->buffer_field.is_create_field = TRUE;
> +			}
>   		} else {	/* All other opcodes */
> 
>   			/*

That of course means that the if condition here is still not fulfilled,
so the problem is not fixed.

> diff -Nurp linux.before_name/drivers/acpi/acpica/exfield.c linux.after_name/drivers/acpi/acpica/exfield.c
> --- linux.before_name/drivers/acpi/acpica/exfield.c	2019-12-03 13:36:21.791578717 -0800
> +++ linux.after_name/drivers/acpi/acpica/exfield.c	2019-12-03 13:36:18.257579062 -0800
> @@ -153,12 +154,17 @@ acpi_ex_read_data_from_field(struct acpi
>   	 * the use of arithmetic operators on the returned value if the
>   	 * field size is equal or smaller than an Integer.
>   	 *
> +	 * However, all buffer fields created by create_field operator needs to
> +	 * remain as a buffer to match other AML interpreter implementations.
> +	 *
>   	 * Note: Field.length is in bits.
>   	 */
>   	buffer_length =
>   	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc->field.bit_length);
>   
> -	if (buffer_length > acpi_gbl_integer_byte_width) {
> +	if (buffer_length > acpi_gbl_integer_byte_width ||
> +	    (obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD &&
> +	     obj_desc->buffer_field.is_create_field)) {
>   
>   		/* Field is too large for an Integer, create a Buffer instead */
> 

If I'm not mistaken I've proposed a patch here that is conceptually
quite similar (but may have some flaws or cause other issues, as I'm not
really familiar with the ACPICA code-base). My idea was to modify
acpi_ds_init_buffer_field (dsopcode.c) instead of
acpi_ds_create_operand. As said, I'm not sure however if this may have
any other consequences that I'm unaware of.

Also I've avoided creating an is_create_field flag by changing the field
access from AML_FIELD_ACCESS_BYTE to AML_FIELD_ACCESS_BUFFER (which
doesn't seem to be used anywhere else) and checking that, but the
general idea of extending the if condition in
acpi_ex_read_data_from_field is the same.

Link to the patch:
https://github.com/qzed/linux-surfacegen5-acpi/blob/2014964bac52f138109443de3e92dc2c6cff5e83/patches/5.3/0001-ACPI-Fix-buffer-integer-type-mismatch.patch

Thanks,
Maximilian

RE: PROBLEM: Calling ObjectType on buffer field reports type integer

[Kaneda, Erik]

> -----Original Message-----
> From: Maximilian Luz <luzmaximilian@gmail.com>
> Sent: Wednesday, December 4, 2019 11:34 AM
> To: Kaneda, Erik <erik.kaneda@intel.com>; Moore, Robert
> <robert.moore@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org; devel@acpica.org
> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type integer
> 
> On 12/3/19 11:21 PM, Kaneda, Erik wrote:
> > Hi Maximilian,
> >
> > Please try the patch below:
> 
> Hi,
> 
> I've tested this on 5.4.1 and the current upstream master now.
> Unfortunately it doesn't seem to fix the issue.
> 
> Specifically, it seems like the flag doesn't get set here (printk-debugging
> indicates that the code in the if never runs):
> 
> >
> >
> /******************************************************************
> ***
> > ********* diff -Nurp linux.before_name/drivers/acpi/acpica/dsutils.c
> > linux.after_name/drivers/acpi/acpica/dsutils.c
> > --- linux.before_name/drivers/acpi/acpica/dsutils.c	2019-12-03
> 13:36:21.782578718 -0800
> > +++ linux.after_name/drivers/acpi/acpica/dsutils.c	2019-12-03
> 13:36:17.765579110 -0800
> > @@ -470,6 +470,9 @@ acpi_ds_create_operand(struct acpi_walk_
> >   			    ACPI_CAST_PTR(union acpi_operand_object,
> >   					  walk_state->deferred_node);
> >   			status = AE_OK;
> > +			if (walk_state->opcode == AML_CREATE_FIELD_OP) {
> > +				obj_desc->buffer_field.is_create_field = TRUE;
> > +			}
> >   		} else {	/* All other opcodes */
> >
> >   			/*
> 
> That of course means that the if condition here is still not fulfilled, so the
> problem is not fixed.
> 
> > diff -Nurp linux.before_name/drivers/acpi/acpica/exfield.c
> linux.after_name/drivers/acpi/acpica/exfield.c
> > --- linux.before_name/drivers/acpi/acpica/exfield.c	2019-12-03
> 13:36:21.791578717 -0800
> > +++ linux.after_name/drivers/acpi/acpica/exfield.c	2019-12-03
> 13:36:18.257579062 -0800
> > @@ -153,12 +154,17 @@ acpi_ex_read_data_from_field(struct acpi
> >   	 * the use of arithmetic operators on the returned value if the
> >   	 * field size is equal or smaller than an Integer.
> >   	 *
> > +	 * However, all buffer fields created by create_field operator needs to
> > +	 * remain as a buffer to match other AML interpreter implementations.
> > +	 *
> >   	 * Note: Field.length is in bits.
> >   	 */
> >   	buffer_length =
> >
> > (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc->field.bit_length);
> >
> > -	if (buffer_length > acpi_gbl_integer_byte_width) {
> > +	if (buffer_length > acpi_gbl_integer_byte_width ||
> > +	    (obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD &&
> > +	     obj_desc->buffer_field.is_create_field)) {
> >
> >   		/* Field is too large for an Integer, create a Buffer instead */
> >
> 
> If I'm not mistaken I've proposed a patch here that is conceptually quite similar
> (but may have some flaws or cause other issues, as I'm not really familiar with
> the ACPICA code-base). My idea was to modify acpi_ds_init_buffer_field
> (dsopcode.c) instead of acpi_ds_create_operand. As said, I'm not sure however
> if this may have any other consequences that I'm unaware of.
> 
> Also I've avoided creating an is_create_field flag by changing the field access
> from AML_FIELD_ACCESS_BYTE to AML_FIELD_ACCESS_BUFFER (which doesn't
> seem to be used anywhere else) and checking that, but the general idea of
> extending the if condition in acpi_ex_read_data_from_field is the same.
> 
> Link to the patch:
> https://github.com/qzed/linux-surfacegen5-
> acpi/blob/2014964bac52f138109443de3e92dc2c6cff5e83/patches/5.3/0001-
> ACPI-Fix-buffer-integer-type-mismatch.patch

Your solution looks very similar to my patch.
In AcpiDsInitBufferField, I would prefer to use a new field "is_create_field" or something like that. AML_FIELD_ACCESS_BUFFER
does get used in other parts of ACPICA so let's create and use a new field in the object union.

Go ahead and re-submit the patch with the above changes. I'll edit your explanation with more details and include this in
the next ACPICA release. It'll land in linux after the release.

Thanks!
Erik
> 
> Thanks,
> Maximilian

Re: PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz]

On 12/6/19 12:57 AM, Kaneda, Erik wrote:
> Your solution looks very similar to my patch.
> In AcpiDsInitBufferField, I would prefer to use a new field "is_create_field" or something like that. AML_FIELD_ACCESS_BUFFER
> does get used in other parts of ACPICA so let's create and use a new field in the object union.
> 
> Go ahead and re-submit the patch with the above changes. I'll edit your explanation with more details and include this in
> the next ACPICA release. It'll land in linux after the release.

Thank you!

I've re-submitted the patch with the requested changes and included your
comments.

Regards,
Maximilian