Patchwork acpi: method: tidy up some more tests

login
register
mail settings
Submitter Colin King
Date Jan. 10, 2013, 11:52 a.m.
Message ID <1357818776-23141-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/210981/
State Accepted
Headers show

Comments

Colin King - Jan. 10, 2013, 11:52 a.m.
From: Colin Ian King <colin.king@canonical.com>

This patch tidies up some of the tests by checking for failures
early on and bailing out of the test early.  This means we can
remove one level of if statement nesting which makes the code
less convoluted.

Also remove some unwanted extra whitespaces.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpi/method/method.c | 159 ++++++++++++++++++++++++-----------------------
 1 file changed, 81 insertions(+), 78 deletions(-)
Keng-Yu Lin - Jan. 29, 2013, 5:46 a.m.
On Thu, Jan 10, 2013 at 7:52 PM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> This patch tidies up some of the tests by checking for failures
> early on and bailing out of the test early.  This means we can
> remove one level of if statement nesting which makes the code
> less convoluted.
>
> Also remove some unwanted extra whitespaces.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/acpi/method/method.c | 159 ++++++++++++++++++++++++-----------------------
>  1 file changed, 81 insertions(+), 78 deletions(-)
>
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index baf9c36..21c89c0 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -1937,25 +1937,26 @@ static void method_test_STA_return(
>
>         FWTS_UNUSED(private);
>
> -       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -               if ((obj->Integer.Value & 3) == 2) {
> -                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -                               "Method_STAEnabledNotPresent",
> -                               "%s indicates that the device is enabled "
> -                               "but not present, which is impossible.", name);
> -                       failed = true;
> -               }
> -               if ((obj->Integer.Value & ~0x1f) != 0) {
> -                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -                               "Method_STAReservedBitsSet",
> -                               "%s is returning non-zero reserved "
> -                               "bits 5-31. These should be zero.", name);
> -                       failed = true;
> -               }
> +       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +               return;
>
> -               if (!failed)
> -                       method_passed_sane_uint64(fw, name, obj->Integer.Value);
> +       if ((obj->Integer.Value & 3) == 2) {
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                       "Method_STAEnabledNotPresent",
> +                       "%s indicates that the device is enabled "
> +                       "but not present, which is impossible.", name);
> +               failed = true;
> +       }
> +       if ((obj->Integer.Value & ~0x1f) != 0) {
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                       "Method_STAReservedBitsSet",
> +                       "%s is returning non-zero reserved "
> +                       "bits 5-31. These should be zero.", name);
> +               failed = true;
>         }
> +
> +       if (!failed)
> +               method_passed_sane_uint64(fw, name, obj->Integer.Value);
>  }
>
>  static int method_test_STA(fwts_framework *fw)
> @@ -2011,18 +2012,19 @@ static void method_test_SEG_return(
>  {
>         FWTS_UNUSED(private);
>
> -       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -               if ((obj->Integer.Value & 0xffff0000)) {
> -                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -                               "Method_SEGIllegalReserved",
> -                               "%s returned value 0x%8.8" PRIx64 " and some of the "
> -                               "upper 16 reserved bits are set when they "
> -                               "should in fact be zero.",
> -                               name, obj->Integer.Value);
> -                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -               } else
> -                       method_passed_sane_uint64(fw, name, obj->Integer.Value);
> -       }
> +       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +               return;
> +
> +       if ((obj->Integer.Value & 0xffff0000)) {
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                       "Method_SEGIllegalReserved",
> +                       "%s returned value 0x%8.8" PRIx64 " and some of the "
> +                       "upper 16 reserved bits are set when they "
> +                       "should in fact be zero.",
> +                       name, obj->Integer.Value);
> +               fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +       } else
> +               method_passed_sane_uint64(fw, name, obj->Integer.Value);
>  }
>
>  static int method_test_SEG(fwts_framework *fw)
> @@ -2308,7 +2310,7 @@ static void method_test_type_integer(
>         int element,
>         char *message)
>  {
> -       if (obj->Package.Elements[element].Type  != ACPI_TYPE_INTEGER) {
> +       if (obj->Package.Elements[element].Type != ACPI_TYPE_INTEGER) {
>                 fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
>                         "_CPC package element %d (%s) was not an integer.",
>                         element, message);
> @@ -2323,7 +2325,7 @@ static void method_test_type_buffer(
>         int element,
>         char *message)
>  {
> -       if (obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER) {
> +       if (obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER) {
>                 fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
>                         "_CPC package element %d (%s) was not a buffer.",
>                         element, message);
> @@ -2338,8 +2340,8 @@ static void method_test_type_mixed(
>         int element,
>         char *message)
>  {
> -       if ((obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER) &&
> -           (obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER)) {
> +       if ((obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER) &&
> +           (obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER)) {
>                 fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
>                         "_CPC package element %d (%s) was not a buffer "
>                         "or an integer.", element, message);
> @@ -3235,19 +3237,19 @@ static void method_test_GCP_return(
>  {
>         FWTS_UNUSED(private);
>
> -       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -               if (obj->Integer.Value & ~0xf) {
> -                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -                               "Method_GCPReturn",
> -                               "%s returned %" PRId64 ", should be between 0 and 15, "
> -                               "one or more of the reserved bits 4..31 seem "
> -                               "to be set.",
> -                               name, obj->Integer.Value);
> -                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -               } else {
> -                       method_passed_sane_uint64(fw, name, obj->Integer.Value);
> -               }
> -       }
> +       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +               return;
> +
> +       if (obj->Integer.Value & ~0xf) {
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                       "Method_GCPReturn",
> +                       "%s returned %" PRId64 ", should be between 0 and 15, "
> +                       "one or more of the reserved bits 4..31 seem "
> +                       "to be set.",
> +                       name, obj->Integer.Value);
> +               fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +       } else
> +               method_passed_sane_uint64(fw, name, obj->Integer.Value);
>  }
>
>  static int method_test_GCP(fwts_framework *fw)
> @@ -3300,19 +3302,19 @@ static void method_test_GWS_return(
>  {
>         FWTS_UNUSED(private);
>
> -       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -               if (obj->Integer.Value & ~0x3) {
> -                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -                               "Method_GWSReturn",
> -                               "%s returned %" PRIu64 ", should be between 0 and 3, "
> -                               "one or more of the reserved bits 2..31 seem "
> -                               "to be set.",
> -                               name, obj->Integer.Value);
> -                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -               } else {
> -                       method_passed_sane_uint64(fw, name, obj->Integer.Value);
> -               }
> -       }
> +       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +               return;
> +
> +       if (obj->Integer.Value & ~0x3) {
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +                       "Method_GWSReturn",
> +                       "%s returned %" PRIu64 ", should be between 0 and 3, "
> +                       "one or more of the reserved bits 2..31 seem "
> +                       "to be set.",
> +                       name, obj->Integer.Value);
> +               fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +       } else
> +               method_passed_sane_uint64(fw, name, obj->Integer.Value);
>  }
>
>  static int method_test_GWS(fwts_framework *fw)
> @@ -3395,25 +3397,26 @@ static void method_test_SBS_return(
>
>         FWTS_UNUSED(private);
>
> -       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -               switch (obj->Integer.Value) {
> -               case 0 ... 4:
> -                       fwts_passed(fw, "%s correctly returned value %" PRIu64 " %s",
> -                               name, obj->Integer.Value,
> -                               sbs_info[obj->Integer.Value]);
> -                       break;
> -               default:
> -                       fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_SBSReturn",
> -                               "%s returned %" PRIu64 ", should be between 0 and 4.",
> -                               name, obj->Integer.Value);
> -                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -                       fwts_advice(fw,
> -                               "Smart Battery %s is incorrectly informing "
> -                               "the OS about the smart battery "
> -                               "configuration. This is a bug and needs to be "
> -                               "fixed.", name);
> -                       break;
> -               }
> +       if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +               return;
> +
> +       switch (obj->Integer.Value) {
> +       case 0 ... 4:
> +               fwts_passed(fw, "%s correctly returned value %" PRIu64 " %s",
> +                       name, obj->Integer.Value,
> +                       sbs_info[obj->Integer.Value]);
> +               break;
> +       default:
> +               fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_SBSReturn",
> +                       "%s returned %" PRIu64 ", should be between 0 and 4.",
> +                       name, obj->Integer.Value);
> +               fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +               fwts_advice(fw,
> +                       "Smart Battery %s is incorrectly informing "
> +                       "the OS about the smart battery "
> +                       "configuration. This is a bug and needs to be "
> +                       "fixed.", name);
> +               break;
>         }
>  }
>
> --
> 1.8.0
>
Acked-by: Keng-Yu Lin <kengyu@canonical.com>
Alex Hung - Jan. 31, 2013, 8:26 a.m.
On 01/10/2013 07:52 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> This patch tidies up some of the tests by checking for failures
> early on and bailing out of the test early.  This means we can
> remove one level of if statement nesting which makes the code
> less convoluted.
>
> Also remove some unwanted extra whitespaces.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/acpi/method/method.c | 159 ++++++++++++++++++++++++-----------------------
>   1 file changed, 81 insertions(+), 78 deletions(-)
>
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index baf9c36..21c89c0 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -1937,25 +1937,26 @@ static void method_test_STA_return(
>
>   	FWTS_UNUSED(private);
>
> -	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -		if ((obj->Integer.Value & 3) == 2) {
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -				"Method_STAEnabledNotPresent",
> -				"%s indicates that the device is enabled "
> -				"but not present, which is impossible.", name);
> -			failed = true;
> -		}
> -		if ((obj->Integer.Value & ~0x1f) != 0) {
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -				"Method_STAReservedBitsSet",
> -				"%s is returning non-zero reserved "
> -				"bits 5-31. These should be zero.", name);
> -			failed = true;
> -		}
> +	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +		return;
>
> -		if (!failed)
> -			method_passed_sane_uint64(fw, name, obj->Integer.Value);
> +	if ((obj->Integer.Value & 3) == 2) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"Method_STAEnabledNotPresent",
> +			"%s indicates that the device is enabled "
> +			"but not present, which is impossible.", name);
> +		failed = true;
> +	}
> +	if ((obj->Integer.Value & ~0x1f) != 0) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"Method_STAReservedBitsSet",
> +			"%s is returning non-zero reserved "
> +			"bits 5-31. These should be zero.", name);
> +		failed = true;
>   	}
> +
> +	if (!failed)
> +		method_passed_sane_uint64(fw, name, obj->Integer.Value);
>   }
>
>   static int method_test_STA(fwts_framework *fw)
> @@ -2011,18 +2012,19 @@ static void method_test_SEG_return(
>   {
>   	FWTS_UNUSED(private);
>
> -	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -		if ((obj->Integer.Value & 0xffff0000)) {
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -				"Method_SEGIllegalReserved",
> -				"%s returned value 0x%8.8" PRIx64 " and some of the "
> -				"upper 16 reserved bits are set when they "
> -				"should in fact be zero.",
> -				name, obj->Integer.Value);
> -			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -		} else
> -			method_passed_sane_uint64(fw, name, obj->Integer.Value);
> -	}
> +	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +		return;
> +
> +	if ((obj->Integer.Value & 0xffff0000)) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"Method_SEGIllegalReserved",
> +			"%s returned value 0x%8.8" PRIx64 " and some of the "
> +			"upper 16 reserved bits are set when they "
> +			"should in fact be zero.",
> +			name, obj->Integer.Value);
> +		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +	} else
> +		method_passed_sane_uint64(fw, name, obj->Integer.Value);
>   }
>
>   static int method_test_SEG(fwts_framework *fw)
> @@ -2308,7 +2310,7 @@ static void method_test_type_integer(
>   	int element,
>   	char *message)
>   {
> -	if (obj->Package.Elements[element].Type  != ACPI_TYPE_INTEGER) {
> +	if (obj->Package.Elements[element].Type != ACPI_TYPE_INTEGER) {
>   		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
>   			"_CPC package element %d (%s) was not an integer.",
>   			element, message);
> @@ -2323,7 +2325,7 @@ static void method_test_type_buffer(
>   	int element,
>   	char *message)
>   {
> -	if (obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER) {
> +	if (obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER) {
>   		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
>   			"_CPC package element %d (%s) was not a buffer.",
>   			element, message);
> @@ -2338,8 +2340,8 @@ static void method_test_type_mixed(
>   	int element,
>   	char *message)
>   {
> -	if ((obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER) &&
> -	    (obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER)) {
> +	if ((obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER) &&
> +	    (obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER)) {
>   		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
>   			"_CPC package element %d (%s) was not a buffer "
>   			"or an integer.", element, message);
> @@ -3235,19 +3237,19 @@ static void method_test_GCP_return(
>   {
>   	FWTS_UNUSED(private);
>
> -	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -		if (obj->Integer.Value & ~0xf) {
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -				"Method_GCPReturn",
> -				"%s returned %" PRId64 ", should be between 0 and 15, "
> -				"one or more of the reserved bits 4..31 seem "
> -				"to be set.",
> -				name, obj->Integer.Value);
> -			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -		} else {
> -			method_passed_sane_uint64(fw, name, obj->Integer.Value);
> -		}
> -	}
> +	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +		return;
> +
> +	if (obj->Integer.Value & ~0xf) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"Method_GCPReturn",
> +			"%s returned %" PRId64 ", should be between 0 and 15, "
> +			"one or more of the reserved bits 4..31 seem "
> +			"to be set.",
> +			name, obj->Integer.Value);
> +		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +	} else
> +		method_passed_sane_uint64(fw, name, obj->Integer.Value);
>   }
>
>   static int method_test_GCP(fwts_framework *fw)
> @@ -3300,19 +3302,19 @@ static void method_test_GWS_return(
>   {
>   	FWTS_UNUSED(private);
>
> -	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -		if (obj->Integer.Value & ~0x3) {
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -				"Method_GWSReturn",
> -				"%s returned %" PRIu64 ", should be between 0 and 3, "
> -				"one or more of the reserved bits 2..31 seem "
> -				"to be set.",
> -				name, obj->Integer.Value);
> -			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -		} else {
> -			method_passed_sane_uint64(fw, name, obj->Integer.Value);
> -		}
> -	}
> +	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +		return;
> +
> +	if (obj->Integer.Value & ~0x3) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			"Method_GWSReturn",
> +			"%s returned %" PRIu64 ", should be between 0 and 3, "
> +			"one or more of the reserved bits 2..31 seem "
> +			"to be set.",
> +			name, obj->Integer.Value);
> +		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +	} else
> +		method_passed_sane_uint64(fw, name, obj->Integer.Value);
>   }
>
>   static int method_test_GWS(fwts_framework *fw)
> @@ -3395,25 +3397,26 @@ static void method_test_SBS_return(
>
>   	FWTS_UNUSED(private);
>
> -	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
> -		switch (obj->Integer.Value) {
> -		case 0 ... 4:
> -			fwts_passed(fw, "%s correctly returned value %" PRIu64 " %s",
> -				name, obj->Integer.Value,
> -				sbs_info[obj->Integer.Value]);
> -			break;
> -		default:
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_SBSReturn",
> -				"%s returned %" PRIu64 ", should be between 0 and 4.",
> -				name, obj->Integer.Value);
> -			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -			fwts_advice(fw,
> -				"Smart Battery %s is incorrectly informing "
> -				"the OS about the smart battery "
> -				"configuration. This is a bug and needs to be "
> -				"fixed.", name);
> -			break;
> -		}
> +	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
> +		return;
> +
> +	switch (obj->Integer.Value) {
> +	case 0 ... 4:
> +		fwts_passed(fw, "%s correctly returned value %" PRIu64 " %s",
> +			name, obj->Integer.Value,
> +			sbs_info[obj->Integer.Value]);
> +		break;
> +	default:
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_SBSReturn",
> +			"%s returned %" PRIu64 ", should be between 0 and 4.",
> +			name, obj->Integer.Value);
> +		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +		fwts_advice(fw,
> +			"Smart Battery %s is incorrectly informing "
> +			"the OS about the smart battery "
> +			"configuration. This is a bug and needs to be "
> +			"fixed.", name);
> +		break;
>   	}
>   }
>
>
Acked-by: Alex Hung <alex.hung@canonical.com>

Patch

diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
index baf9c36..21c89c0 100644
--- a/src/acpi/method/method.c
+++ b/src/acpi/method/method.c
@@ -1937,25 +1937,26 @@  static void method_test_STA_return(
 
 	FWTS_UNUSED(private);
 
-	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
-		if ((obj->Integer.Value & 3) == 2) {
-			fwts_failed(fw, LOG_LEVEL_MEDIUM,
-				"Method_STAEnabledNotPresent",
-				"%s indicates that the device is enabled "
-				"but not present, which is impossible.", name);
-			failed = true;
-		}
-		if ((obj->Integer.Value & ~0x1f) != 0) {
-			fwts_failed(fw, LOG_LEVEL_MEDIUM,
-				"Method_STAReservedBitsSet",
-				"%s is returning non-zero reserved "
-				"bits 5-31. These should be zero.", name);
-			failed = true;
-		}
+	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
+		return;
 
-		if (!failed)
-			method_passed_sane_uint64(fw, name, obj->Integer.Value);
+	if ((obj->Integer.Value & 3) == 2) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"Method_STAEnabledNotPresent",
+			"%s indicates that the device is enabled "
+			"but not present, which is impossible.", name);
+		failed = true;
+	}
+	if ((obj->Integer.Value & ~0x1f) != 0) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"Method_STAReservedBitsSet",
+			"%s is returning non-zero reserved "
+			"bits 5-31. These should be zero.", name);
+		failed = true;
 	}
+
+	if (!failed)
+		method_passed_sane_uint64(fw, name, obj->Integer.Value);
 }
 
 static int method_test_STA(fwts_framework *fw)
@@ -2011,18 +2012,19 @@  static void method_test_SEG_return(
 {
 	FWTS_UNUSED(private);
 
-	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
-		if ((obj->Integer.Value & 0xffff0000)) {
-			fwts_failed(fw, LOG_LEVEL_MEDIUM,
-				"Method_SEGIllegalReserved",
-				"%s returned value 0x%8.8" PRIx64 " and some of the "
-				"upper 16 reserved bits are set when they "
-				"should in fact be zero.",
-				name, obj->Integer.Value);
-			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
-		} else
-			method_passed_sane_uint64(fw, name, obj->Integer.Value);
-	}
+	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
+		return;
+
+	if ((obj->Integer.Value & 0xffff0000)) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"Method_SEGIllegalReserved",
+			"%s returned value 0x%8.8" PRIx64 " and some of the "
+			"upper 16 reserved bits are set when they "
+			"should in fact be zero.",
+			name, obj->Integer.Value);
+		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
+	} else
+		method_passed_sane_uint64(fw, name, obj->Integer.Value);
 }
 
 static int method_test_SEG(fwts_framework *fw)
@@ -2308,7 +2310,7 @@  static void method_test_type_integer(
 	int element,
 	char *message)
 {
-	if (obj->Package.Elements[element].Type  != ACPI_TYPE_INTEGER) {
+	if (obj->Package.Elements[element].Type != ACPI_TYPE_INTEGER) {
 		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
 			"_CPC package element %d (%s) was not an integer.",
 			element, message);
@@ -2323,7 +2325,7 @@  static void method_test_type_buffer(
 	int element,
 	char *message)
 {
-	if (obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER) {
+	if (obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER) {
 		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
 			"_CPC package element %d (%s) was not a buffer.",
 			element, message);
@@ -2338,8 +2340,8 @@  static void method_test_type_mixed(
 	int element,
 	char *message)
 {
-	if ((obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER) &&
-	    (obj->Package.Elements[element].Type  != ACPI_TYPE_BUFFER)) {
+	if ((obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER) &&
+	    (obj->Package.Elements[element].Type != ACPI_TYPE_BUFFER)) {
 		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_CPCElementType",
 			"_CPC package element %d (%s) was not a buffer "
 			"or an integer.", element, message);
@@ -3235,19 +3237,19 @@  static void method_test_GCP_return(
 {
 	FWTS_UNUSED(private);
 
-	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
-		if (obj->Integer.Value & ~0xf) {
-			fwts_failed(fw, LOG_LEVEL_MEDIUM,
-				"Method_GCPReturn",
-				"%s returned %" PRId64 ", should be between 0 and 15, "
-				"one or more of the reserved bits 4..31 seem "
-				"to be set.",
-				name, obj->Integer.Value);
-			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
-		} else {
-			method_passed_sane_uint64(fw, name, obj->Integer.Value);
-		}
-	}
+	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
+		return;
+
+	if (obj->Integer.Value & ~0xf) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"Method_GCPReturn",
+			"%s returned %" PRId64 ", should be between 0 and 15, "
+			"one or more of the reserved bits 4..31 seem "
+			"to be set.",
+			name, obj->Integer.Value);
+		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
+	} else
+		method_passed_sane_uint64(fw, name, obj->Integer.Value);
 }
 
 static int method_test_GCP(fwts_framework *fw)
@@ -3300,19 +3302,19 @@  static void method_test_GWS_return(
 {
 	FWTS_UNUSED(private);
 
-	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
-		if (obj->Integer.Value & ~0x3) {
-			fwts_failed(fw, LOG_LEVEL_MEDIUM,
-				"Method_GWSReturn",
-				"%s returned %" PRIu64 ", should be between 0 and 3, "
-				"one or more of the reserved bits 2..31 seem "
-				"to be set.",
-				name, obj->Integer.Value);
-			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
-		} else {
-			method_passed_sane_uint64(fw, name, obj->Integer.Value);
-		}
-	}
+	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
+		return;
+
+	if (obj->Integer.Value & ~0x3) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM,
+			"Method_GWSReturn",
+			"%s returned %" PRIu64 ", should be between 0 and 3, "
+			"one or more of the reserved bits 2..31 seem "
+			"to be set.",
+			name, obj->Integer.Value);
+		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
+	} else
+		method_passed_sane_uint64(fw, name, obj->Integer.Value);
 }
 
 static int method_test_GWS(fwts_framework *fw)
@@ -3395,25 +3397,26 @@  static void method_test_SBS_return(
 
 	FWTS_UNUSED(private);
 
-	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
-		switch (obj->Integer.Value) {
-		case 0 ... 4:
-			fwts_passed(fw, "%s correctly returned value %" PRIu64 " %s",
-				name, obj->Integer.Value,
-				sbs_info[obj->Integer.Value]);
-			break;
-		default:
-			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_SBSReturn",
-				"%s returned %" PRIu64 ", should be between 0 and 4.",
-				name, obj->Integer.Value);
-			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
-			fwts_advice(fw,
-				"Smart Battery %s is incorrectly informing "
-				"the OS about the smart battery "
-				"configuration. This is a bug and needs to be "
-				"fixed.", name);
-			break;
-		}
+	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) != FWTS_OK)
+		return;
+
+	switch (obj->Integer.Value) {
+	case 0 ... 4:
+		fwts_passed(fw, "%s correctly returned value %" PRIu64 " %s",
+			name, obj->Integer.Value,
+			sbs_info[obj->Integer.Value]);
+		break;
+	default:
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_SBSReturn",
+			"%s returned %" PRIu64 ", should be between 0 and 4.",
+			name, obj->Integer.Value);
+		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
+		fwts_advice(fw,
+			"Smart Battery %s is incorrectly informing "
+			"the OS about the smart battery "
+			"configuration. This is a bug and needs to be "
+			"fixed.", name);
+		break;
 	}
 }