Re: [PATCH v2 05/18] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bob,

On 2014-8-20 6:55, Moore, Robert wrote:
> I should warn you that FADT version numbers are notoriously unreliable;
> In fact, in ACPICA we were eventually forced to abandon them entirely. 
> We use the actual size of the FADT instead.

Yes, I heard that story, thanks for the reminding. But I also see that the
revision number is used on x86 and ia64 now in parsing FADT, it is a firmware
bug not to comply with the spec, and on ARM, only ACPI 5.1 or higher can be
used in Linux, Major.Minor revision was introduced in ACPI 5.1 and it was a
major change for it, so I think firmware should comply with that, if firmware
just copy some code from somewhere else and leave the revision number
unchanged, we will not boot (disable ACPI).

The size of FADT for 5.0 and 5.1 is no difference, it is pretty hard to use
that to identify the version of FADT, did I  miss something?

Thanks
Hanjun

> 
> Bob
> 
> 
> 
>> -----Original Message-----
>> From: Hanjun Guo [mailto:hanjun.guo@xxxxxxxxxx]
>> Sent: Tuesday, August 19, 2014 5:14 AM
>> To: Mark Rutland
>> Cc: Catalin Marinas; Rafael J. Wysocki; graeme.gregory@xxxxxxxxxx; Arnd
>> Bergmann; Olof Johansson; grant.likely@xxxxxxxxxx; Sudeep Holla; Will
>> Deacon; Jason Cooper; Marc Zyngier; Bjorn Helgaas; Daniel Lezcano; Mark
>> Brown; Rob Herring; Robert Richter; Zheng, Lv; Moore, Robert; Lorenzo
>> Pieralisi; Liviu Dudau; Randy Dunlap; Charles Garcia-Tobin; linux-
>> acpi@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; linaro-acpi@xxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v2 05/18] ARM64 / ACPI: Parse FADT table to get PSCI
>> flags for PSCI init
>>
>> On 2014-8-19 19:10, Mark Rutland wrote:
>>>>>> @@ -47,6 +49,26 @@ void __init __acpi_unmap_table(char *map, unsigned
>> long size)
>>>>>>  	early_memunmap(map, size);
>>>>>>  }
>>>>>>
>>>>>> +static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>>>>> +{
>>>>>> +	struct acpi_table_fadt *fadt = (struct acpi_table_fadt
>> *)table;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Revision in table header is the FADT Major version,
>>>>>> +	 * and there is a minor version of FADT which was introduced
>>>>>> +	 * by ACPI 5.1, we only deal with ACPI 5.1 or higher version
>>>>>> +	 * to get arm boot flags, or we will disable ACPI.
>>>>>> +	 */
>>>>>> +	if (table->revision < 5 || fadt->minor_revision < 1) {
>>>>>
>>>>> If we ever get revision 6.0, this would trigger.
>>>>
>>>> Yes, good catch, actually I already fixed that in my local git repo,
>>>>
>>>> +       if (table->revision > 5 ||
>>>> +           (table->revision == 5 && fadt->minor_revision >= 1)) {
>>>> +               return 0;
>>>> +       } else {
>>>> +               pr_info("FADT revision is %d.%d, no PSCI support,
>>>> + should be 5.1
>>>> or higher\n",
>>>> +                       table->revision, fadt->minor_revision);
>>>> +               disable_acpi();
>>>> +               return -EINVAL;
>>>> +       }
>>>
>>> Given you return in the first path, you don't need the remaining code
>>> to live in an else block.
>>
>> Agreed, I will update it, and move disable_acpi() outside this function
>> and keep it in one place as Sudeep suggested.
>>
>> Thanks
>> Hanjun
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux