NACK: [PATCH] acpi: method: add helper to check for package contents
Colin Ian King
colin.king at canonical.com
Thu Jan 10 19:06:43 UTC 2013
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 at 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 at 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) {
>
More information about the fwts-devel
mailing list