| Message ID | 1517427808-8678-1-git-send-email-jhugo@codeaurora.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [V2] acpi: fadt: Fix FACS checking | expand |
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@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@canonical.com>
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@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@canonical.com>
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."); } }
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@codeaurora.org> --- src/acpi/fadt/fadt.c | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-)