PROBLEM: Calling ObjectType on buffer field reports type integer

Commit Message

Maximilian Luz March 14, 2019, 2:24 p.m. UTC

Hi all,

it seems that buffer fields (created via CreateField) are incorrectly
reported as being of type integer (via ObjectType) when they are small
enough to allow for that.

As far as I know all kernel versions are affected by this, I have
specifically checked 5.0 (5.0.2), 4.20, 4.19 and 4.18.

In more detail: On the Microsoft Surface Book 2, Surface Pro (2017),
Surface Pro 6, Surface Laptop and Surface Laptop 2 devices there is the
following piece of AML code:

     Local0 = ^^_SAN.RQST (0x02, One, One, Zero, One)
     If ((ObjectType (Local0) != 0x03))
     {
         // Error path
     }
     Else
     {
         // Success path
     }

Which executes a command (RQST) and then checks the type of the returned
field to determine if that command was successful (i.e. returned a
buffer object) or failed (i.e. returned any other type, including
integer). The buffer field that is being returned by RQST is crated as
follows:

     CreateField (RQBF, 0x20, Local3, ARB)
     Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
     ...
     Return (Local4)

If the returned buffer field is small enough (smaller than
acpi_gbl_integer_byte_width), the error-path will always be executed.

The full DSDT for the Surface Book 2 can be found here:
https://github.com/qzed/surfacebook2-dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L15396

I have attached a patch (for 5.0) that fixes this issue and has been in
use on the affected devices for a few months now via

     https://github.com/qzed/linux-surfacegen5-acpi and
     https://github.com/jakeday/linux-surface/

I'm not aware of any issues regarding the patch, but then again this
has, to my knowledge, only been tested on Microsoft Surface devices.

Maximilian

Signed-off-by: Maximilian Luz <[e-mail]>
---
  drivers/acpi/acpica/dsopcode.c |  2 +-
  drivers/acpi/acpica/exfield.c  | 12 +++++++++---
  2 files changed, 10 insertions(+), 4 deletions(-)

Comments

Schmauss, Erik March 14, 2019, 5:19 p.m. UTC | #1

> -----Original Message-----
> From: Maximilian Luz [mailto:[e-mail]]
> Sent: Thursday, March 14, 2019 7:24 AM
> To: Moore, Robert <[e-mail]>; Schmauss, Erik
> <[e-mail]>; Wysocki, Rafael J
> <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: PROBLEM: Calling ObjectType on buffer field reports type
> integer
> 
> Hi all,
> 
> it seems that buffer fields (created via CreateField) are incorrectly
> reported as being of type integer (via ObjectType) when they are small
> enough to allow for that.
> 
> As far as I know all kernel versions are affected by this, I have
> specifically checked 5.0 (5.0.2), 4.20, 4.19 and 4.18.
> 
> In more detail: On the Microsoft Surface Book 2, Surface Pro (2017),
> Surface Pro 6, Surface Laptop and Surface Laptop 2 devices there is the
> following piece of AML code:
> 
>      Local0 = ^^_SAN.RQST (0x02, One, One, Zero, One)
>      If ((ObjectType (Local0) != 0x03))
>      {
>          // Error path
>      }
>      Else
>      {
>          // Success path
>      }
> 
> Which executes a command (RQST) and then checks the type of the
> returned field to determine if that command was successful (i.e.
> returned a buffer object) or failed (i.e. returned any other type,
> including integer). The buffer field that is being returned by RQST is
> crated as
> follows:
> 
>      CreateField (RQBF, 0x20, Local3, ARB)
>      Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
>      ...
>      Return (Local4)
> 
> If the returned buffer field is small enough (smaller than
> acpi_gbl_integer_byte_width), the error-path will always be executed.
> 
> The full DSDT for the Surface Book 2 can be found here:
> https://github.com/qzed/surfacebook2-
> dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L153
> 96
> 
> I have attached a patch (for 5.0) that fixes this issue and has been in
> use on the affected devices for a few months now via
> 
>      https://github.com/qzed/linux-surfacegen5-acpi and
>      https://github.com/jakeday/linux-surface/
> 
> I'm not aware of any issues regarding the patch, but then again this
> has, to my knowledge, only been tested on Microsoft Surface devices.

I'll take a look at this and test the behavior on windows OS. Surface laptops
tend to have interesting firmware that expose these differences.

Erik
> 
> Maximilian
> 
> Signed-off-by: Maximilian Luz <[e-mail]>
> ---
>   drivers/acpi/acpica/dsopcode.c |  2 +-
>   drivers/acpi/acpica/exfield.c  | 12 +++++++++---
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/dsopcode.c
> b/drivers/acpi/acpica/dsopcode.c index 78f9de260d5f..0cd858520f5b
> 100644
> --- a/drivers/acpi/acpica/dsopcode.c
> +++ b/drivers/acpi/acpica/dsopcode.c
> @@ -123,7 +123,7 @@ acpi_ds_init_buffer_field(u16 aml_opcode,
> 
>   		/* Offset is in bits, count is in bits */
> 
> -		field_flags = AML_FIELD_ACCESS_BYTE;
> +		field_flags = AML_FIELD_ACCESS_BUFFER;
>   		bit_offset = offset;
>   		bit_count = (u32) length_desc->integer.value;
> 
> diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
> index e5798f15793a..55abd9e035a0 100644
> --- a/drivers/acpi/acpica/exfield.c
> +++ b/drivers/acpi/acpica/exfield.c
> @@ -98,6 +98,7 @@ acpi_ex_read_data_from_field(struct
> acpi_walk_state *walk_state,
>   	union acpi_operand_object *buffer_desc;
>   	void *buffer;
>   	u32 buffer_length;
> +	u8 field_flags;
> 
>   	ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field,
> obj_desc);
> 
> @@ -146,11 +147,16 @@ acpi_ex_read_data_from_field(struct
> acpi_walk_state *walk_state,
>   	 * Note: Field.length is in bits.
>   	 */
>   	buffer_length =
> -	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
> >field.bit_length);
> +	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
> >common_field.bit_length);
> +	field_flags = obj_desc->common_field.field_flags;
> 
> -	if (buffer_length > acpi_gbl_integer_byte_width) {
> +	if (buffer_length > acpi_gbl_integer_byte_width ||
> +	    (field_flags & AML_FIELD_ACCESS_TYPE_MASK) ==
> +AML_FIELD_ACCESS_BUFFER) {
> 
> -		/* Field is too large for an Integer, create a Buffer
> instead */
> +		/*
> +		 * Field is either too large for an Integer, or a actually of
> type
> +		 * buffer, so create a Buffer.
> +		 */
> 
>   		buffer_desc =
> acpi_ut_create_buffer_object(buffer_length);
>   		if (!buffer_desc) {
> --
> 2.20.1
>

Maximilian Luz March 19, 2019, 4:29 p.m. UTC | #2

On 3/18/19 11:28 PM, Schmauss, Erik wrote:
> 
> 
>> -----Original Message-----
>> From: [e-mail] [mailto:linux-acpi-
>> [e-mail]] On Behalf Of Schmauss, Erik
>> Sent: Thursday, March 14, 2019 10:19 AM
>> To: Maximilian Luz <[e-mail]>; Moore, Robert
>> <[e-mail]>; Wysocki, Rafael J
>> <[e-mail]>
>> Cc: [e-mail]; [e-mail]
>> Subject: RE: PROBLEM: Calling ObjectType on buffer field reports type
>> integer
>>
>>
>>
>>> -----Original Message-----
>>> From: Maximilian Luz [mailto:[e-mail]]
>>> Sent: Thursday, March 14, 2019 7:24 AM
>>> To: Moore, Robert <[e-mail]>; Schmauss, Erik
>>> <[e-mail]>; Wysocki, Rafael J
>>> <[e-mail]>
>>> Cc: [e-mail]; [e-mail]
>>> Subject: PROBLEM: Calling ObjectType on buffer field reports type
>>> integer
>>>
>>> Hi all,
>>>
>>> it seems that buffer fields (created via CreateField) are incorrectly
>>> reported as being of type integer (via ObjectType) when they are
>> small
>>> enough to allow for that.
>>>
>>> As far as I know all kernel versions are affected by this, I have
>>> specifically checked 5.0 (5.0.2), 4.20, 4.19 and 4.18.
>>>
>>> In more detail: On the Microsoft Surface Book 2, Surface Pro (2017),
>>> Surface Pro 6, Surface Laptop and Surface Laptop 2 devices there is
>>> the following piece of AML code:
>>>
>>>       Local0 = ^^_SAN.RQST (0x02, One, One, Zero, One)
>>>       If ((ObjectType (Local0) != 0x03))
>>>       {
>>>           // Error path
>>>       }
>>>       Else
>>>       {
>>>           // Success path
>>>       }
>>>
>>> Which executes a command (RQST) and then checks the type of the
>>> returned field to determine if that command was successful (i.e.
>>> returned a buffer object) or failed (i.e. returned any other type,
>>> including integer). The buffer field that is being returned by RQST is
>>> crated as
>>> follows:
>>>
>>>       CreateField (RQBF, 0x20, Local3, ARB)
>>>       Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
>>>       ...
>>>       Return (Local4)
>>>
>>> If the returned buffer field is small enough (smaller than
>>> acpi_gbl_integer_byte_width), the error-path will always be
>> executed.
>>>
>>> The full DSDT for the Surface Book 2 can be found here:
>>> https://github.com/qzed/surfacebook2-
>>>
>> dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L153
>>> 96
>>>
>>> I have attached a patch (for 5.0) that fixes this issue and has been
>>> in use on the affected devices for a few months now via
>>>
>>>       https://github.com/qzed/linux-surfacegen5-acpi and
>>>       https://github.com/jakeday/linux-surface/
>>>
>>> I'm not aware of any issues regarding the patch, but then again this
>>> has, to my knowledge, only been tested on Microsoft Surface
>> devices.
>>
>> I'll take a look at this and test the behavior on windows OS. Surface
>> laptops tend to have interesting firmware that expose these
>> differences.
> 
> I decided to run the following code on windows
> 
>      Name (BUF1, Buffer() {0x32, 0x56, 0x12, 0xff, 0x12, 0xd1, 0xd0, 0xd6, 0x54})
>      Name (BUF2, Buffer(0x09) {})
>      Method (DS00)
>      {
>          CreateField (BUF1, 1, 1, FLD0)
>          local0 = FLD0
>          return (ObjectType(Local0))
>      }
>      Method (DS01)
>      {
>          CreateField (BUF1, 0, 72, FLD1)
>          local1 = FLD1
>          return (ObjectType(Local1))
>      }
>      Method (DS02)
>      {
>          CreateField (BUF2, 0, 72, FLD2)
>          local2 = FLD2
>          return (ObjectType(Local2))
>      }
> 
> Here's an output from windbg
> 
> AMLI(? for help)-> run \ds00
> run \ds00
> 
> \DS00 completed successfully with object data:
> Integer(:Value=0x3[3])
> 
> AMLI(? for help)-> run \ds01
> run \ds01
> 
> \DS01 completed successfully with object data:
> Integer(:Value=0x3[3])
> 
> AMLI(? for help)-> run \ds02
> run \ds02
> 
> \DS02 completed successfully with object data:
> Integer(:Value=0x3[3])
> 
> AMLI(? for help)->
> 
> So it does seem like windows AML interpreter is doing what you expect it to do.
> After I applied your patch with a small modification below and it causes some regressions in our
> AML test suite. I would like to take time to look at each of these to make sure that all of the behavioral
> Differences are expected. Bob will be back in the office so I'll discuss this with him as well.

Thank you for the update!

I mainly choose this solution because it was the first one I came up
with, I'm not that experienced with acpica so yours may very well be
better. What I found a bit odd though and why I stuck with this
solution was that AML_FIELD_ACCESS_BUFFER did not seem to be used
anywhere (in contrast to the other field access types).

I've tried your modification. However, just replacing the check with

     obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD

did seem to break things for me, but I may be missing something.

More specifics on what is being broken: The communication via the
OperationRegion does not seem to work properly any more. There is a
status flag in the communication buffer that should get cleared and it
looks like it isn't. My current theory is:

The flag being checked is a byte field in the communication buffer,
created via CreateByteField (RQBF, 0x00, VSTS). Thus it should have type
ACPI_TYPE_BUFFER_FIELD (?). Due to this, your change seems to create a
buffer instead of an integer object in acpi_ex_read_data_from_field.
When this field is now evaluated for the communication check via

     If ((VSTS == Zero)) { /* success */ } Else { /* failure */ }

the check in acpi_ex_do_logical_op exmisc.c converts the second argument
to a buffer of size acpi_gbl_integer_byte_width. The buffer sizes are
then different as VSTS has size 1 and thus the check fails, getting
misinterpreted as communication-failure.

Maximilian

> 
> Thanks,
> Erik
>>
>> Erik
>>>
>>> Maximilian
>>>
>>> Signed-off-by: Maximilian Luz <[e-mail]>
>>> ---
>>>    drivers/acpi/acpica/dsopcode.c |  2 +-
>>>    drivers/acpi/acpica/exfield.c  | 12 +++++++++---
>>>    2 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/acpi/acpica/dsopcode.c
>>> b/drivers/acpi/acpica/dsopcode.c index 78f9de260d5f..0cd858520f5b
>>> 100644
>>> --- a/drivers/acpi/acpica/dsopcode.c
>>> +++ b/drivers/acpi/acpica/dsopcode.c
>>> @@ -123,7 +123,7 @@ acpi_ds_init_buffer_field(u16 aml_opcode,
>>>
>>>    		/* Offset is in bits, count is in bits */
>>>
>>> -		field_flags = AML_FIELD_ACCESS_BYTE;
>>> +		field_flags = AML_FIELD_ACCESS_BUFFER;
>>>    		bit_offset = offset;
>>>    		bit_count = (u32) length_desc->integer.value;
>>>
>>> diff --git a/drivers/acpi/acpica/exfield.c
>>> b/drivers/acpi/acpica/exfield.c index e5798f15793a..55abd9e035a0
>>> 100644
>>> --- a/drivers/acpi/acpica/exfield.c
>>> +++ b/drivers/acpi/acpica/exfield.c
>>> @@ -98,6 +98,7 @@ acpi_ex_read_data_from_field(struct
>>> acpi_walk_state *walk_state,
>>>    	union acpi_operand_object *buffer_desc;
>>>    	void *buffer;
>>>    	u32 buffer_length;
>>> +	u8 field_flags;
>>>
>>>    	ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field,
>>> obj_desc);
>>>
>>> @@ -146,11 +147,16 @@ acpi_ex_read_data_from_field(struct
>>> acpi_walk_state *walk_state,
>>>    	 * Note: Field.length is in bits.
>>>    	 */
>>>    	buffer_length =
>>> -	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
>>>> field.bit_length);
>>> +	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
>>>> common_field.bit_length);
>>> +	field_flags = obj_desc->common_field.field_flags;
>>>
>>> -	if (buffer_length > acpi_gbl_integer_byte_width) {
>>> +	if (buffer_length > acpi_gbl_integer_byte_width ||
>>> +	    (field_flags & AML_FIELD_ACCESS_TYPE_MASK) ==
>>> +AML_FIELD_ACCESS_BUFFER) {
> 
> Rather than using field_flags at all, we can do something like
> 
> if ((BufferLength > AcpiGbl_IntegerByteWidth) || (ObjDesc->Common.Type == ACPI_TYPE_BUFFER_FIELD))
> 
> This will restrict translations to the CreateField but your solution might also be valid...
> I'll run a few more test cases tomorrow.
>>>
>>> -		/* Field is too large for an Integer, create a Buffer
>>> instead */
>>> +		/*
>>> +		 * Field is either too large for an Integer, or a actually of
>>> type
>>> +		 * buffer, so create a Buffer.
>>> +		 */
>>>
>>>    		buffer_desc =
>>> acpi_ut_create_buffer_object(buffer_length);
>>>    		if (!buffer_desc) {
>>> --
>>> 2.20.1
>>>
>

Schmauss, Erik March 19, 2019, 8:19 p.m. UTC | #3

> -----Original Message-----
> From: Maximilian Luz [mailto:[e-mail]]
> Sent: Tuesday, March 19, 2019 9:30 AM
> To: Schmauss, Erik <[e-mail]>; Moore, Robert
> <[e-mail]>; Wysocki, Rafael J
> <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type
> integer
> 
> 
> 
> On 3/18/19 11:28 PM, Schmauss, Erik wrote:
> >
> >
> >> -----Original Message-----
> >> From: [e-mail] [mailto:linux-acpi-
> >> [e-mail]] On Behalf Of Schmauss, Erik
> >> Sent: Thursday, March 14, 2019 10:19 AM
> >> To: Maximilian Luz <[e-mail]>; Moore, Robert
> >> <[e-mail]>; Wysocki, Rafael J
> >> <[e-mail]>
> >> Cc: [e-mail]; [e-mail]
> >> Subject: RE: PROBLEM: Calling ObjectType on buffer field reports
> type
> >> integer
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Maximilian Luz [mailto:[e-mail]]
> >>> Sent: Thursday, March 14, 2019 7:24 AM
> >>> To: Moore, Robert <[e-mail]>; Schmauss, Erik
> >>> <[e-mail]>; Wysocki, Rafael J
> >>> <[e-mail]>
> >>> Cc: [e-mail]; [e-mail]
> >>> Subject: PROBLEM: Calling ObjectType on buffer field reports type
> >>> integer
> >>>
> >>> Hi all,
> >>>
> >>> it seems that buffer fields (created via CreateField) are
> >>> incorrectly reported as being of type integer (via ObjectType)
> when
> >>> they are
> >> small
> >>> enough to allow for that.
> >>>
> >>> As far as I know all kernel versions are affected by this, I have
> >>> specifically checked 5.0 (5.0.2), 4.20, 4.19 and 4.18.
> >>>
> >>> In more detail: On the Microsoft Surface Book 2, Surface Pro
> (2017),
> >>> Surface Pro 6, Surface Laptop and Surface Laptop 2 devices there
> is
> >>> the following piece of AML code:
> >>>
> >>>       Local0 = ^^_SAN.RQST (0x02, One, One, Zero, One)
> >>>       If ((ObjectType (Local0) != 0x03))
> >>>       {
> >>>           // Error path
> >>>       }
> >>>       Else
> >>>       {
> >>>           // Success path
> >>>       }
> >>>
> >>> Which executes a command (RQST) and then checks the type of
> the
> >>> returned field to determine if that command was successful (i.e.
> >>> returned a buffer object) or failed (i.e. returned any other type,
> >>> including integer). The buffer field that is being returned by RQST
> >>> is crated as
> >>> follows:
> >>>
> >>>       CreateField (RQBF, 0x20, Local3, ARB)
> >>>       Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
> >>>       ...
> >>>       Return (Local4)
> >>>
> >>> If the returned buffer field is small enough (smaller than
> >>> acpi_gbl_integer_byte_width), the error-path will always be
> >> executed.
> >>>
> >>> The full DSDT for the Surface Book 2 can be found here:
> >>> https://github.com/qzed/surfacebook2-
> >>>
> >>
> dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L153
> >>> 96
> >>>
> >>> I have attached a patch (for 5.0) that fixes this issue and has been
> >>> in use on the affected devices for a few months now via
> >>>
> >>>       https://github.com/qzed/linux-surfacegen5-acpi and
> >>>       https://github.com/jakeday/linux-surface/
> >>>
> >>> I'm not aware of any issues regarding the patch, but then again
> this
> >>> has, to my knowledge, only been tested on Microsoft Surface
> >> devices.
> >>
> >> I'll take a look at this and test the behavior on windows OS. Surface
> >> laptops tend to have interesting firmware that expose these
> >> differences.
> >
> > I decided to run the following code on windows
> >
> >      Name (BUF1, Buffer() {0x32, 0x56, 0x12, 0xff, 0x12, 0xd1, 0xd0,
> 0xd6, 0x54})
> >      Name (BUF2, Buffer(0x09) {})
> >      Method (DS00)
> >      {
> >          CreateField (BUF1, 1, 1, FLD0)
> >          local0 = FLD0
> >          return (ObjectType(Local0))
> >      }
> >      Method (DS01)
> >      {
> >          CreateField (BUF1, 0, 72, FLD1)
> >          local1 = FLD1
> >          return (ObjectType(Local1))
> >      }
> >      Method (DS02)
> >      {
> >          CreateField (BUF2, 0, 72, FLD2)
> >          local2 = FLD2
> >          return (ObjectType(Local2))
> >      }
> >
> > Here's an output from windbg
> >
> > AMLI(? for help)-> run \ds00
> > run \ds00
> >
> > \DS00 completed successfully with object data:
> > Integer(:Value=0x3[3])
> >
> > AMLI(? for help)-> run \ds01
> > run \ds01
> >
> > \DS01 completed successfully with object data:
> > Integer(:Value=0x3[3])
> >
> > AMLI(? for help)-> run \ds02
> > run \ds02
> >
> > \DS02 completed successfully with object data:
> > Integer(:Value=0x3[3])
> >
> > AMLI(? for help)->
> >
> > So it does seem like windows AML interpreter is doing what you
> expect it to do.
> > After I applied your patch with a small modification below and it
> > causes some regressions in our AML test suite. I would like to take
> > time to look at each of these to make sure that all of the behavioral
> Differences are expected. Bob will be back in the office so I'll discuss
> this with him as well.
> 
> Thank you for the update!
> 
> I mainly choose this solution because it was the first one I came up
> with, I'm not that experienced with acpica so yours may very well be
> better. What I found a bit odd though and why I stuck with this
> solution was that AML_FIELD_ACCESS_BUFFER did not seem to be
> used anywhere (in contrast to the other field access types).
> 
> I've tried your modification. However, just replacing the check with
> 
>      obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD
> 
> did seem to break things for me, but I may be missing something.
> 
> More specifics on what is being broken: The communication via the
> OperationRegion does not seem to work properly any more. There is a

Yeah, that's what I was thinking... After discussions with Bob, this whole
Behavior is an issue with Winodws AML interpreter not following the spec.

Page 927 of ACPI 6.3 specification states the following:

"If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it will be treated as an Integer. Otherwise, it will be treated as a Buffer."

So windows is not following this rule here. This rule is also the same for
field units so our next step is to check windows behavior for this. It would
be nice to file a bug against windows but they've never responded to these
reports in the past....

For the record, windows does detect the object type of CreateField correctly.

Method (SS02)
{
        CreateField (BUF2, 0, 72, FLD2)
        return (ObjectType(FLD2))
}

This method returns 0xE as expected.

> status flag in the communication buffer that should get cleared and it
> looks like it isn't. My current theory is:
> 
> The flag being checked is a byte field in the communication buffer,
> created via CreateByteField (RQBF, 0x00, VSTS). Thus it should have
> type ACPI_TYPE_BUFFER_FIELD (?). Due to this, your change seems to
> create a buffer instead of an integer object in
> acpi_ex_read_data_from_field.
> When this field is now evaluated for the communication check via
> 
>      If ((VSTS == Zero)) { /* success */ } Else { /* failure */ }
> 
> the check in acpi_ex_do_logical_op exmisc.c converts the second
> argument to a buffer of size acpi_gbl_integer_byte_width. The buffer
> sizes are then different as VSTS has size 1 and thus the check fails,
> getting misinterpreted as communication-failure.
> 
> Maximilian
> 
> >
> > Thanks,
> > Erik
> >>
> >> Erik
> >>>
> >>> Maximilian
> >>>
> >>> Signed-off-by: Maximilian Luz <[e-mail]>
> >>> ---
> >>>    drivers/acpi/acpica/dsopcode.c |  2 +-
> >>>    drivers/acpi/acpica/exfield.c  | 12 +++++++++---
> >>>    2 files changed, 10 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/acpica/dsopcode.c
> >>> b/drivers/acpi/acpica/dsopcode.c index
> 78f9de260d5f..0cd858520f5b
> >>> 100644
> >>> --- a/drivers/acpi/acpica/dsopcode.c
> >>> +++ b/drivers/acpi/acpica/dsopcode.c
> >>> @@ -123,7 +123,7 @@ acpi_ds_init_buffer_field(u16
> aml_opcode,
> >>>
> >>>    		/* Offset is in bits, count is in bits */
> >>>
> >>> -		field_flags = AML_FIELD_ACCESS_BYTE;
> >>> +		field_flags = AML_FIELD_ACCESS_BUFFER;
> >>>    		bit_offset = offset;
> >>>    		bit_count = (u32) length_desc->integer.value;
> >>>
> >>> diff --git a/drivers/acpi/acpica/exfield.c
> >>> b/drivers/acpi/acpica/exfield.c index e5798f15793a..55abd9e035a0
> >>> 100644
> >>> --- a/drivers/acpi/acpica/exfield.c
> >>> +++ b/drivers/acpi/acpica/exfield.c
> >>> @@ -98,6 +98,7 @@ acpi_ex_read_data_from_field(struct
> >>> acpi_walk_state *walk_state,
> >>>    	union acpi_operand_object *buffer_desc;
> >>>    	void *buffer;
> >>>    	u32 buffer_length;
> >>> +	u8 field_flags;
> >>>
> >>>    	ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field,
> >>> obj_desc);
> >>>
> >>> @@ -146,11 +147,16 @@ acpi_ex_read_data_from_field(struct
> >>> acpi_walk_state *walk_state,
> >>>    	 * Note: Field.length is in bits.
> >>>    	 */
> >>>    	buffer_length =
> >>> -	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
> >>>> field.bit_length);
> >>> +	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
> >>>> common_field.bit_length);
> >>> +	field_flags = obj_desc->common_field.field_flags;
> >>>
> >>> -	if (buffer_length > acpi_gbl_integer_byte_width) {
> >>> +	if (buffer_length > acpi_gbl_integer_byte_width ||
> >>> +	    (field_flags & AML_FIELD_ACCESS_TYPE_MASK) ==
> >>> +AML_FIELD_ACCESS_BUFFER) {
> >
> > Rather than using field_flags at all, we can do something like
> >
> > if ((BufferLength > AcpiGbl_IntegerByteWidth) || (ObjDesc-
> >Common.Type
> > == ACPI_TYPE_BUFFER_FIELD))
> >
> > This will restrict translations to the CreateField but your solution
> might also be valid...
> > I'll run a few more test cases tomorrow.
> >>>
> >>> -		/* Field is too large for an Integer, create a Buffer
> >>> instead */
> >>> +		/*
> >>> +		 * Field is either too large for an Integer, or a actually of
> >>> type
> >>> +		 * buffer, so create a Buffer.
> >>> +		 */
> >>>
> >>>    		buffer_desc =
> >>> acpi_ut_create_buffer_object(buffer_length);
> >>>    		if (!buffer_desc) {
> >>> --
> >>> 2.20.1
> >>>
> >

Maximilian Luz March 22, 2019, 8:54 p.m. UTC | #4

On 3/19/19 9:19 PM, Schmauss, Erik wrote:
> 
> 
>> -----Original Message-----
>> From: Maximilian Luz [mailto:[e-mail]]
>> Sent: Tuesday, March 19, 2019 9:30 AM
>> To: Schmauss, Erik <[e-mail]>; Moore, Robert
>> <[e-mail]>; Wysocki, Rafael J
>> <[e-mail]>
>> Cc: [e-mail]; [e-mail]
>> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type
>> integer
>>
>>
>>
>> On 3/18/19 11:28 PM, Schmauss, Erik wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: [e-mail] [mailto:linux-acpi-
>>>> [e-mail]] On Behalf Of Schmauss, Erik
>>>> Sent: Thursday, March 14, 2019 10:19 AM
>>>> To: Maximilian Luz <[e-mail]>; Moore, Robert
>>>> <[e-mail]>; Wysocki, Rafael J
>>>> <[e-mail]>
>>>> Cc: [e-mail]; [e-mail]
>>>> Subject: RE: PROBLEM: Calling ObjectType on buffer field reports
>> type
>>>> integer
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Maximilian Luz [mailto:[e-mail]]
>>>>> Sent: Thursday, March 14, 2019 7:24 AM
>>>>> To: Moore, Robert <[e-mail]>; Schmauss, Erik
>>>>> <[e-mail]>; Wysocki, Rafael J
>>>>> <[e-mail]>
>>>>> Cc: [e-mail]; [e-mail]
>>>>> Subject: PROBLEM: Calling ObjectType on buffer field reports type
>>>>> integer
>>>>>
>>>>> Hi all,
>>>>>
>>>>> it seems that buffer fields (created via CreateField) are
>>>>> incorrectly reported as being of type integer (via ObjectType)
>> when
>>>>> they are
>>>> small
>>>>> enough to allow for that.
>>>>>
>>>>> As far as I know all kernel versions are affected by this, I have
>>>>> specifically checked 5.0 (5.0.2), 4.20, 4.19 and 4.18.
>>>>>
>>>>> In more detail: On the Microsoft Surface Book 2, Surface Pro
>> (2017),
>>>>> Surface Pro 6, Surface Laptop and Surface Laptop 2 devices there
>> is
>>>>> the following piece of AML code:
>>>>>
>>>>>        Local0 = ^^_SAN.RQST (0x02, One, One, Zero, One)
>>>>>        If ((ObjectType (Local0) != 0x03))
>>>>>        {
>>>>>            // Error path
>>>>>        }
>>>>>        Else
>>>>>        {
>>>>>            // Success path
>>>>>        }
>>>>>
>>>>> Which executes a command (RQST) and then checks the type of
>> the
>>>>> returned field to determine if that command was successful (i.e.
>>>>> returned a buffer object) or failed (i.e. returned any other type,
>>>>> including integer). The buffer field that is being returned by RQST
>>>>> is crated as
>>>>> follows:
>>>>>
>>>>>        CreateField (RQBF, 0x20, Local3, ARB)
>>>>>        Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
>>>>>        ...
>>>>>        Return (Local4)
>>>>>
>>>>> If the returned buffer field is small enough (smaller than
>>>>> acpi_gbl_integer_byte_width), the error-path will always be
>>>> executed.
>>>>>
>>>>> The full DSDT for the Surface Book 2 can be found here:
>>>>> https://github.com/qzed/surfacebook2-
>>>>>
>>>>
>> dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L153
>>>>> 96
>>>>>
>>>>> I have attached a patch (for 5.0) that fixes this issue and has been
>>>>> in use on the affected devices for a few months now via
>>>>>
>>>>>        https://github.com/qzed/linux-surfacegen5-acpi and
>>>>>        https://github.com/jakeday/linux-surface/
>>>>>
>>>>> I'm not aware of any issues regarding the patch, but then again
>> this
>>>>> has, to my knowledge, only been tested on Microsoft Surface
>>>> devices.
>>>>
>>>> I'll take a look at this and test the behavior on windows OS. Surface
>>>> laptops tend to have interesting firmware that expose these
>>>> differences.
>>>
>>> I decided to run the following code on windows
>>>
>>>       Name (BUF1, Buffer() {0x32, 0x56, 0x12, 0xff, 0x12, 0xd1, 0xd0,
>> 0xd6, 0x54})
>>>       Name (BUF2, Buffer(0x09) {})
>>>       Method (DS00)
>>>       {
>>>           CreateField (BUF1, 1, 1, FLD0)
>>>           local0 = FLD0
>>>           return (ObjectType(Local0))
>>>       }
>>>       Method (DS01)
>>>       {
>>>           CreateField (BUF1, 0, 72, FLD1)
>>>           local1 = FLD1
>>>           return (ObjectType(Local1))
>>>       }
>>>       Method (DS02)
>>>       {
>>>           CreateField (BUF2, 0, 72, FLD2)
>>>           local2 = FLD2
>>>           return (ObjectType(Local2))
>>>       }
>>>
>>> Here's an output from windbg
>>>
>>> AMLI(? for help)-> run \ds00
>>> run \ds00
>>>
>>> \DS00 completed successfully with object data:
>>> Integer(:Value=0x3[3])
>>>
>>> AMLI(? for help)-> run \ds01
>>> run \ds01
>>>
>>> \DS01 completed successfully with object data:
>>> Integer(:Value=0x3[3])
>>>
>>> AMLI(? for help)-> run \ds02
>>> run \ds02
>>>
>>> \DS02 completed successfully with object data:
>>> Integer(:Value=0x3[3])
>>>
>>> AMLI(? for help)->
>>>
>>> So it does seem like windows AML interpreter is doing what you
>> expect it to do.
>>> After I applied your patch with a small modification below and it
>>> causes some regressions in our AML test suite. I would like to take
>>> time to look at each of these to make sure that all of the behavioral
>> Differences are expected. Bob will be back in the office so I'll discuss
>> this with him as well.
>>
>> Thank you for the update!
>>
>> I mainly choose this solution because it was the first one I came up
>> with, I'm not that experienced with acpica so yours may very well be
>> better. What I found a bit odd though and why I stuck with this
>> solution was that AML_FIELD_ACCESS_BUFFER did not seem to be
>> used anywhere (in contrast to the other field access types).
>>
>> I've tried your modification. However, just replacing the check with
>>
>>       obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD
>>
>> did seem to break things for me, but I may be missing something.
>>
>> More specifics on what is being broken: The communication via the
>> OperationRegion does not seem to work properly any more. There is a
> 
> Yeah, that's what I was thinking... After discussions with Bob, this whole
> Behavior is an issue with Winodws AML interpreter not following the spec.
> 
> Page 927 of ACPI 6.3 specification states the following:
> 
> "If the Buffer Field is smaller than or equal to the size of an Integer (in bits), it will be treated as an Integer. Otherwise, it will be treated as a Buffer."
> 
> So windows is not following this rule here. This rule is also the same for
> field units so our next step is to check windows behavior for this. It would
> be nice to file a bug against windows but they've never responded to these
> reports in the past....
> 
> For the record, windows does detect the object type of CreateField correctly.
> 
> Method (SS02)
> {
>          CreateField (BUF2, 0, 72, FLD2)
>          return (ObjectType(FLD2))
> }
> 
> This method returns 0xE as expected.

Thanks for the reference. Indeed looks like Microsoft is doing their own
thing here, I should have figured as much. How are things like this
being handled? Adopt Microsoft's behavior?

Maximilian

> 
>> status flag in the communication buffer that should get cleared and it
>> looks like it isn't. My current theory is:
>>
>> The flag being checked is a byte field in the communication buffer,
>> created via CreateByteField (RQBF, 0x00, VSTS). Thus it should have
>> type ACPI_TYPE_BUFFER_FIELD (?). Due to this, your change seems to
>> create a buffer instead of an integer object in
>> acpi_ex_read_data_from_field.
>> When this field is now evaluated for the communication check via
>>
>>       If ((VSTS == Zero)) { /* success */ } Else { /* failure */ }
>>
>> the check in acpi_ex_do_logical_op exmisc.c converts the second
>> argument to a buffer of size acpi_gbl_integer_byte_width. The buffer
>> sizes are then different as VSTS has size 1 and thus the check fails,
>> getting misinterpreted as communication-failure.
>>
>> Maximilian
>>
>>>
>>> Thanks,
>>> Erik
>>>>
>>>> Erik
>>>>>
>>>>> Maximilian
>>>>>
>>>>> Signed-off-by: Maximilian Luz <[e-mail]>
>>>>> ---
>>>>>     drivers/acpi/acpica/dsopcode.c |  2 +-
>>>>>     drivers/acpi/acpica/exfield.c  | 12 +++++++++---
>>>>>     2 files changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/acpica/dsopcode.c
>>>>> b/drivers/acpi/acpica/dsopcode.c index
>> 78f9de260d5f..0cd858520f5b
>>>>> 100644
>>>>> --- a/drivers/acpi/acpica/dsopcode.c
>>>>> +++ b/drivers/acpi/acpica/dsopcode.c
>>>>> @@ -123,7 +123,7 @@ acpi_ds_init_buffer_field(u16
>> aml_opcode,
>>>>>
>>>>>     		/* Offset is in bits, count is in bits */
>>>>>
>>>>> -		field_flags = AML_FIELD_ACCESS_BYTE;
>>>>> +		field_flags = AML_FIELD_ACCESS_BUFFER;
>>>>>     		bit_offset = offset;
>>>>>     		bit_count = (u32) length_desc->integer.value;
>>>>>
>>>>> diff --git a/drivers/acpi/acpica/exfield.c
>>>>> b/drivers/acpi/acpica/exfield.c index e5798f15793a..55abd9e035a0
>>>>> 100644
>>>>> --- a/drivers/acpi/acpica/exfield.c
>>>>> +++ b/drivers/acpi/acpica/exfield.c
>>>>> @@ -98,6 +98,7 @@ acpi_ex_read_data_from_field(struct
>>>>> acpi_walk_state *walk_state,
>>>>>     	union acpi_operand_object *buffer_desc;
>>>>>     	void *buffer;
>>>>>     	u32 buffer_length;
>>>>> +	u8 field_flags;
>>>>>
>>>>>     	ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field,
>>>>> obj_desc);
>>>>>
>>>>> @@ -146,11 +147,16 @@ acpi_ex_read_data_from_field(struct
>>>>> acpi_walk_state *walk_state,
>>>>>     	 * Note: Field.length is in bits.
>>>>>     	 */
>>>>>     	buffer_length =
>>>>> -	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
>>>>>> field.bit_length);
>>>>> +	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
>>>>>> common_field.bit_length);
>>>>> +	field_flags = obj_desc->common_field.field_flags;
>>>>>
>>>>> -	if (buffer_length > acpi_gbl_integer_byte_width) {
>>>>> +	if (buffer_length > acpi_gbl_integer_byte_width ||
>>>>> +	    (field_flags & AML_FIELD_ACCESS_TYPE_MASK) ==
>>>>> +AML_FIELD_ACCESS_BUFFER) {
>>>
>>> Rather than using field_flags at all, we can do something like
>>>
>>> if ((BufferLength > AcpiGbl_IntegerByteWidth) || (ObjDesc-
>>> Common.Type
>>> == ACPI_TYPE_BUFFER_FIELD))
>>>
>>> This will restrict translations to the CreateField but your solution
>> might also be valid...
>>> I'll run a few more test cases tomorrow.
>>>>>
>>>>> -		/* Field is too large for an Integer, create a Buffer
>>>>> instead */
>>>>> +		/*
>>>>> +		 * Field is either too large for an Integer, or a actually of
>>>>> type
>>>>> +		 * buffer, so create a Buffer.
>>>>> +		 */
>>>>>
>>>>>     		buffer_desc =
>>>>> acpi_ut_create_buffer_object(buffer_length);
>>>>>     		if (!buffer_desc) {
>>>>> --
>>>>> 2.20.1
>>>>>
>>>

Schmauss, Erik March 22, 2019, 9:28 p.m. UTC | #5

> -----Original Message-----
> From: Maximilian Luz [mailto:[e-mail]]
> Sent: Friday, March 22, 2019 1:55 PM
> To: Schmauss, Erik <[e-mail]>; Moore, Robert
> <[e-mail]>; Wysocki, Rafael J
> <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type
> integer
> 
> 
> 
> On 3/19/19 9:19 PM, Schmauss, Erik wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maximilian Luz [mailto:[e-mail]]
> >> Sent: Tuesday, March 19, 2019 9:30 AM
> >> To: Schmauss, Erik <[e-mail]>; Moore, Robert
> >> <[e-mail]>; Wysocki, Rafael J
> >> <[e-mail]>
> >> Cc: [e-mail]; [e-mail]
> >> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports
> type
> >> integer
> >>
> >>
> >>
> >> On 3/18/19 11:28 PM, Schmauss, Erik wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: [e-mail] [mailto:linux-acpi-
> >>>> [e-mail]] On Behalf Of Schmauss, Erik
> >>>> Sent: Thursday, March 14, 2019 10:19 AM
> >>>> To: Maximilian Luz <[e-mail]>; Moore, Robert
> >>>> <[e-mail]>; Wysocki, Rafael J
> >>>> <[e-mail]>
> >>>> Cc: [e-mail]; [e-mail]
> >>>> Subject: RE: PROBLEM: Calling ObjectType on buffer field reports
> >> type
> >>>> integer
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Maximilian Luz [mailto:[e-mail]]
> >>>>> Sent: Thursday, March 14, 2019 7:24 AM
> >>>>> To: Moore, Robert <[e-mail]>; Schmauss, Erik
> >>>>> <[e-mail]>; Wysocki, Rafael J
> >>>>> <[e-mail]>
> >>>>> Cc: [e-mail]; [e-mail]
> >>>>> Subject: PROBLEM: Calling ObjectType on buffer field reports
> type
> >>>>> integer
> >>>>>
> >>>>> Hi all,
> >>>>>
> >>>>> it seems that buffer fields (created via CreateField) are
> >>>>> incorrectly reported as being of type integer (via ObjectType)
> >> when
> >>>>> they are
> >>>> small
> >>>>> enough to allow for that.
> >>>>>
> >>>>> As far as I know all kernel versions are affected by this, I have
> >>>>> specifically checked 5.0 (5.0.2), 4.20, 4.19 and 4.18.
> >>>>>
> >>>>> In more detail: On the Microsoft Surface Book 2, Surface Pro
> >> (2017),
> >>>>> Surface Pro 6, Surface Laptop and Surface Laptop 2 devices
> there
> >> is
> >>>>> the following piece of AML code:
> >>>>>
> >>>>>        Local0 = ^^_SAN.RQST (0x02, One, One, Zero, One)
> >>>>>        If ((ObjectType (Local0) != 0x03))
> >>>>>        {
> >>>>>            // Error path
> >>>>>        }
> >>>>>        Else
> >>>>>        {
> >>>>>            // Success path
> >>>>>        }
> >>>>>
> >>>>> Which executes a command (RQST) and then checks the type of
> >> the
> >>>>> returned field to determine if that command was successful
> (i.e.
> >>>>> returned a buffer object) or failed (i.e. returned any other type,
> >>>>> including integer). The buffer field that is being returned by
> >>>>> RQST is crated as
> >>>>> follows:
> >>>>>
> >>>>>        CreateField (RQBF, 0x20, Local3, ARB)
> >>>>>        Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
> >>>>>        ...
> >>>>>        Return (Local4)
> >>>>>
> >>>>> If the returned buffer field is small enough (smaller than
> >>>>> acpi_gbl_integer_byte_width), the error-path will always be
> >>>> executed.
> >>>>>
> >>>>> The full DSDT for the Surface Book 2 can be found here:
> >>>>> https://github.com/qzed/surfacebook2-
> >>>>>
> >>>>
> >>
> dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L153
> >>>>> 96
> >>>>>
> >>>>> I have attached a patch (for 5.0) that fixes this issue and has
> >>>>> been in use on the affected devices for a few months now via
> >>>>>
> >>>>>        https://github.com/qzed/linux-surfacegen5-acpi and
> >>>>>        https://github.com/jakeday/linux-surface/
> >>>>>
> >>>>> I'm not aware of any issues regarding the patch, but then again
> >> this
> >>>>> has, to my knowledge, only been tested on Microsoft Surface
> >>>> devices.
> >>>>
> >>>> I'll take a look at this and test the behavior on windows OS.
> >>>> Surface laptops tend to have interesting firmware that expose
> these
> >>>> differences.
> >>>
> >>> I decided to run the following code on windows
> >>>
> >>>       Name (BUF1, Buffer() {0x32, 0x56, 0x12, 0xff, 0x12, 0xd1,
> >>> 0xd0,
> >> 0xd6, 0x54})
> >>>       Name (BUF2, Buffer(0x09) {})
> >>>       Method (DS00)
> >>>       {
> >>>           CreateField (BUF1, 1, 1, FLD0)
> >>>           local0 = FLD0
> >>>           return (ObjectType(Local0))
> >>>       }
> >>>       Method (DS01)
> >>>       {
> >>>           CreateField (BUF1, 0, 72, FLD1)
> >>>           local1 = FLD1
> >>>           return (ObjectType(Local1))
> >>>       }
> >>>       Method (DS02)
> >>>       {
> >>>           CreateField (BUF2, 0, 72, FLD2)
> >>>           local2 = FLD2
> >>>           return (ObjectType(Local2))
> >>>       }
> >>>
> >>> Here's an output from windbg
> >>>
> >>> AMLI(? for help)-> run \ds00
> >>> run \ds00
> >>>
> >>> \DS00 completed successfully with object data:
> >>> Integer(:Value=0x3[3])
> >>>
> >>> AMLI(? for help)-> run \ds01
> >>> run \ds01
> >>>
> >>> \DS01 completed successfully with object data:
> >>> Integer(:Value=0x3[3])
> >>>
> >>> AMLI(? for help)-> run \ds02
> >>> run \ds02
> >>>
> >>> \DS02 completed successfully with object data:
> >>> Integer(:Value=0x3[3])
> >>>
> >>> AMLI(? for help)->
> >>>
> >>> So it does seem like windows AML interpreter is doing what you
> >> expect it to do.
> >>> After I applied your patch with a small modification below and it
> >>> causes some regressions in our AML test suite. I would like to take
> >>> time to look at each of these to make sure that all of the
> >>> behavioral
> >> Differences are expected. Bob will be back in the office so I'll
> >> discuss this with him as well.
> >>
> >> Thank you for the update!
> >>
> >> I mainly choose this solution because it was the first one I came up
> >> with, I'm not that experienced with acpica so yours may very well
> be
> >> better. What I found a bit odd though and why I stuck with this
> >> solution was that AML_FIELD_ACCESS_BUFFER did not seem to be
> used
> >> anywhere (in contrast to the other field access types).
> >>
> >> I've tried your modification. However, just replacing the check with
> >>
> >>       obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD
> >>
> >> did seem to break things for me, but I may be missing something.
> >>
> >> More specifics on what is being broken: The communication via the
> >> OperationRegion does not seem to work properly any more. There
> is a
> >
> > Yeah, that's what I was thinking... After discussions with Bob, this
> > whole Behavior is an issue with Winodws AML interpreter not
> following the spec.
> >
> > Page 927 of ACPI 6.3 specification states the following:
> >
> > "If the Buffer Field is smaller than or equal to the size of an Integer
> (in bits), it will be treated as an Integer. Otherwise, it will be treated as
> a Buffer."
> >
> > So windows is not following this rule here. This rule is also the same
> > for field units so our next step is to check windows behavior for
> > this. It would be nice to file a bug against windows but they've never
> > responded to these reports in the past....
> >
> > For the record, windows does detect the object type of CreateField
> correctly.
> >
> > Method (SS02)
> > {
> >          CreateField (BUF2, 0, 72, FLD2)
> >          return (ObjectType(FLD2))
> > }
> >
> > This method returns 0xE as expected.
> 
> Thanks for the reference. Indeed looks like Microsoft is doing their
> own thing here, I should have figured as much. How are things like this
> being handled? Adopt Microsoft's behavior?

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
> 
> Maximilian
> 
> >
> >> status flag in the communication buffer that should get cleared and
> >> it looks like it isn't. My current theory is:
> >>
> >> The flag being checked is a byte field in the communication buffer,
> >> created via CreateByteField (RQBF, 0x00, VSTS). Thus it should have
> >> type ACPI_TYPE_BUFFER_FIELD (?). Due to this, your change seems
> to
> >> create a buffer instead of an integer object in
> >> acpi_ex_read_data_from_field.
> >> When this field is now evaluated for the communication check via
> >>
> >>       If ((VSTS == Zero)) { /* success */ } Else { /* failure */ }
> >>
> >> the check in acpi_ex_do_logical_op exmisc.c converts the second
> >> argument to a buffer of size acpi_gbl_integer_byte_width. The
> buffer
> >> sizes are then different as VSTS has size 1 and thus the check fails,
> >> getting misinterpreted as communication-failure.
> >>
> >> Maximilian
> >>
> >>>
> >>> Thanks,
> >>> Erik
> >>>>
> >>>> Erik
> >>>>>
> >>>>> Maximilian
> >>>>>
> >>>>> Signed-off-by: Maximilian Luz <[e-mail]>
> >>>>> ---
> >>>>>     drivers/acpi/acpica/dsopcode.c |  2 +-
> >>>>>     drivers/acpi/acpica/exfield.c  | 12 +++++++++---
> >>>>>     2 files changed, 10 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/acpi/acpica/dsopcode.c
> >>>>> b/drivers/acpi/acpica/dsopcode.c index
> >> 78f9de260d5f..0cd858520f5b
> >>>>> 100644
> >>>>> --- a/drivers/acpi/acpica/dsopcode.c
> >>>>> +++ b/drivers/acpi/acpica/dsopcode.c
> >>>>> @@ -123,7 +123,7 @@ acpi_ds_init_buffer_field(u16
> >> aml_opcode,
> >>>>>
> >>>>>     		/* Offset is in bits, count is in bits */
> >>>>>
> >>>>> -		field_flags = AML_FIELD_ACCESS_BYTE;
> >>>>> +		field_flags = AML_FIELD_ACCESS_BUFFER;
> >>>>>     		bit_offset = offset;
> >>>>>     		bit_count = (u32) length_desc->integer.value;
> >>>>>
> >>>>> diff --git a/drivers/acpi/acpica/exfield.c
> >>>>> b/drivers/acpi/acpica/exfield.c index
> e5798f15793a..55abd9e035a0
> >>>>> 100644
> >>>>> --- a/drivers/acpi/acpica/exfield.c
> >>>>> +++ b/drivers/acpi/acpica/exfield.c
> >>>>> @@ -98,6 +98,7 @@ acpi_ex_read_data_from_field(struct
> >>>>> acpi_walk_state *walk_state,
> >>>>>     	union acpi_operand_object *buffer_desc;
> >>>>>     	void *buffer;
> >>>>>     	u32 buffer_length;
> >>>>> +	u8 field_flags;
> >>>>>
> >>>>>
> 	ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field,
> >>>>> obj_desc);
> >>>>>
> >>>>> @@ -146,11 +147,16 @@ acpi_ex_read_data_from_field(struct
> >>>>> acpi_walk_state *walk_state,
> >>>>>     	 * Note: Field.length is in bits.
> >>>>>     	 */
> >>>>>     	buffer_length =
> >>>>> -
> (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
> >>>>>> field.bit_length);
> >>>>> +
> (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
> >>>>>> common_field.bit_length);
> >>>>> +	field_flags = obj_desc->common_field.field_flags;
> >>>>>
> >>>>> -	if (buffer_length > acpi_gbl_integer_byte_width) {
> >>>>> +	if (buffer_length > acpi_gbl_integer_byte_width ||
> >>>>> +	    (field_flags & AML_FIELD_ACCESS_TYPE_MASK) ==
> >>>>> +AML_FIELD_ACCESS_BUFFER) {
> >>>
> >>> Rather than using field_flags at all, we can do something like
> >>>
> >>> if ((BufferLength > AcpiGbl_IntegerByteWidth) || (ObjDesc-
> >>> Common.Type == ACPI_TYPE_BUFFER_FIELD))
> >>>
> >>> This will restrict translations to the CreateField but your solution
> >> might also be valid...
> >>> I'll run a few more test cases tomorrow.
> >>>>>
> >>>>> -		/* Field is too large for an Integer, create a
> Buffer
> >>>>> instead */
> >>>>> +		/*
> >>>>> +		 * Field is either too large for an Integer, or a
> actually of
> >>>>> type
> >>>>> +		 * buffer, so create a Buffer.
> >>>>> +		 */
> >>>>>
> >>>>>     		buffer_desc =
> >>>>> acpi_ut_create_buffer_object(buffer_length);
> >>>>>     		if (!buffer_desc) {
> >>>>> --
> >>>>> 2.20.1
> >>>>>
> >>>

Maximilian Luz March 24, 2019, 6:58 p.m. UTC | #6

On 3/22/19 10:28 PM, Schmauss, Erik wrote:
> 
> 
>> -----Original Message-----
>> From: Maximilian Luz [mailto:[e-mail]]
>> Sent: Friday, March 22, 2019 1:55 PM
>> To: Schmauss, Erik <[e-mail]>; Moore, Robert
>> <[e-mail]>; Wysocki, Rafael J
>> <[e-mail]>
>> Cc: [e-mail]; [e-mail]
>> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports type
>> integer
>>
>>
>>
>> On 3/19/19 9:19 PM, Schmauss, Erik wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Maximilian Luz [mailto:[e-mail]]
>>>> Sent: Tuesday, March 19, 2019 9:30 AM
>>>> To: Schmauss, Erik <[e-mail]>; Moore, Robert
>>>> <[e-mail]>; Wysocki, Rafael J
>>>> <[e-mail]>
>>>> Cc: [e-mail]; [e-mail]
>>>> Subject: Re: PROBLEM: Calling ObjectType on buffer field reports
>> type
>>>> integer
>>>>
>>>>
>>>>
>>>> On 3/18/19 11:28 PM, Schmauss, Erik wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: [e-mail] [mailto:linux-acpi-
>>>>>> [e-mail]] On Behalf Of Schmauss, Erik
>>>>>> Sent: Thursday, March 14, 2019 10:19 AM
>>>>>> To: Maximilian Luz <[e-mail]>; Moore, Robert
>>>>>> <[e-mail]>; Wysocki, Rafael J
>>>>>> <[e-mail]>
>>>>>> Cc: [e-mail]; [e-mail]
>>>>>> Subject: RE: PROBLEM: Calling ObjectType on buffer field reports
>>>> type
>>>>>> integer
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Maximilian Luz [mailto:[e-mail]]
>>>>>>> Sent: Thursday, March 14, 2019 7:24 AM
>>>>>>> To: Moore, Robert <[e-mail]>; Schmauss, Erik
>>>>>>> <[e-mail]>; Wysocki, Rafael J
>>>>>>> <[e-mail]>
>>>>>>> Cc: [e-mail]; [e-mail]
>>>>>>> Subject: PROBLEM: Calling ObjectType on buffer field reports
>> type
>>>>>>> integer
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> it seems that buffer fields (created via CreateField) are
>>>>>>> incorrectly reported as being of type integer (via ObjectType)
>>>> when
>>>>>>> they are
>>>>>> small
>>>>>>> enough to allow for that.
>>>>>>>
>>>>>>> As far as I know all kernel versions are affected by this, I have
>>>>>>> specifically checked 5.0 (5.0.2), 4.20, 4.19 and 4.18.
>>>>>>>
>>>>>>> In more detail: On the Microsoft Surface Book 2, Surface Pro
>>>> (2017),
>>>>>>> Surface Pro 6, Surface Laptop and Surface Laptop 2 devices
>> there
>>>> is
>>>>>>> the following piece of AML code:
>>>>>>>
>>>>>>>         Local0 = ^^_SAN.RQST (0x02, One, One, Zero, One)
>>>>>>>         If ((ObjectType (Local0) != 0x03))
>>>>>>>         {
>>>>>>>             // Error path
>>>>>>>         }
>>>>>>>         Else
>>>>>>>         {
>>>>>>>             // Success path
>>>>>>>         }
>>>>>>>
>>>>>>> Which executes a command (RQST) and then checks the type of
>>>> the
>>>>>>> returned field to determine if that command was successful
>> (i.e.
>>>>>>> returned a buffer object) or failed (i.e. returned any other type,
>>>>>>> including integer). The buffer field that is being returned by
>>>>>>> RQST is crated as
>>>>>>> follows:
>>>>>>>
>>>>>>>         CreateField (RQBF, 0x20, Local3, ARB)
>>>>>>>         Local4 = ARB /* \_SB_._SAN.RQSX.ARB_ */
>>>>>>>         ...
>>>>>>>         Return (Local4)
>>>>>>>
>>>>>>> If the returned buffer field is small enough (smaller than
>>>>>>> acpi_gbl_integer_byte_width), the error-path will always be
>>>>>> executed.
>>>>>>>
>>>>>>> The full DSDT for the Surface Book 2 can be found here:
>>>>>>> https://github.com/qzed/surfacebook2-
>>>>>>>
>>>>>>
>>>>
>> dsdt/blob/fa0ca7c7cba8fb0da1e79c282f9a9f4a12eaaa17/dsdt.dsl#L153
>>>>>>> 96
>>>>>>>
>>>>>>> I have attached a patch (for 5.0) that fixes this issue and has
>>>>>>> been in use on the affected devices for a few months now via
>>>>>>>
>>>>>>>         https://github.com/qzed/linux-surfacegen5-acpi and
>>>>>>>         https://github.com/jakeday/linux-surface/
>>>>>>>
>>>>>>> I'm not aware of any issues regarding the patch, but then again
>>>> this
>>>>>>> has, to my knowledge, only been tested on Microsoft Surface
>>>>>> devices.
>>>>>>
>>>>>> I'll take a look at this and test the behavior on windows OS.
>>>>>> Surface laptops tend to have interesting firmware that expose
>> these
>>>>>> differences.
>>>>>
>>>>> I decided to run the following code on windows
>>>>>
>>>>>        Name (BUF1, Buffer() {0x32, 0x56, 0x12, 0xff, 0x12, 0xd1,
>>>>> 0xd0,
>>>> 0xd6, 0x54})
>>>>>        Name (BUF2, Buffer(0x09) {})
>>>>>        Method (DS00)
>>>>>        {
>>>>>            CreateField (BUF1, 1, 1, FLD0)
>>>>>            local0 = FLD0
>>>>>            return (ObjectType(Local0))
>>>>>        }
>>>>>        Method (DS01)
>>>>>        {
>>>>>            CreateField (BUF1, 0, 72, FLD1)
>>>>>            local1 = FLD1
>>>>>            return (ObjectType(Local1))
>>>>>        }
>>>>>        Method (DS02)
>>>>>        {
>>>>>            CreateField (BUF2, 0, 72, FLD2)
>>>>>            local2 = FLD2
>>>>>            return (ObjectType(Local2))
>>>>>        }
>>>>>
>>>>> Here's an output from windbg
>>>>>
>>>>> AMLI(? for help)-> run \ds00
>>>>> run \ds00
>>>>>
>>>>> \DS00 completed successfully with object data:
>>>>> Integer(:Value=0x3[3])
>>>>>
>>>>> AMLI(? for help)-> run \ds01
>>>>> run \ds01
>>>>>
>>>>> \DS01 completed successfully with object data:
>>>>> Integer(:Value=0x3[3])
>>>>>
>>>>> AMLI(? for help)-> run \ds02
>>>>> run \ds02
>>>>>
>>>>> \DS02 completed successfully with object data:
>>>>> Integer(:Value=0x3[3])
>>>>>
>>>>> AMLI(? for help)->
>>>>>
>>>>> So it does seem like windows AML interpreter is doing what you
>>>> expect it to do.
>>>>> After I applied your patch with a small modification below and it
>>>>> causes some regressions in our AML test suite. I would like to take
>>>>> time to look at each of these to make sure that all of the
>>>>> behavioral
>>>> Differences are expected. Bob will be back in the office so I'll
>>>> discuss this with him as well.
>>>>
>>>> Thank you for the update!
>>>>
>>>> I mainly choose this solution because it was the first one I came up
>>>> with, I'm not that experienced with acpica so yours may very well
>> be
>>>> better. What I found a bit odd though and why I stuck with this
>>>> solution was that AML_FIELD_ACCESS_BUFFER did not seem to be
>> used
>>>> anywhere (in contrast to the other field access types).
>>>>
>>>> I've tried your modification. However, just replacing the check with
>>>>
>>>>        obj_desc->common.type == ACPI_TYPE_BUFFER_FIELD
>>>>
>>>> did seem to break things for me, but I may be missing something.
>>>>
>>>> More specifics on what is being broken: The communication via the
>>>> OperationRegion does not seem to work properly any more. There
>> is a
>>>
>>> Yeah, that's what I was thinking... After discussions with Bob, this
>>> whole Behavior is an issue with Winodws AML interpreter not
>> following the spec.
>>>
>>> Page 927 of ACPI 6.3 specification states the following:
>>>
>>> "If the Buffer Field is smaller than or equal to the size of an Integer
>> (in bits), it will be treated as an Integer. Otherwise, it will be treated as
>> a Buffer."
>>>
>>> So windows is not following this rule here. This rule is also the same
>>> for field units so our next step is to check windows behavior for
>>> this. It would be nice to file a bug against windows but they've never
>>> responded to these reports in the past....
>>>
>>> For the record, windows does detect the object type of CreateField
>> correctly.
>>>
>>> Method (SS02)
>>> {
>>>           CreateField (BUF2, 0, 72, FLD2)
>>>           return (ObjectType(FLD2))
>>> }
>>>
>>> This method returns 0xE as expected.
>>
>> Thanks for the reference. Indeed looks like Microsoft is doing their
>> own thing here, I should have figured as much. How are things like this
>> being handled? Adopt Microsoft's behavior?
> 
> 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...

Alright, let's hope Microsoft acknowledges this.

Thanks,
Maximilian

> 	
> Erik
>>
>> Maximilian
>>
>>>
>>>> status flag in the communication buffer that should get cleared and
>>>> it looks like it isn't. My current theory is:
>>>>
>>>> The flag being checked is a byte field in the communication buffer,
>>>> created via CreateByteField (RQBF, 0x00, VSTS). Thus it should have
>>>> type ACPI_TYPE_BUFFER_FIELD (?). Due to this, your change seems
>> to
>>>> create a buffer instead of an integer object in
>>>> acpi_ex_read_data_from_field.
>>>> When this field is now evaluated for the communication check via
>>>>
>>>>        If ((VSTS == Zero)) { /* success */ } Else { /* failure */ }
>>>>
>>>> the check in acpi_ex_do_logical_op exmisc.c converts the second
>>>> argument to a buffer of size acpi_gbl_integer_byte_width. The
>> buffer
>>>> sizes are then different as VSTS has size 1 and thus the check fails,
>>>> getting misinterpreted as communication-failure.
>>>>
>>>> Maximilian
>>>>
>>>>>
>>>>> Thanks,
>>>>> Erik
>>>>>>
>>>>>> Erik
>>>>>>>
>>>>>>> Maximilian
>>>>>>>
>>>>>>> Signed-off-by: Maximilian Luz <[e-mail]>
>>>>>>> ---
>>>>>>>      drivers/acpi/acpica/dsopcode.c |  2 +-
>>>>>>>      drivers/acpi/acpica/exfield.c  | 12 +++++++++---
>>>>>>>      2 files changed, 10 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/acpi/acpica/dsopcode.c
>>>>>>> b/drivers/acpi/acpica/dsopcode.c index
>>>> 78f9de260d5f..0cd858520f5b
>>>>>>> 100644
>>>>>>> --- a/drivers/acpi/acpica/dsopcode.c
>>>>>>> +++ b/drivers/acpi/acpica/dsopcode.c
>>>>>>> @@ -123,7 +123,7 @@ acpi_ds_init_buffer_field(u16
>>>> aml_opcode,
>>>>>>>
>>>>>>>      		/* Offset is in bits, count is in bits */
>>>>>>>
>>>>>>> -		field_flags = AML_FIELD_ACCESS_BYTE;
>>>>>>> +		field_flags = AML_FIELD_ACCESS_BUFFER;
>>>>>>>      		bit_offset = offset;
>>>>>>>      		bit_count = (u32) length_desc->integer.value;
>>>>>>>
>>>>>>> diff --git a/drivers/acpi/acpica/exfield.c
>>>>>>> b/drivers/acpi/acpica/exfield.c index
>> e5798f15793a..55abd9e035a0
>>>>>>> 100644
>>>>>>> --- a/drivers/acpi/acpica/exfield.c
>>>>>>> +++ b/drivers/acpi/acpica/exfield.c
>>>>>>> @@ -98,6 +98,7 @@ acpi_ex_read_data_from_field(struct
>>>>>>> acpi_walk_state *walk_state,
>>>>>>>      	union acpi_operand_object *buffer_desc;
>>>>>>>      	void *buffer;
>>>>>>>      	u32 buffer_length;
>>>>>>> +	u8 field_flags;
>>>>>>>
>>>>>>>
>> 	ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field,
>>>>>>> obj_desc);
>>>>>>>
>>>>>>> @@ -146,11 +147,16 @@ acpi_ex_read_data_from_field(struct
>>>>>>> acpi_walk_state *walk_state,
>>>>>>>      	 * Note: Field.length is in bits.
>>>>>>>      	 */
>>>>>>>      	buffer_length =
>>>>>>> -
>> (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
>>>>>>>> field.bit_length);
>>>>>>> +
>> (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc-
>>>>>>>> common_field.bit_length);
>>>>>>> +	field_flags = obj_desc->common_field.field_flags;
>>>>>>>
>>>>>>> -	if (buffer_length > acpi_gbl_integer_byte_width) {
>>>>>>> +	if (buffer_length > acpi_gbl_integer_byte_width ||
>>>>>>> +	    (field_flags & AML_FIELD_ACCESS_TYPE_MASK) ==
>>>>>>> +AML_FIELD_ACCESS_BUFFER) {
>>>>>
>>>>> Rather than using field_flags at all, we can do something like
>>>>>
>>>>> if ((BufferLength > AcpiGbl_IntegerByteWidth) || (ObjDesc-
>>>>> Common.Type == ACPI_TYPE_BUFFER_FIELD))
>>>>>
>>>>> This will restrict translations to the CreateField but your solution
>>>> might also be valid...
>>>>> I'll run a few more test cases tomorrow.
>>>>>>>
>>>>>>> -		/* Field is too large for an Integer, create a
>> Buffer
>>>>>>> instead */
>>>>>>> +		/*
>>>>>>> +		 * Field is either too large for an Integer, or a
>> actually of
>>>>>>> type
>>>>>>> +		 * buffer, so create a Buffer.
>>>>>>> +		 */
>>>>>>>
>>>>>>>      		buffer_desc =
>>>>>>> acpi_ut_create_buffer_object(buffer_length);
>>>>>>>      		if (!buffer_desc) {
>>>>>>> --
>>>>>>> 2.20.1
>>>>>>>
>>>>>

Maximilian Luz July 20, 2019, 8:45 p.m. UTC | #7

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

Schmauss, Erik July 22, 2019, 11:01 p.m. UTC | #8

> -----Original Message-----
> From: Maximilian Luz [mailto:[e-mail]]
> Sent: Saturday, July 20, 2019 1:45 PM
> To: Schmauss, Erik <[e-mail]>; Moore, Robert
> <[e-mail]>; Wysocki, Rafael J <[e-mail]>
> Cc: [e-mail]; [e-mail]
> 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

Maximilian Luz July 23, 2019, 11:38 a.m. UTC | #9

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

Maximilian Luz Nov. 10, 2019, 9:29 p.m. UTC | #10

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

Moore, Robert Nov. 22, 2019, 5:11 p.m. UTC | #11

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


-----Original Message-----
From: Maximilian Luz <[e-mail]> 
Sent: Sunday, November 10, 2019 1:30 PM
To: Schmauss, Erik <[e-mail]>; Moore, Robert <[e-mail]>; Wysocki, Rafael J <[e-mail]>
Cc: [e-mail]; [e-mail]
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

Moore, Robert Nov. 22, 2019, 10:07 p.m. UTC | #12

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 <[e-mail]> 
Sent: Friday, November 22, 2019 9:12 AM
To: Maximilian Luz <[e-mail]>; Schmauss, Erik <[e-mail]>; Wysocki, Rafael J <[e-mail]>
Cc: [e-mail]; [e-mail]
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 <[e-mail]> 
Sent: Sunday, November 10, 2019 1:30 PM
To: Schmauss, Erik <[e-mail]>; Moore, Robert <[e-mail]>; Wysocki, Rafael J <[e-mail]>
Cc: [e-mail]; [e-mail]
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

Maximilian Luz Nov. 22, 2019, 11 p.m. UTC | #13

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

Schmauss, Erik Nov. 22, 2019, 11:29 p.m. UTC | #14

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 <[e-mail]>
> Sent: Friday, November 22, 2019 2:07 PM
> To: Moore, Robert <[e-mail]>; Maximilian Luz
> <[e-mail]>; Schmauss, Erik <[e-mail]>;
> Wysocki, Rafael J <[e-mail]>
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
> Sent: Friday, November 22, 2019 9:12 AM
> To: Maximilian Luz <[e-mail]>; Schmauss, Erik
> <[e-mail]>; Wysocki, Rafael J <[e-mail]>
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
> Sent: Sunday, November 10, 2019 1:30 PM
> To: Schmauss, Erik <[e-mail]>; Moore, Robert
> <[e-mail]>; Wysocki, Rafael J <[e-mail]>
> Cc: [e-mail]; [e-mail]
> 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 -- [e-mail]
> To unsubscribe send an email to [e-mail]
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

Maximilian Luz Nov. 23, 2019, 12:33 a.m. UTC | #15

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

Schmauss, Erik Nov. 27, 2019, 12:25 a.m. UTC | #16

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

Erik Kaneda Dec. 3, 2019, 10:21 p.m. UTC | #17

> -----Original Message-----
> From: [e-mail] <linux-acpi-
> [e-mail]> On Behalf Of Schmauss, Erik
> Sent: Tuesday, November 26, 2019 4:26 PM
> To: Maximilian Luz <[e-mail]>; Moore, Robert
> <[e-mail]>; Wysocki, Rafael J
> <[e-mail]>
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
Date: Tue, 3 Dec 2019 12:42:51 -0800
From: Erik Schmauss <[e-mail]>
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 <[e-mail]>
Signed-off-by: Erik Schmauss <[e-mail]>
---
 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
>

Maximilian Luz Dec. 4, 2019, 7:33 p.m. UTC | #18

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

Erik Kaneda Dec. 5, 2019, 11:57 p.m. UTC | #19

> -----Original Message-----
> From: Maximilian Luz <[e-mail]>
> Sent: Wednesday, December 4, 2019 11:34 AM
> To: Kaneda, Erik <[e-mail]>; Moore, Robert
> <[e-mail]>; Wysocki, Rafael J <[e-mail]>
> Cc: [e-mail]; [e-mail]
> 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

Maximilian Luz Dec. 6, 2019, 1:48 p.m. UTC | #20

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

Patch

diff --git a/drivers/acpi/acpica/dsopcode.c b/drivers/acpi/acpica/dsopcode.c
index 78f9de260d5f..0cd858520f5b 100644
--- a/drivers/acpi/acpica/dsopcode.c
+++ b/drivers/acpi/acpica/dsopcode.c
@@ -123,7 +123,7 @@  acpi_ds_init_buffer_field(u16 aml_opcode,

  		/* Offset is in bits, count is in bits */

-		field_flags = AML_FIELD_ACCESS_BYTE;
+		field_flags = AML_FIELD_ACCESS_BUFFER;
  		bit_offset = offset;
  		bit_count = (u32) length_desc->integer.value;

diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
index e5798f15793a..55abd9e035a0 100644
--- a/drivers/acpi/acpica/exfield.c
+++ b/drivers/acpi/acpica/exfield.c
@@ -98,6 +98,7 @@  acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
  	union acpi_operand_object *buffer_desc;
  	void *buffer;
  	u32 buffer_length;
+	u8 field_flags;

  	ACPI_FUNCTION_TRACE_PTR(ex_read_data_from_field, obj_desc);

@@ -146,11 +147,16 @@  acpi_ex_read_data_from_field(struct acpi_walk_state *walk_state,
  	 * Note: Field.length is in bits.
  	 */
  	buffer_length =
-	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc->field.bit_length);
+	    (acpi_size)ACPI_ROUND_BITS_UP_TO_BYTES(obj_desc->common_field.bit_length);
+	field_flags = obj_desc->common_field.field_flags;

-	if (buffer_length > acpi_gbl_integer_byte_width) {
+	if (buffer_length > acpi_gbl_integer_byte_width ||
+	    (field_flags & AML_FIELD_ACCESS_TYPE_MASK) == AML_FIELD_ACCESS_BUFFER) {

-		/* Field is too large for an Integer, create a Buffer instead */
+		/*
+		 * Field is either too large for an Integer, or a actually of type
+		 * buffer, so create a Buffer.
+		 */

  		buffer_desc = acpi_ut_create_buffer_object(buffer_length);
  		if (!buffer_desc) {