diff mbox series

[RFC] acpi/arm64: ignore 5.1 FADTs that are reported as 5.0

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

Commit Message

Ard Biesheuvel June 19, 2019, 12:18 p.m. UTC
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

Comments

Sudeep Holla June 19, 2019, 12:24 p.m. UTC | #1
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
Lee Jones June 19, 2019, 12:26 p.m. UTC | #2
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
Graeme Gregory June 20, 2019, 7:53 a.m. UTC | #3
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

>
Lee Jones June 20, 2019, 7:57 a.m. UTC | #4
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
Lorenzo Pieralisi June 20, 2019, 9:43 a.m. UTC | #5
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

>
Lorenzo Pieralisi June 20, 2019, 9:49 a.m. UTC | #6
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
Lee Jones June 20, 2019, 12:25 p.m. UTC | #7
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
Hanjun Guo June 24, 2019, 6:39 a.m. UTC | #8
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
Catalin Marinas June 24, 2019, 2:55 p.m. UTC | #9
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 mbox series

Patch

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)) {