ACK: [PATCH V2] acpi: fadt: Fix FACS checking

[Colin Ian King]

On 31/01/18 19:43, 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 reorganizing the checks to follow in logical succession,
> only applying pass/fail results appropiate to the specific scenario (both
> fields zero, both fields non-zero, one field zero with the other non-zero).
> 
> Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
> ---
>  src/acpi/fadt/fadt.c | 27 +++++----------------------
>  1 file changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c
> index 9863566..93a5862 100644
> --- a/src/acpi/fadt/fadt.c
> +++ b/src/acpi/fadt/fadt.c
> @@ -271,28 +271,7 @@ static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw)
>  				    "reduced mode is not set. ");
>  		}
>  
> -	}
> -
> -	if ((fadt->firmware_control != 0 && fadt->x_firmware_ctrl == 0) ||
> -	    (fadt->firmware_control == 0 && fadt->x_firmware_ctrl != 0)) {
> -		fwts_passed(fw,
> -			    "Only one of FIRMWARE_CTRL and X_FIRMWARE_CTRL "
> -			    "is non-zero.");
> -	} else {
> -		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> -			    "FADTFACSBothSet",
> -			    "Both 32-bit FIRMWARE_CTRL and 64-bit "
> -			    "X_FIRMWARE_CTRL pointers to the FACS are "
> -			    "set but only one is allowed.");
> -		fwts_advice(fw,
> -			    "Having both FACS pointers set is ambiguous; "
> -			    "there is no way to determine which one is "
> -			    "the correct address.  The kernel workaround "
> -			    "is to always use the 64-bit X_FIRMWARE_CTRL.");
> -	}
> -
> -
> -	if (fadt->firmware_control != 0 && fadt->x_firmware_ctrl != 0) {
> +	} else 	if (fadt->firmware_control != 0 && fadt->x_firmware_ctrl != 0) {
>  		if ((uint64_t)fadt->firmware_control == fadt->x_firmware_ctrl) {
>  			fwts_passed(fw,
>  				    "Both FIRMWARE_CTRL and X_FIRMWARE_CTRL "
> @@ -326,6 +305,10 @@ static void acpi_table_check_fadt_firmware_ctrl(fwts_framework *fw)
>  				    "64-bit X_FIRMWARE_CTRL pointer to the "
>  				    "FACS. ");
>  		}
> +	} else { /* only one of firmware_control/x_firmware_ctrl is 0 */
> +		fwts_passed(fw,
> +			    "Only one of FIRMWARE_CTRL and X_FIRMWARE_CTRL "
> +			    "is non-zero.");
>  	}
>  }
>  
> 
Looks good to me, thanks Jeffrey.

Acked-by: Colin Ian King <colin.king at canonical.com>