| Message ID | 1357823270-24887-1-git-send-email-colin.king@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
This was a partial fix. Ignore this patch, complete patch coming along soon. NACK. On 10/01/13 13:07, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Add method_package_elements_all_type() that checks to see if the > package contains elements that are all of a specified type. This > allows us to remove a lot of duplicated code. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > src/acpi/method/method.c | 237 ++++++++++++++++++++--------------------------- > 1 file changed, 103 insertions(+), 134 deletions(-) > > diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c > index 21c89c0..5b1cfdf 100644 > --- a/src/acpi/method/method.c > +++ b/src/acpi/method/method.c > @@ -746,6 +746,64 @@ static void method_test_polling_return( > } > } > > +/* > + * Common types that can be returned. This is not a complete > + * list but it does cover the types we expect to return from > + * an ACPI evaluation. > + */ > +static const char *method_type_name(const ACPI_OBJECT_TYPE type) > +{ > + switch (type) { > + case ACPI_TYPE_INTEGER: > + return "integer"; > + case ACPI_TYPE_STRING: > + return "string"; > + case ACPI_TYPE_BUFFER: > + return "buffer"; > + case ACPI_TYPE_PACKAGE: > + return "package"; > + case ACPI_TYPE_BUFFER_FIELD: > + return "buffer_field"; > + case ACPI_TYPE_LOCAL_REFERENCE: > + return "reference"; > + default: > + return "unknown"; > + } > +} > + > +/* > + * method_package_elements_all_type() > + * sanity check fields in a package that all have > + * the same type > + */ > +static int method_package_elements_all_type( > + fwts_framework *fw, > + const char *name, > + const char *objname, > + const ACPI_OBJECT *obj, > + const ACPI_OBJECT_TYPE type) > +{ > + uint32_t i; > + bool failed = false; > + char tmp[128]; > + > + for (i = 0; i < obj->Package.Count; i++) { > + if (obj->Package.Elements[i].Type != type) { > + snprintf(tmp, sizeof(tmp), "Method%sElementType", objname); > + fwts_failed(fw, LOG_LEVEL_MEDIUM, tmp, > + "%s package element %" PRIu32 " was not the expected " > + "type '%s', was instead type '%s'.", > + name, i, > + method_type_name(type), > + method_type_name(obj->Package.Elements[i].Type)); > + fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + failed = true; > + } > + } > + > + return failed ? FWTS_ERROR: FWTS_OK; > +} > + > /****************************************************************************/ > > /* > @@ -1855,28 +1913,16 @@ static void method_test_EDL_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - bool failed = false; > - > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > return; > > - /* All elements in the package must be references */ > - for (i = 0; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_LOCAL_REFERENCE) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_EDLElementType", > - "%s package element %" PRIu32 " was not a reference.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed = true; > - } > - } > + if (method_package_elements_all_type(fw, name, "_EDL", > + obj, ACPI_TYPE_LOCAL_REFERENCE) != FWTS_OK) > + return; > > - if (!failed) > - method_passed_sane(fw, name, "package"); > + method_passed_sane(fw, name, "package"); > } > > static int method_test_EDL(fwts_framework *fw) > @@ -2103,28 +2149,18 @@ static void method_test_power_resources_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - bool failed = false; > + char *objname = (char *)private; > > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > return; > > - /* All elements in the package must be references */ > - for (i = 0; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_LOCAL_REFERENCE) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_PowerResourceElementType", > - "%s package element %" PRIu32 " was not a reference.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed = true; > - } > - } > + if (method_package_elements_all_type(fw, name, objname, > + obj, ACPI_TYPE_LOCAL_REFERENCE) != FWTS_OK) > + return; > > - if (!failed) > - method_passed_sane(fw, name, "package"); > + method_passed_sane(fw, name, "package"); > } > > #define method_test_POWER(name) \ > @@ -2422,12 +2458,8 @@ static void method_test_CSD_return( > uint32_t j; > bool elements_ok = true; > > - if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_CSDElementType", > - "%s package element %" PRIu32 " was not a package.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_CSD", > + obj, ACPI_TYPE_PACKAGE) != FWTS_OK) { > failed = true; > continue; /* Skip processing sub-package */ > } > @@ -2694,9 +2726,6 @@ static void method_test_PCT_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - bool failed = false; > - > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > @@ -2706,23 +2735,11 @@ static void method_test_PCT_return( > if (method_package_count_min(fw, name, "_PCT", obj, 2) != FWTS_OK) > return; > > - for (i = 0; i < obj->Package.Count; i++) { > - /* > - * Fairly shallow checks here, should probably check > - * for a register description in the buffer > - */ > - if (obj->Package.Elements[i].Type != ACPI_TYPE_BUFFER) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_PCTElementType", > - "%s package element %" PRIu32 > - " was not a buffer.", name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed = true; > - continue; /* Skip processing sub-package */ > - } > - } > - if (!failed) > - method_passed_sane(fw, name, "package"); > + if (method_package_elements_all_type(fw, name, "_PCT", > + obj, ACPI_TYPE_BUFFER) != FWTS_OK) > + return; > + > + method_passed_sane(fw, name, "package"); > } > > static int method_test_PCT(fwts_framework *fw) > @@ -3762,8 +3779,7 @@ static void method_test_BST_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - int failed = 0; > + bool failed = false; > > FWTS_UNUSED(private); > > @@ -3773,17 +3789,8 @@ static void method_test_BST_return( > if (method_package_count_equal(fw, name, "_BST", obj, 4) != FWTS_OK) > return; > > - for (i = 0; (i < 4) && (i < obj->Package.Count); i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_BSTBadType", > - "%s package element %" PRIu32 > - " is not of type DWORD Integer.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > - } > - } > + if (method_package_elements_all_type(fw, name, "_BST", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > + return; > > /* Sanity check each field */ > /* Battery State */ > @@ -3794,7 +3801,7 @@ static void method_test_BST_return( > "be 0..7, got 0x%8.8" PRIx64 ".", > name, obj->Package.Elements[0].Integer.Value); > fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > + failed = true; > } > /* Ensure bits 0 (discharging) and 1 (charging) are not both set, see 10.2.2.6 */ > if (((obj->Package.Elements[0].Integer.Value) & 3) == 3) { > @@ -3805,7 +3812,7 @@ static void method_test_BST_return( > "which is not allowed. Got value 0x%8.8" PRIx64 ".", > name, obj->Package.Elements[0].Integer.Value); > fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > + failed = true; > } > /* Battery Present Rate - cannot check, pulled from EC */ > /* Battery Remaining Capacity - cannot check, pulled from EC */ > @@ -3889,9 +3896,6 @@ static void method_test_BMD_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - int failed = 0; > - > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > @@ -3900,22 +3904,12 @@ static void method_test_BMD_return( > if (method_package_count_equal(fw, name, "_BMD", obj, 5) != FWTS_OK) > return; > > + if (method_package_elements_all_type(fw, name, "_BMD", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > + return; > + > fwts_acpi_object_dump(fw, obj); > > - for (i= 0; (i < 4) && (i < obj->Package.Count); i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_BMDBadType", > - "%s package element %" PRIu32 > - " is not of type DWORD Integer.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > - } > - } > - /* TODO: check return values */ > - if (!failed) > - method_passed_sane(fw, name, "package"); > + method_passed_sane(fw, name, "package"); > } > > static int method_test_BMD(fwts_framework *fw) > @@ -4031,17 +4025,8 @@ static void method_test_FIF_return( > if (method_package_count_equal(fw, name, "_FIF", obj, 4) != FWTS_OK) > return; > > - fwts_acpi_object_dump(fw, obj); > - > - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[2].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[3].Type != ACPI_TYPE_INTEGER)) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_FIFBadType", > - "%s should return package of 4 " > - "integers.", name); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_FIF", > + obj, ACPI_TYPE_INTEGER) != FWTS_OK) { > fwts_advice(fw, > "%s is not returning the correct " > "fan information. It may be worth " > @@ -4049,8 +4034,12 @@ static void method_test_FIF_return( > "interactive 'fan' test to see if " > "this affects the control and " > "operation of the fan.", name); > - } else > - method_passed_sane(fw, name, "package"); > + return; > + } > + > + fwts_acpi_object_dump(fw, obj); > + > + method_passed_sane(fw, name, "package"); > } > > static int method_test_FIF(fwts_framework *fw) > @@ -4084,15 +4073,8 @@ static void method_test_FST_return( > if (method_package_count_equal(fw, name, "_FST", obj, 3) != FWTS_OK) > return; > > - fwts_acpi_object_dump(fw, obj); > - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[2].Type != ACPI_TYPE_INTEGER)) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_FSTBadType", > - "%s should return package of 3 " > - "integers.", name); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_FST", > + obj, ACPI_TYPE_INTEGER) != FWTS_OK) { > fwts_advice(fw, > "%s is not returning the correct " > "fan status information. It may be " > @@ -4100,8 +4082,12 @@ static void method_test_FST_return( > "suite interactive 'fan' test to see " > "if this affects the control and " > "operation of the fan.", name); > - } else > - method_passed_sane(fw, name, "package"); > + return; > + } > + > + fwts_acpi_object_dump(fw, obj); > + > + method_passed_sane(fw, name, "package"); > } > > static int method_test_FST(fwts_framework *fw) > @@ -4416,16 +4402,8 @@ static void method_test_WAK_return( > if (method_package_count_equal(fw, name, "_WAK", obj, 2) != FWTS_OK) > return; > > - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER)) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_WAKBadType", > - "%s should return package of 2 " > - "integers, got %" PRIu32 " instead.", > - name, obj->Package.Count); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_WAK", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > return; > - } > > method_passed_sane(fw, name, "package"); > } > @@ -4618,19 +4596,10 @@ static void method_test_BCL_return( > if (method_package_count_min(fw, name, "_BCL", obj, 3) != FWTS_OK) > return; > > - fwts_acpi_object_dump(fw, obj); > - > - for (i = 0; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) > - failed++; > - } > - if (failed) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_BCLNoPackage", > - "%s did not return a package of %" PRIu32 > - " integers.", name, obj->Package.Count); > + if (method_package_elements_all_type(fw, name, "_FIF", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > return; > - } > + > + fwts_acpi_object_dump(fw, obj); > > if (obj->Package.Elements[0].Integer.Value < > obj->Package.Elements[1].Integer.Value) { >
On 01/10/2013 09:07 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Add method_package_elements_all_type() that checks to see if the > package contains elements that are all of a specified type. This > allows us to remove a lot of duplicated code. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > src/acpi/method/method.c | 237 ++++++++++++++++++++--------------------------- > 1 file changed, 103 insertions(+), 134 deletions(-) > > diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c > index 21c89c0..5b1cfdf 100644 > --- a/src/acpi/method/method.c > +++ b/src/acpi/method/method.c > @@ -746,6 +746,64 @@ static void method_test_polling_return( > } > } > > +/* > + * Common types that can be returned. This is not a complete > + * list but it does cover the types we expect to return from > + * an ACPI evaluation. > + */ > +static const char *method_type_name(const ACPI_OBJECT_TYPE type) > +{ > + switch (type) { > + case ACPI_TYPE_INTEGER: > + return "integer"; > + case ACPI_TYPE_STRING: > + return "string"; > + case ACPI_TYPE_BUFFER: > + return "buffer"; > + case ACPI_TYPE_PACKAGE: > + return "package"; > + case ACPI_TYPE_BUFFER_FIELD: > + return "buffer_field"; > + case ACPI_TYPE_LOCAL_REFERENCE: > + return "reference"; > + default: > + return "unknown"; > + } > +} > + > +/* > + * method_package_elements_all_type() > + * sanity check fields in a package that all have > + * the same type > + */ > +static int method_package_elements_all_type( > + fwts_framework *fw, > + const char *name, > + const char *objname, > + const ACPI_OBJECT *obj, > + const ACPI_OBJECT_TYPE type) > +{ > + uint32_t i; > + bool failed = false; > + char tmp[128]; > + > + for (i = 0; i < obj->Package.Count; i++) { > + if (obj->Package.Elements[i].Type != type) { > + snprintf(tmp, sizeof(tmp), "Method%sElementType", objname); > + fwts_failed(fw, LOG_LEVEL_MEDIUM, tmp, > + "%s package element %" PRIu32 " was not the expected " > + "type '%s', was instead type '%s'.", > + name, i, > + method_type_name(type), > + method_type_name(obj->Package.Elements[i].Type)); > + fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + failed = true; > + } > + } > + > + return failed ? FWTS_ERROR: FWTS_OK; > +} > + > /****************************************************************************/ > > /* > @@ -1855,28 +1913,16 @@ static void method_test_EDL_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - bool failed = false; > - > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > return; > > - /* All elements in the package must be references */ > - for (i = 0; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_LOCAL_REFERENCE) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_EDLElementType", > - "%s package element %" PRIu32 " was not a reference.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed = true; > - } > - } > + if (method_package_elements_all_type(fw, name, "_EDL", > + obj, ACPI_TYPE_LOCAL_REFERENCE) != FWTS_OK) > + return; > > - if (!failed) > - method_passed_sane(fw, name, "package"); > + method_passed_sane(fw, name, "package"); > } > > static int method_test_EDL(fwts_framework *fw) > @@ -2103,28 +2149,18 @@ static void method_test_power_resources_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - bool failed = false; > + char *objname = (char *)private; > > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > return; > > - /* All elements in the package must be references */ > - for (i = 0; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_LOCAL_REFERENCE) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_PowerResourceElementType", > - "%s package element %" PRIu32 " was not a reference.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed = true; > - } > - } > + if (method_package_elements_all_type(fw, name, objname, > + obj, ACPI_TYPE_LOCAL_REFERENCE) != FWTS_OK) > + return; > > - if (!failed) > - method_passed_sane(fw, name, "package"); > + method_passed_sane(fw, name, "package"); > } > > #define method_test_POWER(name) \ > @@ -2422,12 +2458,8 @@ static void method_test_CSD_return( > uint32_t j; > bool elements_ok = true; > > - if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_CSDElementType", > - "%s package element %" PRIu32 " was not a package.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_CSD", > + obj, ACPI_TYPE_PACKAGE) != FWTS_OK) { > failed = true; > continue; /* Skip processing sub-package */ > } > @@ -2694,9 +2726,6 @@ static void method_test_PCT_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - bool failed = false; > - > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > @@ -2706,23 +2735,11 @@ static void method_test_PCT_return( > if (method_package_count_min(fw, name, "_PCT", obj, 2) != FWTS_OK) > return; > > - for (i = 0; i < obj->Package.Count; i++) { > - /* > - * Fairly shallow checks here, should probably check > - * for a register description in the buffer > - */ > - if (obj->Package.Elements[i].Type != ACPI_TYPE_BUFFER) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_PCTElementType", > - "%s package element %" PRIu32 > - " was not a buffer.", name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed = true; > - continue; /* Skip processing sub-package */ > - } > - } > - if (!failed) > - method_passed_sane(fw, name, "package"); > + if (method_package_elements_all_type(fw, name, "_PCT", > + obj, ACPI_TYPE_BUFFER) != FWTS_OK) > + return; > + > + method_passed_sane(fw, name, "package"); > } > > static int method_test_PCT(fwts_framework *fw) > @@ -3762,8 +3779,7 @@ static void method_test_BST_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - int failed = 0; > + bool failed = false; > > FWTS_UNUSED(private); > > @@ -3773,17 +3789,8 @@ static void method_test_BST_return( > if (method_package_count_equal(fw, name, "_BST", obj, 4) != FWTS_OK) > return; > > - for (i = 0; (i < 4) && (i < obj->Package.Count); i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_BSTBadType", > - "%s package element %" PRIu32 > - " is not of type DWORD Integer.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > - } > - } > + if (method_package_elements_all_type(fw, name, "_BST", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > + return; > > /* Sanity check each field */ > /* Battery State */ > @@ -3794,7 +3801,7 @@ static void method_test_BST_return( > "be 0..7, got 0x%8.8" PRIx64 ".", > name, obj->Package.Elements[0].Integer.Value); > fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > + failed = true; > } > /* Ensure bits 0 (discharging) and 1 (charging) are not both set, see 10.2.2.6 */ > if (((obj->Package.Elements[0].Integer.Value) & 3) == 3) { > @@ -3805,7 +3812,7 @@ static void method_test_BST_return( > "which is not allowed. Got value 0x%8.8" PRIx64 ".", > name, obj->Package.Elements[0].Integer.Value); > fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > + failed = true; > } > /* Battery Present Rate - cannot check, pulled from EC */ > /* Battery Remaining Capacity - cannot check, pulled from EC */ > @@ -3889,9 +3896,6 @@ static void method_test_BMD_return( > ACPI_OBJECT *obj, > void *private) > { > - uint32_t i; > - int failed = 0; > - > FWTS_UNUSED(private); > > if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) > @@ -3900,22 +3904,12 @@ static void method_test_BMD_return( > if (method_package_count_equal(fw, name, "_BMD", obj, 5) != FWTS_OK) > return; > > + if (method_package_elements_all_type(fw, name, "_BMD", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > + return; > + > fwts_acpi_object_dump(fw, obj); > > - for (i= 0; (i < 4) && (i < obj->Package.Count); i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_BMDBadType", > - "%s package element %" PRIu32 > - " is not of type DWORD Integer.", > - name, i); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > - failed++; > - } > - } > - /* TODO: check return values */ > - if (!failed) > - method_passed_sane(fw, name, "package"); > + method_passed_sane(fw, name, "package"); > } > > static int method_test_BMD(fwts_framework *fw) > @@ -4031,17 +4025,8 @@ static void method_test_FIF_return( > if (method_package_count_equal(fw, name, "_FIF", obj, 4) != FWTS_OK) > return; > > - fwts_acpi_object_dump(fw, obj); > - > - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[2].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[3].Type != ACPI_TYPE_INTEGER)) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_FIFBadType", > - "%s should return package of 4 " > - "integers.", name); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_FIF", > + obj, ACPI_TYPE_INTEGER) != FWTS_OK) { > fwts_advice(fw, > "%s is not returning the correct " > "fan information. It may be worth " > @@ -4049,8 +4034,12 @@ static void method_test_FIF_return( > "interactive 'fan' test to see if " > "this affects the control and " > "operation of the fan.", name); > - } else > - method_passed_sane(fw, name, "package"); > + return; > + } > + > + fwts_acpi_object_dump(fw, obj); > + > + method_passed_sane(fw, name, "package"); > } > > static int method_test_FIF(fwts_framework *fw) > @@ -4084,15 +4073,8 @@ static void method_test_FST_return( > if (method_package_count_equal(fw, name, "_FST", obj, 3) != FWTS_OK) > return; > > - fwts_acpi_object_dump(fw, obj); > - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[2].Type != ACPI_TYPE_INTEGER)) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_FSTBadType", > - "%s should return package of 3 " > - "integers.", name); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_FST", > + obj, ACPI_TYPE_INTEGER) != FWTS_OK) { > fwts_advice(fw, > "%s is not returning the correct " > "fan status information. It may be " > @@ -4100,8 +4082,12 @@ static void method_test_FST_return( > "suite interactive 'fan' test to see " > "if this affects the control and " > "operation of the fan.", name); > - } else > - method_passed_sane(fw, name, "package"); > + return; > + } > + > + fwts_acpi_object_dump(fw, obj); > + > + method_passed_sane(fw, name, "package"); > } > > static int method_test_FST(fwts_framework *fw) > @@ -4416,16 +4402,8 @@ static void method_test_WAK_return( > if (method_package_count_equal(fw, name, "_WAK", obj, 2) != FWTS_OK) > return; > > - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || > - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER)) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_WAKBadType", > - "%s should return package of 2 " > - "integers, got %" PRIu32 " instead.", > - name, obj->Package.Count); > - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); > + if (method_package_elements_all_type(fw, name, "_WAK", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > return; > - } > > method_passed_sane(fw, name, "package"); > } > @@ -4618,19 +4596,10 @@ static void method_test_BCL_return( > if (method_package_count_min(fw, name, "_BCL", obj, 3) != FWTS_OK) > return; > > - fwts_acpi_object_dump(fw, obj); > - > - for (i = 0; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) > - failed++; > - } > - if (failed) { > - fwts_failed(fw, LOG_LEVEL_MEDIUM, > - "Method_BCLNoPackage", > - "%s did not return a package of %" PRIu32 > - " integers.", name, obj->Package.Count); > + if (method_package_elements_all_type(fw, name, "_FIF", obj, ACPI_TYPE_INTEGER) != FWTS_OK) > return; > - } > + > + fwts_acpi_object_dump(fw, obj); > > if (obj->Package.Elements[0].Integer.Value < > obj->Package.Elements[1].Integer.Value) { > Acked-by: Ivan Hu <ivan.hu@canonical.com>
diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c index 21c89c0..5b1cfdf 100644 --- a/src/acpi/method/method.c +++ b/src/acpi/method/method.c @@ -746,6 +746,64 @@ static void method_test_polling_return( } } +/* + * Common types that can be returned. This is not a complete + * list but it does cover the types we expect to return from + * an ACPI evaluation. + */ +static const char *method_type_name(const ACPI_OBJECT_TYPE type) +{ + switch (type) { + case ACPI_TYPE_INTEGER: + return "integer"; + case ACPI_TYPE_STRING: + return "string"; + case ACPI_TYPE_BUFFER: + return "buffer"; + case ACPI_TYPE_PACKAGE: + return "package"; + case ACPI_TYPE_BUFFER_FIELD: + return "buffer_field"; + case ACPI_TYPE_LOCAL_REFERENCE: + return "reference"; + default: + return "unknown"; + } +} + +/* + * method_package_elements_all_type() + * sanity check fields in a package that all have + * the same type + */ +static int method_package_elements_all_type( + fwts_framework *fw, + const char *name, + const char *objname, + const ACPI_OBJECT *obj, + const ACPI_OBJECT_TYPE type) +{ + uint32_t i; + bool failed = false; + char tmp[128]; + + for (i = 0; i < obj->Package.Count; i++) { + if (obj->Package.Elements[i].Type != type) { + snprintf(tmp, sizeof(tmp), "Method%sElementType", objname); + fwts_failed(fw, LOG_LEVEL_MEDIUM, tmp, + "%s package element %" PRIu32 " was not the expected " + "type '%s', was instead type '%s'.", + name, i, + method_type_name(type), + method_type_name(obj->Package.Elements[i].Type)); + fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); + failed = true; + } + } + + return failed ? FWTS_ERROR: FWTS_OK; +} + /****************************************************************************/ /* @@ -1855,28 +1913,16 @@ static void method_test_EDL_return( ACPI_OBJECT *obj, void *private) { - uint32_t i; - bool failed = false; - FWTS_UNUSED(private); if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) return; - /* All elements in the package must be references */ - for (i = 0; i < obj->Package.Count; i++) { - if (obj->Package.Elements[i].Type != ACPI_TYPE_LOCAL_REFERENCE) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_EDLElementType", - "%s package element %" PRIu32 " was not a reference.", - name, i); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); - failed = true; - } - } + if (method_package_elements_all_type(fw, name, "_EDL", + obj, ACPI_TYPE_LOCAL_REFERENCE) != FWTS_OK) + return; - if (!failed) - method_passed_sane(fw, name, "package"); + method_passed_sane(fw, name, "package"); } static int method_test_EDL(fwts_framework *fw) @@ -2103,28 +2149,18 @@ static void method_test_power_resources_return( ACPI_OBJECT *obj, void *private) { - uint32_t i; - bool failed = false; + char *objname = (char *)private; FWTS_UNUSED(private); if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) return; - /* All elements in the package must be references */ - for (i = 0; i < obj->Package.Count; i++) { - if (obj->Package.Elements[i].Type != ACPI_TYPE_LOCAL_REFERENCE) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_PowerResourceElementType", - "%s package element %" PRIu32 " was not a reference.", - name, i); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); - failed = true; - } - } + if (method_package_elements_all_type(fw, name, objname, + obj, ACPI_TYPE_LOCAL_REFERENCE) != FWTS_OK) + return; - if (!failed) - method_passed_sane(fw, name, "package"); + method_passed_sane(fw, name, "package"); } #define method_test_POWER(name) \ @@ -2422,12 +2458,8 @@ static void method_test_CSD_return( uint32_t j; bool elements_ok = true; - if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_CSDElementType", - "%s package element %" PRIu32 " was not a package.", - name, i); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); + if (method_package_elements_all_type(fw, name, "_CSD", + obj, ACPI_TYPE_PACKAGE) != FWTS_OK) { failed = true; continue; /* Skip processing sub-package */ } @@ -2694,9 +2726,6 @@ static void method_test_PCT_return( ACPI_OBJECT *obj, void *private) { - uint32_t i; - bool failed = false; - FWTS_UNUSED(private); if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) @@ -2706,23 +2735,11 @@ static void method_test_PCT_return( if (method_package_count_min(fw, name, "_PCT", obj, 2) != FWTS_OK) return; - for (i = 0; i < obj->Package.Count; i++) { - /* - * Fairly shallow checks here, should probably check - * for a register description in the buffer - */ - if (obj->Package.Elements[i].Type != ACPI_TYPE_BUFFER) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_PCTElementType", - "%s package element %" PRIu32 - " was not a buffer.", name, i); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); - failed = true; - continue; /* Skip processing sub-package */ - } - } - if (!failed) - method_passed_sane(fw, name, "package"); + if (method_package_elements_all_type(fw, name, "_PCT", + obj, ACPI_TYPE_BUFFER) != FWTS_OK) + return; + + method_passed_sane(fw, name, "package"); } static int method_test_PCT(fwts_framework *fw) @@ -3762,8 +3779,7 @@ static void method_test_BST_return( ACPI_OBJECT *obj, void *private) { - uint32_t i; - int failed = 0; + bool failed = false; FWTS_UNUSED(private); @@ -3773,17 +3789,8 @@ static void method_test_BST_return( if (method_package_count_equal(fw, name, "_BST", obj, 4) != FWTS_OK) return; - for (i = 0; (i < 4) && (i < obj->Package.Count); i++) { - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_BSTBadType", - "%s package element %" PRIu32 - " is not of type DWORD Integer.", - name, i); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); - failed++; - } - } + if (method_package_elements_all_type(fw, name, "_BST", obj, ACPI_TYPE_INTEGER) != FWTS_OK) + return; /* Sanity check each field */ /* Battery State */ @@ -3794,7 +3801,7 @@ static void method_test_BST_return( "be 0..7, got 0x%8.8" PRIx64 ".", name, obj->Package.Elements[0].Integer.Value); fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); - failed++; + failed = true; } /* Ensure bits 0 (discharging) and 1 (charging) are not both set, see 10.2.2.6 */ if (((obj->Package.Elements[0].Integer.Value) & 3) == 3) { @@ -3805,7 +3812,7 @@ static void method_test_BST_return( "which is not allowed. Got value 0x%8.8" PRIx64 ".", name, obj->Package.Elements[0].Integer.Value); fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); - failed++; + failed = true; } /* Battery Present Rate - cannot check, pulled from EC */ /* Battery Remaining Capacity - cannot check, pulled from EC */ @@ -3889,9 +3896,6 @@ static void method_test_BMD_return( ACPI_OBJECT *obj, void *private) { - uint32_t i; - int failed = 0; - FWTS_UNUSED(private); if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK) @@ -3900,22 +3904,12 @@ static void method_test_BMD_return( if (method_package_count_equal(fw, name, "_BMD", obj, 5) != FWTS_OK) return; + if (method_package_elements_all_type(fw, name, "_BMD", obj, ACPI_TYPE_INTEGER) != FWTS_OK) + return; + fwts_acpi_object_dump(fw, obj); - for (i= 0; (i < 4) && (i < obj->Package.Count); i++) { - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_BMDBadType", - "%s package element %" PRIu32 - " is not of type DWORD Integer.", - name, i); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); - failed++; - } - } - /* TODO: check return values */ - if (!failed) - method_passed_sane(fw, name, "package"); + method_passed_sane(fw, name, "package"); } static int method_test_BMD(fwts_framework *fw) @@ -4031,17 +4025,8 @@ static void method_test_FIF_return( if (method_package_count_equal(fw, name, "_FIF", obj, 4) != FWTS_OK) return; - fwts_acpi_object_dump(fw, obj); - - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER) || - (obj->Package.Elements[2].Type != ACPI_TYPE_INTEGER) || - (obj->Package.Elements[3].Type != ACPI_TYPE_INTEGER)) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_FIFBadType", - "%s should return package of 4 " - "integers.", name); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); + if (method_package_elements_all_type(fw, name, "_FIF", + obj, ACPI_TYPE_INTEGER) != FWTS_OK) { fwts_advice(fw, "%s is not returning the correct " "fan information. It may be worth " @@ -4049,8 +4034,12 @@ static void method_test_FIF_return( "interactive 'fan' test to see if " "this affects the control and " "operation of the fan.", name); - } else - method_passed_sane(fw, name, "package"); + return; + } + + fwts_acpi_object_dump(fw, obj); + + method_passed_sane(fw, name, "package"); } static int method_test_FIF(fwts_framework *fw) @@ -4084,15 +4073,8 @@ static void method_test_FST_return( if (method_package_count_equal(fw, name, "_FST", obj, 3) != FWTS_OK) return; - fwts_acpi_object_dump(fw, obj); - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER) || - (obj->Package.Elements[2].Type != ACPI_TYPE_INTEGER)) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_FSTBadType", - "%s should return package of 3 " - "integers.", name); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); + if (method_package_elements_all_type(fw, name, "_FST", + obj, ACPI_TYPE_INTEGER) != FWTS_OK) { fwts_advice(fw, "%s is not returning the correct " "fan status information. It may be " @@ -4100,8 +4082,12 @@ static void method_test_FST_return( "suite interactive 'fan' test to see " "if this affects the control and " "operation of the fan.", name); - } else - method_passed_sane(fw, name, "package"); + return; + } + + fwts_acpi_object_dump(fw, obj); + + method_passed_sane(fw, name, "package"); } static int method_test_FST(fwts_framework *fw) @@ -4416,16 +4402,8 @@ static void method_test_WAK_return( if (method_package_count_equal(fw, name, "_WAK", obj, 2) != FWTS_OK) return; - if ((obj->Package.Elements[0].Type != ACPI_TYPE_INTEGER) || - (obj->Package.Elements[1].Type != ACPI_TYPE_INTEGER)) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_WAKBadType", - "%s should return package of 2 " - "integers, got %" PRIu32 " instead.", - name, obj->Package.Count); - fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN); + if (method_package_elements_all_type(fw, name, "_WAK", obj, ACPI_TYPE_INTEGER) != FWTS_OK) return; - } method_passed_sane(fw, name, "package"); } @@ -4618,19 +4596,10 @@ static void method_test_BCL_return( if (method_package_count_min(fw, name, "_BCL", obj, 3) != FWTS_OK) return; - fwts_acpi_object_dump(fw, obj); - - for (i = 0; i < obj->Package.Count; i++) { - if (obj->Package.Elements[i].Type != ACPI_TYPE_INTEGER) - failed++; - } - if (failed) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, - "Method_BCLNoPackage", - "%s did not return a package of %" PRIu32 - " integers.", name, obj->Package.Count); + if (method_package_elements_all_type(fw, name, "_FIF", obj, ACPI_TYPE_INTEGER) != FWTS_OK) return; - } + + fwts_acpi_object_dump(fw, obj); if (obj->Package.Elements[0].Integer.Value < obj->Package.Elements[1].Integer.Value) {