[PATCH] acpi: fadt: Fix FACS checking

[Jeffrey Hugo]

On 1/29/2018 11:00 AM, Alex Hung wrote:
> 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?
> 

Depends on your preference I guess.

I personally like the early return approach because I think breaks up 
the function a little when reading.  Less for me to juggle around in my 
head.  Particularly because all the checks don't necessarily fit into 
one screen, so its hard to see everything at once.

However, I do see your perspective.  Overall having a series of if/else 
statements does closely represent what the function is actually doing.

I'll code it up as if/else and we can see which approach you'd prefer to 
take.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.