ACK: [PATCH 2/3] acpi: refactor checks for fixed values by fwts_acpi_fixed_value_check
Colin Ian King
colin.king at canonical.com
Tue Jan 5 08:49:57 UTC 2021
On 05/01/2021 04:19, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung at canonical.com>
> ---
> src/acpi/bgrt/bgrt.c | 20 ++------------------
> src/acpi/cpep/cpep.c | 26 ++++++++------------------
> src/acpi/dbg2/dbg2.c | 10 ++--------
> src/acpi/fpdt/fpdt.c | 29 +++++------------------------
> src/acpi/iort/iort.c | 10 ++--------
> src/acpi/msdm/msdm.c | 11 +++--------
> src/acpi/spcr/spcr.c | 18 ++----------------
> src/acpi/spmi/spmi.c | 9 +--------
> src/acpi/srat/srat.c | 8 +-------
> src/acpi/tcpa/tcpa.c | 38 ++++----------------------------------
> src/acpi/wpbt/wpbt.c | 19 ++++---------------
> src/acpi/xenv/xenv.c | 9 +--------
> 12 files changed, 35 insertions(+), 172 deletions(-)
>
> diff --git a/src/acpi/bgrt/bgrt.c b/src/acpi/bgrt/bgrt.c
> index e15044ce..0efee7dc 100644
> --- a/src/acpi/bgrt/bgrt.c
> +++ b/src/acpi/bgrt/bgrt.c
> @@ -46,25 +46,9 @@ static int bgrt_test1(fwts_framework *fw)
> fwts_log_info_verbatim(fw, " Image Offset X: 0x%8.8" PRIx32, bgrt->image_offset_x);
> fwts_log_info_verbatim(fw, " Image Offset Y: 0x%8.8" PRIx32, bgrt->image_offset_y);
>
> - if (bgrt->version != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "BGRTInvalidVersion",
> - "BGRT: Version field is 0x%" PRIx8
> - " and not the expected value of 0x01",
> - bgrt->version);
> - }
> -
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "BGRT", "Version", bgrt->version, 1, &passed);
> fwts_acpi_reserved_bits_check(fw, "BGRT", "BGRT Status", bgrt->status, sizeof(bgrt->status), 3, 7, &passed);
> -
> - if (bgrt->image_type > 0x00) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "BGRTInvalidImageType",
> - "BGRT: Image Type field is 0x%" PRIx8
> - ", the only allowed type is 0x00",
> - bgrt->image_type);
> - }
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "BGRT", "Image Type", bgrt->image_type, 0, &passed);
>
> if (passed)
> fwts_passed(fw, "No issues found in BGRT table.");
> diff --git a/src/acpi/cpep/cpep.c b/src/acpi/cpep/cpep.c
> index e0b0772f..2e0f4eda 100644
> --- a/src/acpi/cpep/cpep.c
> +++ b/src/acpi/cpep/cpep.c
> @@ -58,26 +58,16 @@ static int cpep_test1(fwts_framework *fw)
>
> for (i = 0; i < n; i++) {
> bool cpep_passed = true;
> + char label[40];
> +
> fwts_acpi_cpep_processor_info *info = &cpep->cpep_info[i];
>
> - if (info->type != 0) {
> - cpep_passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "CPEPInvalidProcStructureType",
> - "CPEP: Processor Structure %" PRIu32
> - " Type field is 0x%" PRIx8
> - " and was expected to be 0x00",
> - i, info->type);
> - }
> - if (info->length != 8) {
> - cpep_passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "CPEPInvalidProcStructureType",
> - "CPEP: Processor Structure %" PRIu32
> - " Length field is 0x%" PRIx8
> - " and was expected to be 0x08",
> - i, info->length);
> - }
> + snprintf(label, 40, "Processor Structure %d Type", i);
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "CPEP", label, info->type, 0, &cpep_passed);
> +
> + snprintf(label, 40, "Processor Structure %d Length", i);
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "CPEP", label, info->length, 8, &cpep_passed);
> +
> /* Should probably sanity check processor UID, EIDs at a later date */
>
> /* Some feedback if polling interval is very short */
> diff --git a/src/acpi/dbg2/dbg2.c b/src/acpi/dbg2/dbg2.c
> index d0e72fe1..2b4eb73a 100644
> --- a/src/acpi/dbg2/dbg2.c
> +++ b/src/acpi/dbg2/dbg2.c
> @@ -260,14 +260,8 @@ static int dbg2_test1(fwts_framework *fw)
> fwts_log_info_verbatim(fw, " Address Size Offset: 0x%4.4" PRIx16, info->address_size_offset);
> fwts_log_nl(fw);
>
> - if (info->revision != 0) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "DBG2NonZeroRevision",
> - "DBG2 Info Structure Revision is 0x%2.2" PRIx8
> - " and was expecting 0x00",
> - info->revision);
> - }
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "DBG2", "Info Structure Revision", info->revision, 0, &passed);
> +
> if (port == NULL) {
> passed = false;
> fwts_failed(fw, LOG_LEVEL_HIGH,
> diff --git a/src/acpi/fpdt/fpdt.c b/src/acpi/fpdt/fpdt.c
> index 085477bf..88cd55e9 100644
> --- a/src/acpi/fpdt/fpdt.c
> +++ b/src/acpi/fpdt/fpdt.c
> @@ -71,14 +71,7 @@ static int fpdt_test1(fwts_framework *fw)
> goto done;
> }
>
> - if (header->revision != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "FPDTBadRevision",
> - "FPDT revision is incorrect, expecting 0x01,"
> - " instead got 0x%2.2" PRIx8,
> - header->revision);
> - }
> + fwts_acpi_revision_check("FPDT", header->revision, 1, &passed);
>
> ptr += sizeof(fwts_acpi_table_header);
> while (ptr < data + table->length) {
> @@ -120,14 +113,8 @@ static int fpdt_test1(fwts_framework *fw)
> fwts_log_info(fw, "Note: currently fwts does not check FBPT validity and the associated data");
> }
>
> - if (fbbpr->fpdt.revision != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "FPDTInvalidRevision",
> - "FPDT: Revision field is 0x%" PRIx8
> - " and not the expected value of 0x01",
> - fbbpr->fpdt.revision);
> - }
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "FPDT", "FBPT Revision", fbbpr->fpdt.revision, 1, &passed);
> +
> /* Spec doesn't mention Reserved field should be 0 or not, skip checking the reserved field */
> break;
> case 0x0001: /* S3 Performance Table Pointer Record */
> @@ -153,14 +140,8 @@ static int fpdt_test1(fwts_framework *fw)
> fwts_log_info(fw, "Note: currently fwts does not check S3PT validity and the associated data");
> }
>
> - if (s3ptpr->fpdt.revision != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "FPDTInvalidRevision",
> - "FPDT: Revision field is 0x%" PRIx8
> - " and not the expected value of 0x01",
> - s3ptpr->fpdt.revision);
> - }
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "FPDT", "S3PT Revision", s3ptpr->fpdt.revision, 1, &passed);
> +
> /* Spec doesn't mention Reserved field should be 0 or not, skip checking the reserved field */
> break;
> case 0x0002 ... 0x0fff:
> diff --git a/src/acpi/iort/iort.c b/src/acpi/iort/iort.c
> index e3857c32..2f2d7838 100644
> --- a/src/acpi/iort/iort.c
> +++ b/src/acpi/iort/iort.c
> @@ -70,14 +70,8 @@ static void iort_node_check(
> node->revision);
> }
>
> - } else if (node->revision != 0) {
> - *passed = false;
> - fwts_failed(fw, LOG_LEVEL_LOW,
> - "IORTNodeRevisionNonZero",
> - "IORT Node Revision field is 0x%2.2" PRIx8
> - " and should be zero.",
> - node->revision);
> - }
> + } else
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "IORT", "IORT Node Revision", node->revision, 0, passed);
>
> fwts_acpi_reserved_zero_check(fw, "IORT", "Node Reserved", node->reserved, sizeof(node->reserved), passed);
>
> diff --git a/src/acpi/msdm/msdm.c b/src/acpi/msdm/msdm.c
> index 9f9d0dc7..bffc8699 100644
> --- a/src/acpi/msdm/msdm.c
> +++ b/src/acpi/msdm/msdm.c
> @@ -71,14 +71,9 @@ static int msdm_test1(fwts_framework *fw)
> switch (msdm->data_type) {
> case 0x0001:
> /* Check license key size */
> - if (msdm->data_length != 0x1d) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "MSDMDataLengthInvalid",
> - "MSDM Data Length field should be 0x0000001d, got 0x8.8%" PRIx32
> - " instead",
> - msdm->data_length);
> - } else {
> + if (msdm->data_length != 0x1d)
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "MSDM", "Data Length", msdm->data_length, 29, &passed);
> + else {
> size_t i;
> bool invalid = false;
> /* Note, no checks to see if this is a valid key! */
> diff --git a/src/acpi/spcr/spcr.c b/src/acpi/spcr/spcr.c
> index bde4ec9c..c05da580 100644
> --- a/src/acpi/spcr/spcr.c
> +++ b/src/acpi/spcr/spcr.c
> @@ -202,22 +202,8 @@ static int spcr_test1(fwts_framework *fw)
> " is a reserved baud rate", spcr->baud_rate);
> }
>
> - if (spcr->parity != 0) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "SPCRReservedValueUsed",
> - "SPCR Parity field must be zero, got 0x%2.2" PRIx8 " instead",
> - spcr->parity);
> - }
> -
> - if (spcr->stop_bits != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "SPCRReservedValueUsed",
> - "SPCR Stop field must be 1, got 0x%2.2" PRIx8 " instead",
> - spcr->stop_bits);
> - }
> -
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "SPCR", "Parity", spcr->parity, 0, &passed);
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "SPCR", "Stop", spcr->stop_bits, 1, &passed);
> fwts_acpi_reserved_bits_check(fw, "SPCR", "Flow control", spcr->flow_control, sizeof(spcr->flow_control), 3, 7, &passed);
>
> reserved = false;
> diff --git a/src/acpi/spmi/spmi.c b/src/acpi/spmi/spmi.c
> index ed7886dd..edabb494 100644
> --- a/src/acpi/spmi/spmi.c
> +++ b/src/acpi/spmi/spmi.c
> @@ -96,14 +96,7 @@ static int spmi_test1(fwts_framework *fw)
> spmi->interface_type);
> }
>
> - if (spmi->reserved1 != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_LOW,
> - "SPMIReserved1Not1",
> - "SPMI reserved field (offset 37) must be 0x01, got "
> - "0x%2.2" PRIx8 " instead", spmi->reserved1);
> - }
> -
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "SPMI", "Reserved1", spmi->reserved1, 1, &passed);
> fwts_acpi_reserved_bits_check(fw, "SPMI", "Interrupt type", spmi->interrupt_type, sizeof(spmi->interrupt_type), 2, 7, &passed);
>
> /* Check for zero GPE on specific condition of interrupt type */
> diff --git a/src/acpi/srat/srat.c b/src/acpi/srat/srat.c
> index 1dd19eae..56ee5e3d 100644
> --- a/src/acpi/srat/srat.c
> +++ b/src/acpi/srat/srat.c
> @@ -347,13 +347,7 @@ static int srat_test1(fwts_framework *fw)
> bool passed = true;
> ssize_t length = (ssize_t)srat->header.length;
>
> - if (srat->reserved1 != 1) {
> - fwts_failed(fw, LOG_LEVEL_MEDIUM,
> - "SRATInterfaceReserved",
> - "SRAT reserved field 1 should be 1, instead was "
> - "0x%" PRIx32, srat->reserved1);
> - passed = false;
> - }
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_MEDIUM, "SRAT", "Revision1", srat->reserved1, 1, &passed);
>
> data += sizeof(fwts_acpi_table_srat);
> length -= sizeof(fwts_acpi_table_srat);
> diff --git a/src/acpi/tcpa/tcpa.c b/src/acpi/tcpa/tcpa.c
> index 5904858e..817f7273 100644
> --- a/src/acpi/tcpa/tcpa.c
> +++ b/src/acpi/tcpa/tcpa.c
> @@ -30,23 +30,8 @@ static int tcpa_client_test(fwts_framework *fw, fwts_acpi_table_tcpa *tcpa)
> {
> bool passed = true;
>
> - if (tcpa->header.length != 50) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "TCPABadSize",
> - "TCPA size is incorrect, expecting 0x32 bytes, "
> - "instead got 0x%8.8" PRIx32 " bytes",
> - tcpa->header.length);
> - }
> -
> - if (tcpa->header.revision != 2) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "TCPABadRevision",
> - "TCPA revision is incorrect, expecting 0x02, "
> - "instead got 0x%2.2" PRIx8,
> - tcpa->header.revision);
> - }
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "TCPA", "Length", tcpa->header.length, 50, &passed);
> + fwts_acpi_revision_check("TCPA", tcpa->header.revision, 2, &passed);
>
> fwts_log_info_verbatim(fw, "TCPA Table:");
> fwts_log_info_verbatim(fw, " Platform Class: 0x%4.4" PRIx16, tcpa->platform_class);
> @@ -61,23 +46,8 @@ static int tcpa_server_test(fwts_framework *fw, fwts_acpi_table_tcpa *tcpa)
> bool passed = true;
> uint32_t reserved2;
>
> - if (tcpa->header.length != 100) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "TCPABadSize",
> - "TCPA size is incorrect, expecting 0x64 bytes, "
> - "instead got 0x%8.8" PRIx32 " bytes",
> - tcpa->header.length);
> - }
> -
> - if (tcpa->header.revision != 2) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "TCPABadRevision",
> - "TCPA revision is incorrect, expecting 0x02, "
> - "instead got 0x%2.2" PRIx8,
> - tcpa->header.revision);
> - }
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "TCPA", "Length", tcpa->header.length, 100, &passed);
> + fwts_acpi_revision_check("TCPA", tcpa->header.revision, 2, &passed);
>
> reserved2 = tcpa->server.reserved2[0] + (tcpa->server.reserved2[1] << 4) + (tcpa->server.reserved2[2] << 8);
>
> diff --git a/src/acpi/wpbt/wpbt.c b/src/acpi/wpbt/wpbt.c
> index 64eebf91..7082cff7 100644
> --- a/src/acpi/wpbt/wpbt.c
> +++ b/src/acpi/wpbt/wpbt.c
> @@ -44,22 +44,11 @@ static int wpbt_test1(fwts_framework *fw)
> fwts_log_info_verbatim(fw, " Content Layout: 0x%2.2" PRIx8, wpbt->layout);
> fwts_log_info_verbatim(fw, " Content Type: 0x%2.2" PRIx8, wpbt->type);
>
> - if (wpbt->layout != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "WPBTBadLayout",
> - "WPBT supports Content Layout 1 only, got "
> - "0x%2.2" PRIx8 " instead", wpbt->layout);
> - }
> -
> - if (wpbt->type != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "WPBTBadType",
> - "WPBT supports Content Type 1 only, got "
> - "0x%2.2" PRIx8 " instead", wpbt->type);
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "WPBT", "Layout", wpbt->layout, 1, &passed);
>
> - } else {
> + if (wpbt->type != 1)
> + fwts_acpi_fixed_value_check(fw, LOG_LEVEL_HIGH, "WPBT", "Type", wpbt->type, 1, &passed);
> + else {
> fwts_acpi_table_wpbt_type1 *type = (fwts_acpi_table_wpbt_type1 *) (table->data + sizeof(fwts_acpi_table_wpbt));
> fwts_log_info_verbatim(fw, " Arguments Length: 0x%4.4" PRIx16, type->arguments_length);
>
> diff --git a/src/acpi/xenv/xenv.c b/src/acpi/xenv/xenv.c
> index 012faac6..4199e934 100644
> --- a/src/acpi/xenv/xenv.c
> +++ b/src/acpi/xenv/xenv.c
> @@ -43,14 +43,7 @@ static int xenv_test1(fwts_framework *fw)
> return FWTS_ERROR;
> }
>
> - if (xenv->header.revision != 1) {
> - passed = false;
> - fwts_failed(fw, LOG_LEVEL_HIGH,
> - "XENVBadRevision",
> - "XENV revision is incorrect, expecting 0x01, "
> - "instead got 0x%2.2" PRIx8,
> - xenv->header.revision);
> - }
> + fwts_acpi_revision_check("XENV", xenv->header.revision, 1, &passed);
>
> fwts_log_info_verbatim(fw, "XENV Table:");
> fwts_log_info_verbatim(fw, " GNT Start Address: 0x%16.16" PRIx64, xenv->gnt_start);
>
Acked-by: Colin Ian King <colin.king at canonical.com>
More information about the fwts-devel
mailing list