PROBLEM: Calling ObjectType on buffer field reports type integer

[Maximilian Luz <luzmaximilian@xxxxxxxxx>]

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 <luzmaximilian@xxxxxxxxx>
---
 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