On 2018-01-31 11:43 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 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.");
> }
> }
>
>
Acked-by: Alex Hung <alex.hung at canonical.com>