[PATCH]arm64: ignore FACS load failed for arm64

[Alex Hung]

I saw Al had a discussion on awsg mailing list on this topic. Will it be 
better if we wait until his ECR merge to ACPI5.next and re-visit this topic?

On 14-10-21 06:01 AM, Al Stone wrote:
> On 10/20/2014 04:03 AM, Colin Ian King wrote:
>> On 20/10/14 09:37, Fu Wei wrote:
>>> arm64: ignore load failed for FACS which is involved in old
>>> BIOS style suspend/hibernate and does not make sense on arm64.
>>>
>>> Signed-off-by: Fu Wei <fu.wei at linaro.org>
>>> ---
>>>   src/lib/src/fwts_acpi_tables.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
>>> index 56498e0..0641d93 100644
>>> --- a/src/lib/src/fwts_acpi_tables.c
>>> +++ b/src/lib/src/fwts_acpi_tables.c
>>> @@ -375,7 +375,12 @@ static int fwts_acpi_handle_fadt(
>>>   	    "FACS", "FIRMWARE_CTRL", "X_FIRMWARE_CTRL",
>>>   	     &fadt->firmware_control, &fadt->x_firmware_ctrl,
>>>   	     provenance) != FWTS_OK) {
>>> +#if defined(__aarch64__)
>>> +		fwts_log_warning(fw, "Failed to load FACS: Cannot determine "
>>> +				"address of FACS from FADT. IGNORE for AArch64 platform!");
>>> +#else
>>>   		return FWTS_ERROR;
>>> +#endif
>>>   	}
>>>   	/* Determine DSDT addr and load it */
>>>   	if (fwts_acpi_handle_fadt_tables(fw, fadt,
>>>
>> I've re-read the relevant sections of the ACPI 5.1 specification and I
>> can't see any mention of this table being mandatory or not; so a failure
>> on it not existing may be wrong for any architecture.
>>
>> Does the Fixed ACPI Description Table Fixed Feature Flags
>> HW_REDUCED_ACPI bit being set to 1 imply that the FACS not existing is
>> OK?  (See ACPI 5.1, able 5-35, bit 20).
> Ick.  The spec is ambiguous.  I could not find anything that explicitly
> said FACS is mandatory or optional.  HW reduced mode does not affect
> the *existence* of the FACS table, though; both the FIRMWARE_CTRL and
> X_FIRMWARE_CTRL fields are allowed and usable in HW reduced (top of
> section 5.2.9) and at least one of them must be non-zero (per table
> 5-34).  So, that would imply there must be an FACS, but it is not explicit.
>
> But, there's also things like the Global Lock in the FACS must not be
> used in HW reduced mode.  And, the FACS fields for the waking vector
> probably don't even make sense on ARMv8.  But, the Hardware Signature
> might make sense if a TPM is in use.
>
> So I guess -- based on what I'm seeing in the spec -- I would still
> leave this as an error if neither FIRMWARE_CTRL or X_FIRMWARE_CTRL is
> set.  This seems to be a legitimate (if not weird) bug in the tables
> as currently defined.
>
> I'll try to put together an ECR to clarify this in the spec and make
> the table optional.
>
>
>> If that is a reasonable assumption, maybe that would be a better way of
>> checking. E.g.
>>
>> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
>> index 56498e0..97a7bd4 100644
>> --- a/src/lib/src/fwts_acpi_tables.c
>> +++ b/src/lib/src/fwts_acpi_tables.c
>> @@ -311,6 +311,9 @@ static int fwts_acpi_handle_fadt_tables(
>>                  }
>>                  /* Is it sane? */
>>                  if (addr == 0) {
>> +                       /* HW_REDUCED - address can be zero */
>> +                       if (fadt->flags & (1 << 20))
>> +                               return FWTS_OK;
>>                          fwts_log_error(fw, "Failed to load %s: Cannot
>> determine "
>>                                  "address of %s from FADT, fields %s and
>> %s are zero.",
>>                                  name, name, name_addr32, name_addr64);
>> @@ -320,6 +323,9 @@ static int fwts_acpi_handle_fadt_tables(
>>                  addr = (off_t)*addr32;
>>                  /* Is it sane? */
>>                  if (addr == 0)  {
>> +                       /* HW_REDUCED - address can be zero */
>> +                       if (fadt->flags & (1 << 20))
>> +                               return FWTS_OK;
>>                          fwts_log_error(fw, "Failed to load %s: Cannot
>> determine "
>>                                  "address of %s from FADT, field %s is
>> zero.",
>>                                  name, name, name_addr32);
>>
>>
>> Colin
>>
>