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
