| Message ID | 20190619121831.7614-1-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 2af22f3ec3ca452f1e79b967f634708ff01ced8a |
| Headers | show |
| Series | [RFC] acpi/arm64: ignore 5.1 FADTs that are reported as 5.0 | expand |
On Wed, Jun 19, 2019 at 02:18:31PM +0200, Ard Biesheuvel wrote: > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > are clearly ACPI 5.1 based, given that that is the first ACPI revision > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > which has a non-zero field on those systems. > > So in these cases, infer from the ARM boot flags that the FADT must be > 5.1 or later, and treat it as 5.1. > Makes sense and looks simple to me. Acked-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
On Wed, 19 Jun 2019, Ard Biesheuvel wrote: > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > are clearly ACPI 5.1 based, given that that is the first ACPI revision > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > which has a non-zero field on those systems. > > So in these cases, infer from the ARM boot flags that the FADT must be > 5.1 or later, and treat it as 5.1. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/acpi.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 803f0494dd3e..7722e85fb69c 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -155,10 +155,14 @@ static int __init acpi_fadt_sanity_check(void) > */ > if (table->revision < 5 || > (table->revision == 5 && fadt->minor_revision < 1)) { > - pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", > + pr_err(FW_BUG "Unsupported FADT revision %d.%d, should be 5.1+\n", > table->revision, fadt->minor_revision); > - ret = -EINVAL; > - goto out; > + > + if (!fadt->arm_boot_flags) { > + ret = -EINVAL; > + goto out; > + } > + pr_err("FADT has ARM boot flags set, assuming 5.1\n"); Tested-by: Lee Jones <lee.jones@linaro.org> -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 19, 2019 at 02:18:31PM +0200, Ard Biesheuvel wrote: > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > are clearly ACPI 5.1 based, given that that is the first ACPI revision > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > which has a non-zero field on those systems. > > So in these cases, infer from the ARM boot flags that the FADT must be > 5.1 or later, and treat it as 5.1. > Makes sense, I did actually see this in the wild in SBSA machine too once. Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/acpi.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 803f0494dd3e..7722e85fb69c 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -155,10 +155,14 @@ static int __init acpi_fadt_sanity_check(void) > */ > if (table->revision < 5 || > (table->revision == 5 && fadt->minor_revision < 1)) { > - pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", > + pr_err(FW_BUG "Unsupported FADT revision %d.%d, should be 5.1+\n", > table->revision, fadt->minor_revision); > - ret = -EINVAL; > - goto out; > + > + if (!fadt->arm_boot_flags) { > + ret = -EINVAL; > + goto out; > + } > + pr_err("FADT has ARM boot flags set, assuming 5.1\n"); > } > > if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) { > -- > 2.20.1 >
On Wed, 19 Jun 2019, Sudeep Holla wrote: > On Wed, Jun 19, 2019 at 02:18:31PM +0200, Ard Biesheuvel wrote: > > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > > are clearly ACPI 5.1 based, given that that is the first ACPI revision > > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > > which has a non-zero field on those systems. > > > > So in these cases, infer from the ARM boot flags that the FADT must be > > 5.1 or later, and treat it as 5.1. > > > > Makes sense and looks simple to me. > > Acked-by: Sudeep Holla <sudeep.holla@arm.com> Could we pleeeeease have this in for v5.3? We have available, consumer-level platforms that rely on this change. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 19, 2019 at 02:18:31PM +0200, Ard Biesheuvel wrote: > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > are clearly ACPI 5.1 based, given that that is the first ACPI revision > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > which has a non-zero field on those systems. > > So in these cases, infer from the ARM boot flags that the FADT must be > 5.1 or later, and treat it as 5.1. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/acpi.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) AFAICS this should be harmless, so: Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 803f0494dd3e..7722e85fb69c 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -155,10 +155,14 @@ static int __init acpi_fadt_sanity_check(void) > */ > if (table->revision < 5 || > (table->revision == 5 && fadt->minor_revision < 1)) { > - pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", > + pr_err(FW_BUG "Unsupported FADT revision %d.%d, should be 5.1+\n", > table->revision, fadt->minor_revision); > - ret = -EINVAL; > - goto out; > + > + if (!fadt->arm_boot_flags) { > + ret = -EINVAL; > + goto out; > + } > + pr_err("FADT has ARM boot flags set, assuming 5.1\n"); > } > > if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) { > -- > 2.20.1 >
On Thu, Jun 20, 2019 at 08:57:32AM +0100, Lee Jones wrote: > On Wed, 19 Jun 2019, Sudeep Holla wrote: > > > On Wed, Jun 19, 2019 at 02:18:31PM +0200, Ard Biesheuvel wrote: > > > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > > > are clearly ACPI 5.1 based, given that that is the first ACPI revision > > > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > > > which has a non-zero field on those systems. > > > > > > So in these cases, infer from the ARM boot flags that the FADT must be > > > 5.1 or later, and treat it as 5.1. > > > > > > > Makes sense and looks simple to me. > > > > Acked-by: Sudeep Holla <sudeep.holla@arm.com> > > Could we pleeeeease have this in for v5.3? > > We have available, consumer-level platforms that rely on this change. But we do not have the kernel infrastructure to support them so I am fine with it but urgency is questionable as far as I am concerned. Lorenzo
On Thu, 20 Jun 2019, Lorenzo Pieralisi wrote: > On Thu, Jun 20, 2019 at 08:57:32AM +0100, Lee Jones wrote: > > On Wed, 19 Jun 2019, Sudeep Holla wrote: > > > > > On Wed, Jun 19, 2019 at 02:18:31PM +0200, Ard Biesheuvel wrote: > > > > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > > > > are clearly ACPI 5.1 based, given that that is the first ACPI revision > > > > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > > > > which has a non-zero field on those systems. > > > > > > > > So in these cases, infer from the ARM boot flags that the FADT must be > > > > 5.1 or later, and treat it as 5.1. > > > > > > > > > > Makes sense and looks simple to me. > > > > > > Acked-by: Sudeep Holla <sudeep.holla@arm.com> > > > > Could we pleeeeease have this in for v5.3? > > > > We have available, consumer-level platforms that rely on this change. > > But we do not have the kernel infrastructure to support them so > I am fine with it but urgency is questionable as far as I am > concerned. Yes we do. At least, we will in v5.3. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On 2019/6/19 20:18, Ard Biesheuvel wrote: > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > are clearly ACPI 5.1 based, given that that is the first ACPI revision > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > which has a non-zero field on those systems. > > So in these cases, infer from the ARM boot flags that the FADT must be > 5.1 or later, and treat it as 5.1. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/acpi.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 803f0494dd3e..7722e85fb69c 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -155,10 +155,14 @@ static int __init acpi_fadt_sanity_check(void) > */ > if (table->revision < 5 || > (table->revision == 5 && fadt->minor_revision < 1)) { > - pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", > + pr_err(FW_BUG "Unsupported FADT revision %d.%d, should be 5.1+\n", > table->revision, fadt->minor_revision); > - ret = -EINVAL; > - goto out; > + > + if (!fadt->arm_boot_flags) { > + ret = -EINVAL; > + goto out; > + } > + pr_err("FADT has ARM boot flags set, assuming 5.1\n"); Obviously it's a firmware bug, but should be harmless, Acked-by: Hanjun Guo <guohanjun@huawei.com> Thanks Hanjun
On Wed, Jun 19, 2019 at 02:18:31PM +0200, Ard Biesheuvel wrote: > Some Qualcomm Snapdragon based laptops built to run Microsoft Windows > are clearly ACPI 5.1 based, given that that is the first ACPI revision > that supports ARM, and introduced the FADT 'arm_boot_flags' field, > which has a non-zero field on those systems. > > So in these cases, infer from the ARM boot flags that the FADT must be > 5.1 or later, and treat it as 5.1. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Queued for 5.3. Thanks. -- Catalin
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 803f0494dd3e..7722e85fb69c 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -155,10 +155,14 @@ static int __init acpi_fadt_sanity_check(void) */ if (table->revision < 5 || (table->revision == 5 && fadt->minor_revision < 1)) { - pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", + pr_err(FW_BUG "Unsupported FADT revision %d.%d, should be 5.1+\n", table->revision, fadt->minor_revision); - ret = -EINVAL; - goto out; + + if (!fadt->arm_boot_flags) { + ret = -EINVAL; + goto out; + } + pr_err("FADT has ARM boot flags set, assuming 5.1\n"); } if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) {
Some Qualcomm Snapdragon based laptops built to run Microsoft Windows are clearly ACPI 5.1 based, given that that is the first ACPI revision that supports ARM, and introduced the FADT 'arm_boot_flags' field, which has a non-zero field on those systems. So in these cases, infer from the ARM boot flags that the FADT must be 5.1 or later, and treat it as 5.1. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/acpi.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) -- 2.20.1