Matt Delco has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31161
Change subject: acpi: device: avoid empty property list in acpi_dp_write ......................................................................
acpi: device: avoid empty property list in acpi_dp_write
If an acpi_dp table has children but no properties then acpi_dp_write() will write out a properties UUID and package that contains no properties. The existing function will avoid writing out a UUID and empty package when no children exist, but it seems to assume that properties will always be used. With this change properties are handled in a manner akin to children so that a UUID and package are only written if properties exist.
BUG=none BRANCH=none TEST=Confirmed that prior to this change a UUID and empty package was present for a device that had children but no properties. Verified that after this change the UUID and empty package are no longer present but the child UUID and package are still present.
Change-Id: I6f5597713a1e91ca26b409f36b3ff9eb90a010af Signed-off-by: Matt Delco delco@chromium.org --- M src/arch/x86/acpi_device.c 1 file changed, 26 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/31161/1
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 48b7fee..c62d6f3 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -632,7 +632,7 @@ void acpi_dp_write(struct acpi_dp *table) { struct acpi_dp *dp, *prop; - char *dp_count, *prop_count; + char *dp_count, *prop_count = NULL; int child_count = 0;
if (!table || table->type != ACPI_DP_TYPE_TABLE) @@ -644,32 +644,42 @@ /* Device Property list starts with the next entry */ prop = table->next;
- /* Package (DP), default to 2 elements (assuming no children) */ - dp_count = acpigen_write_package(2); - - /* ToUUID (ACPI_DP_UUID) */ - acpigen_write_uuid(ACPI_DP_UUID); - - /* Package (PROP), element count determined as it is populated */ - prop_count = acpigen_write_package(0); + /* Package (DP), default to assuming no properties or children */ + dp_count = acpigen_write_package(0);
/* Print base properties */ for (dp = prop; dp; dp = dp->next) { if (dp->type == ACPI_DP_TYPE_CHILD) { child_count++; } else { + /* + * The UUID and package is only added when + * we come across the first property. This + * is to avoid creating a zero-length package + * in situations where there are only children. + */ + if (!prop_count) { + *dp_count += 2; + /* ToUUID (ACPI_DP_UUID) */ + acpigen_write_uuid(ACPI_DP_UUID); + /* + * Package (PROP), element count determined as + * it is populated + */ + prop_count = acpigen_write_package(0); + } (*prop_count)++; acpi_dp_write_property(dp); } } - - /* Package (PROP) length */ - acpigen_pop_len(); + if (prop_count) { + /* Package (PROP) length, if a package was written */ + acpigen_pop_len(); + }
if (child_count) { - /* Update DP package count to 4 */ - *dp_count = 4; - + /* Update DP package count to 2 or 4 */ + *dp_count += 2; /* ToUUID (ACPI_DP_CHILD_UUID) */ acpigen_write_uuid(ACPI_DP_CHILD_UUID);
@@ -679,7 +689,7 @@ for (dp = prop; dp; dp = dp->next) if (dp->type == ACPI_DP_TYPE_CHILD) acpi_dp_write_property(dp); - + /* Package (CHILD) length */ acpigen_pop_len(); }
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31161 )
Change subject: acpi: device: avoid empty property list in acpi_dp_write ......................................................................
Patch Set 1:
Does IASL or FWTS complain about that?
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31161 )
Change subject: acpi: device: avoid empty property list in acpi_dp_write ......................................................................
Patch Set 1:
Patch Set 1: Does IASL or FWTS complain about that?
Complain about the original or the proposed patch? The original would only exhibit an issue (or at least questionable behavior) for a device that lacks properties, and so far I haven't come across an existing example in coreboot (they all have properties or properties+children).
I noticed the problem when trying to port the various ipu_mainboard.asl and ipu_endpoints.asl files in coreboot to acpigen (e.g., under mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/ and nearby directories). The ipu_mainboard.asl specifies a _DSD that only has children--when I trying porting this to acpigen and diff'ed the result I noticed the extra empty properties UUID/package. With this change these is no difference after the port. These asl files create ACPI devices with tables that have 1) only properties, 2) only children, and 3) both properties and children, so my port to acpigen happens to exercise the 3 main use cases of this function (the 4th case is neither properties nor children, but I wasn't sure whether to also change the handling for that or just expect that a caller shouldn't call the method if there's no properties/children to write).
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31161 )
Change subject: acpi: device: avoid empty property list in acpi_dp_write ......................................................................
Patch Set 1:
Does IASL or FWTS complain about that?
The errors reported by FWTS (tested against the entire WIP commit chain) appear to be unrelated:
_SB_.PCI0.I2C4.MAXR._HID returned a string 'MX98373' but it was not a valid PNP ID or a valid ACPI ID. _SB_.PCI0.I2C4.MAXL._HID returned a string 'MX98373' but it was not a valid PNP ID or a valid ACPI ID. _SB_.PCI0.XHCI._PS0 returned values, but was expected to return nothing. Object returned: INTEGER: 0x00000000 _SB_.PCI0.XHCI._PS3 returned values, but was expected to return nothing. Object returned: INTEGER: 0x00000000 _SB_.PCI0.B0D4._PTC should contain only Resource Descriptors _SB_.PCI0.B0D4._TSD sub-package 0 element 3 (CoordType) was expected to have value 0xfc (SW_ALL), 0xfd (SW_ANY) or 0xfe (HW_ALL), instead it was 0x0. _SB_.PCI0.B0D4._TSS sub-package 0 element 0 was expected to have value 1..100, instead was 0. MCFG MMIO config space at 0xe0000000 is not reserved in the memory map table DBG2 Generic Address Structure has zero register bit width which is probably incorrect
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31161 )
Change subject: acpi: device: avoid empty property list in acpi_dp_write ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31161 )
Change subject: acpi: device: avoid empty property list in acpi_dp_write ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31161 )
Change subject: acpi: device: avoid empty property list in acpi_dp_write ......................................................................
acpi: device: avoid empty property list in acpi_dp_write
If an acpi_dp table has children but no properties then acpi_dp_write() will write out a properties UUID and package that contains no properties. The existing function will avoid writing out a UUID and empty package when no children exist, but it seems to assume that properties will always be used. With this change properties are handled in a manner akin to children so that a UUID and package are only written if properties exist.
BUG=none BRANCH=none TEST=Confirmed that prior to this change a UUID and empty package was present for a device that had children but no properties. Verified that after this change the UUID and empty package are no longer present but the child UUID and package are still present.
Change-Id: I6f5597713a1e91ca26b409f36b3ff9eb90a010af Signed-off-by: Matt Delco delco@chromium.org Reviewed-on: https://review.coreboot.org/c/31161 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Duncan Laurie dlaurie@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/arch/x86/acpi_device.c 1 file changed, 26 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 48b7fee..c62d6f3 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -632,7 +632,7 @@ void acpi_dp_write(struct acpi_dp *table) { struct acpi_dp *dp, *prop; - char *dp_count, *prop_count; + char *dp_count, *prop_count = NULL; int child_count = 0;
if (!table || table->type != ACPI_DP_TYPE_TABLE) @@ -644,32 +644,42 @@ /* Device Property list starts with the next entry */ prop = table->next;
- /* Package (DP), default to 2 elements (assuming no children) */ - dp_count = acpigen_write_package(2); - - /* ToUUID (ACPI_DP_UUID) */ - acpigen_write_uuid(ACPI_DP_UUID); - - /* Package (PROP), element count determined as it is populated */ - prop_count = acpigen_write_package(0); + /* Package (DP), default to assuming no properties or children */ + dp_count = acpigen_write_package(0);
/* Print base properties */ for (dp = prop; dp; dp = dp->next) { if (dp->type == ACPI_DP_TYPE_CHILD) { child_count++; } else { + /* + * The UUID and package is only added when + * we come across the first property. This + * is to avoid creating a zero-length package + * in situations where there are only children. + */ + if (!prop_count) { + *dp_count += 2; + /* ToUUID (ACPI_DP_UUID) */ + acpigen_write_uuid(ACPI_DP_UUID); + /* + * Package (PROP), element count determined as + * it is populated + */ + prop_count = acpigen_write_package(0); + } (*prop_count)++; acpi_dp_write_property(dp); } } - - /* Package (PROP) length */ - acpigen_pop_len(); + if (prop_count) { + /* Package (PROP) length, if a package was written */ + acpigen_pop_len(); + }
if (child_count) { - /* Update DP package count to 4 */ - *dp_count = 4; - + /* Update DP package count to 2 or 4 */ + *dp_count += 2; /* ToUUID (ACPI_DP_CHILD_UUID) */ acpigen_write_uuid(ACPI_DP_CHILD_UUID);
@@ -679,7 +689,7 @@ for (dp = prop; dp; dp = dp->next) if (dp->type == ACPI_DP_TYPE_CHILD) acpi_dp_write_property(dp); - + /* Package (CHILD) length */ acpigen_pop_len(); }
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Matt Delco (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)