[PATCH] acpi: fadt: Fix FACS checking

[Alex Hung]

On 2018-01-25 07:04 AM, Jeffrey Hugo wrote:
> In case of HW reduced platforms, FIRMWARE_CTRL and X_FIRMWARE_CTRL are
> allowed to be 0 in the FADT as a FACS may not be provided.  The FACS
> validation which checks these fields does register a success for HW reduced
> platforms when the fields are both 0, or registers a failure for non-HW
> reduced platforms as FACS is required in that case.  However, the FACS
> validation continues with other checks, which can override the previous
> success with a failure, causing an invalid test result and confusing output
> messages.
> 
> We can do better by returning early after validating FIRMWARE_CTRL and
> X_FIRMWARE_CTRL are both 0 and the platform is or is not HW reduced as we
> have made a definitive pass/fail determination, and further checking is not
> warranted.
> 
> Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
> ---
>   src/acpi/fadt/fadt.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c
> index 9863566..d18d28c 100644
> --- a/src/acpi/fadt/fadt.c
> +++ b/src/acpi/fadt/fadt.c
> @@ -270,7 +270,8 @@ static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw)
>   				    "Structure (FACS) when ACPI hardware "
>   				    "reduced mode is not set. ");
>   		}
> -
> +		/* We have either passed or failed.  No need to continue */
> +		return;
>   	}
>   
>   	if ((fadt->firmware_control != 0 && fadt->x_firmware_ctrl == 0) ||
> 


While this patch is valid, I am also thinking whether it would be better 
to change the if statements to if-else if-else if-else as below

if (fadt->firmware_control == 0 && fadt->x_firmware_ctrl == 0) {
...
} else if (fadt->firmware_control != 0 && fadt->x_firmware_ctrl != 0) {
... // because this needs to be moved and returns too
} else if ((fadt->firmware_control != 0 && fadt->x_firmware_ctrl == 0) 
|| (fadt->firmware_control == 0 && fadt->x_firmware_ctrl != 0)) {
... // because this needs to be move the last
} else {
...
}

Any comments?