[PATCH 1/2] ACPICA: Detect FACS even for hardware reduced platforms

[PATCH 1/2] ACPICA: Detect FACS even for hardware reduced platforms

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

ACPICA PR https://github.com/acpica/acpica/pull/933

The FACS is optional even on hardware reduced platforms, and may exist
for the purpose of communicating the hardware_signature field to provke
a clean reboot instead of a resume from hibernation.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 drivers/acpi/acpica/tbfadt.c  | 30 +++++++++++++-----------------
 drivers/acpi/acpica/tbutils.c |  7 +------
 2 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/acpica/tbfadt.c b/drivers/acpi/acpica/tbfadt.c
index 44267a92bce5..3c126c6d306b 100644
--- a/drivers/acpi/acpica/tbfadt.c
+++ b/drivers/acpi/acpica/tbfadt.c
@@ -315,23 +315,19 @@ void acpi_tb_parse_fadt(void)
 				       ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL,
 				       NULL, FALSE, TRUE, &acpi_gbl_dsdt_index);
 
-	/* If Hardware Reduced flag is set, there is no FACS */
-
-	if (!acpi_gbl_reduced_hardware) {
-		if (acpi_gbl_FADT.facs) {
-			acpi_tb_install_standard_table((acpi_physical_address)
-						       acpi_gbl_FADT.facs,
-						       ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL,
-						       NULL, FALSE, TRUE,
-						       &acpi_gbl_facs_index);
-		}
-		if (acpi_gbl_FADT.Xfacs) {
-			acpi_tb_install_standard_table((acpi_physical_address)
-						       acpi_gbl_FADT.Xfacs,
-						       ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL,
-						       NULL, FALSE, TRUE,
-						       &acpi_gbl_xfacs_index);
-		}
+	if (acpi_gbl_FADT.facs) {
+		acpi_tb_install_standard_table((acpi_physical_address)
+					       acpi_gbl_FADT.facs,
+					       ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL,
+					       NULL, FALSE, TRUE,
+					       &acpi_gbl_facs_index);
+	}
+	if (acpi_gbl_FADT.Xfacs) {
+		acpi_tb_install_standard_table((acpi_physical_address)
+					       acpi_gbl_FADT.Xfacs,
+					       ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL,
+					       NULL, FALSE, TRUE,
+					       &acpi_gbl_xfacs_index);
 	}
 }
 
diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index bb4a56e5673a..15fa68a5ea6e 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -36,12 +36,7 @@ acpi_status acpi_tb_initialize_facs(void)
 {
 	struct acpi_table_facs *facs;
 
-	/* If Hardware Reduced flag is set, there is no FACS */
-
-	if (acpi_gbl_reduced_hardware) {
-		acpi_gbl_FACS = NULL;
-		return (AE_OK);
-	} else if (acpi_gbl_FADT.Xfacs &&
+	if (acpi_gbl_FADT.Xfacs &&
 		   (!acpi_gbl_FADT.facs
 		    || !acpi_gbl_use32_bit_facs_addresses)) {
 		(void)acpi_get_table_by_index(acpi_gbl_xfacs_index,
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] arm64: acpi: Honour firmware_signature field of FACS, if it exists

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

If the firmware_signature changes then OSPM should not attempt to resume
from hibernate, but should instead perform a clean reboot. Set the global
swsusp_hardware_signature to allow the generic code to include the value
in the swsusp header on disk, and perform the appropriate check on resume.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/arm64/kernel/acpi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index dba8fcec7f33..e0e7b93c16cc 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -26,6 +26,7 @@
 #include <linux/libfdt.h>
 #include <linux/smp.h>
 #include <linux/serial_core.h>
+#include <linux/suspend.h>
 #include <linux/pgtable.h>
 
 #include <acpi/ghes.h>
@@ -227,6 +228,15 @@ void __init acpi_boot_table_init(void)
 		if (earlycon_acpi_spcr_enable)
 			early_init_dt_scan_chosen_stdout();
 	} else {
+#ifdef CONFIG_HIBERNATION
+		struct acpi_table_header *facs = NULL;
+		acpi_get_table(ACPI_SIG_FACS, 1, &facs);
+		if (facs) {
+			swsusp_hardware_signature =
+				((struct acpi_table_facs *)facs)->hardware_signature;
+			acpi_put_table(facs);
+		}
+#endif
 		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
 		if (IS_ENABLED(CONFIG_ACPI_BGRT))
 			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: acpi: Honour firmware_signature field of FACS, if it exists

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1968 bytes --]

On Tue, 2024-03-12 at 13:41 +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> If the firmware_signature changes then OSPM should not attempt to resume
> from hibernate, but should instead perform a clean reboot. Set the global
> swsusp_hardware_signature to allow the generic code to include the value
> in the swsusp header on disk, and perform the appropriate check on resume.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Ping?

> ---
>  arch/arm64/kernel/acpi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index dba8fcec7f33..e0e7b93c16cc 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -26,6 +26,7 @@
>  #include <linux/libfdt.h>
>  #include <linux/smp.h>
>  #include <linux/serial_core.h>
> +#include <linux/suspend.h>
>  #include <linux/pgtable.h>
>  
>  #include <acpi/ghes.h>
> @@ -227,6 +228,15 @@ void __init acpi_boot_table_init(void)
>                 if (earlycon_acpi_spcr_enable)
>                         early_init_dt_scan_chosen_stdout();
>         } else {
> +#ifdef CONFIG_HIBERNATION
> +               struct acpi_table_header *facs = NULL;
> +               acpi_get_table(ACPI_SIG_FACS, 1, &facs);
> +               if (facs) {
> +                       swsusp_hardware_signature =
> +                               ((struct acpi_table_facs *)facs)->hardware_signature;
> +                       acpi_put_table(facs);
> +               }
> +#endif
>                 acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
>                 if (IS_ENABLED(CONFIG_ACPI_BGRT))
>                         acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: acpi: Honour firmware_signature field of FACS, if it exists

[Sudeep Holla]

On Tue, Apr 02, 2024 at 10:29:57AM +0100, David Woodhouse wrote:
> On Tue, 2024-03-12 at 13:41 +0000, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > If the firmware_signature changes then OSPM should not attempt to resume
> > from hibernate, but should instead perform a clean reboot. Set the global
> > swsusp_hardware_signature to allow the generic code to include the value
> > in the swsusp header on disk, and perform the appropriate check on resume.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> 
> Ping?
> 
> > ---
> >  arch/arm64/kernel/acpi.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index dba8fcec7f33..e0e7b93c16cc 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/libfdt.h>
> >  #include <linux/smp.h>
> >  #include <linux/serial_core.h>
> > +#include <linux/suspend.h>
> >  #include <linux/pgtable.h>
> >  
> >  #include <acpi/ghes.h>
> > @@ -227,6 +228,15 @@ void __init acpi_boot_table_init(void)
> >                 if (earlycon_acpi_spcr_enable)
> >                         early_init_dt_scan_chosen_stdout();
> >         } else {
> > +#ifdef CONFIG_HIBERNATION
> > +               struct acpi_table_header *facs = NULL;
> > +               acpi_get_table(ACPI_SIG_FACS, 1, &facs);
> > +               if (facs) {
> > +                       swsusp_hardware_signature =
> > +                               ((struct acpi_table_facs *)facs)->hardware_signature;
> > +                       acpi_put_table(facs);
> > +               }
> > +#endif

I think it is OK as a temporary solution for now. But there was some
investigation last year as part of some work in Linaro to enable
"drivers/acpi/sleep.c" into the build cleaning up some x86-ness in there.
acpi_sleep_hibernate_setup() already does this but enabling sleep.c need
some careful investigation so that it doesn't break any existing arm64/x86
platforms and made need some wordings clarification in the ACPI spec.
Today system suspend work via psci std path bypassing the ACPI paths which
may not be ideal as none of the ACPI methods are honoured. Some arm64
platforms may implement them and expect to be executed in the future,
maybe ?

So, until that happens, I see this as an possible alternative and
temporary solution.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: acpi: Honour firmware_signature field of FACS, if it exists

[Mediouni, Mohamed]

> On 2. Apr 2024, at 12:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> 
> On Tue, Apr 02, 2024 at 10:29:57AM +0100, David Woodhouse wrote:
>> On Tue, 2024-03-12 at 13:41 +0000, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>> 
>>> If the firmware_signature changes then OSPM should not attempt to resume
>>> from hibernate, but should instead perform a clean reboot. Set the global
>>> swsusp_hardware_signature to allow the generic code to include the value
>>> in the swsusp header on disk, and perform the appropriate check on resume.
>>> 
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> 
>> Ping?
>> 
>>> ---
>>> arch/arm64/kernel/acpi.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>> 
>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>>> index dba8fcec7f33..e0e7b93c16cc 100644
>>> --- a/arch/arm64/kernel/acpi.c
>>> +++ b/arch/arm64/kernel/acpi.c
>>> @@ -26,6 +26,7 @@
>>> #include <linux/libfdt.h>
>>> #include <linux/smp.h>
>>> #include <linux/serial_core.h>
>>> +#include <linux/suspend.h>
>>> #include <linux/pgtable.h>
>>> 
>>> #include <acpi/ghes.h>
>>> @@ -227,6 +228,15 @@ void __init acpi_boot_table_init(void)
>>>                if (earlycon_acpi_spcr_enable)
>>>                        early_init_dt_scan_chosen_stdout();
>>>        } else {
>>> +#ifdef CONFIG_HIBERNATION
>>> +               struct acpi_table_header *facs = NULL;
>>> +               acpi_get_table(ACPI_SIG_FACS, 1, &facs);
>>> +               if (facs) {
>>> +                       swsusp_hardware_signature =
>>> +                               ((struct acpi_table_facs *)facs)->hardware_signature;
>>> +                       acpi_put_table(facs);
>>> +               }
>>> +#endif
> 
> I think it is OK as a temporary solution for now. But there was some
> investigation last year as part of some work in Linaro to enable
> "drivers/acpi/sleep.c" into the build cleaning up some x86-ness in there.
> acpi_sleep_hibernate_setup() already does this but enabling sleep.c need
> some careful investigation so that it doesn't break any existing arm64/x86
> platforms and made need some wordings clarification in the ACPI spec.
> Today system suspend work via psci std path bypassing the ACPI paths which
> may not be ideal as none of the ACPI methods are honoured. Some arm64
> platforms may implement them and expect to be executed in the future,
> maybe ?
Current Windows on Arm platforms (seen on SC8280XP) don’t have _GTS
or _PTS methods, and don’t have sleeping objects either.

As such, I don’t expect any users for that potential functionality. Am I missing something 
or hibernation signalling to firmware (on ARM64) can be made PSCI only indefinitely?

Thank you,
-Mohamed
> So, until that happens, I see this as an possible alternative and
> temporary solution.
> 
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> --
> Regards,
> Sudeep




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: acpi: Honour firmware_signature field of FACS, if it exists

[Sudeep Holla]

On Tue, Apr 02, 2024 at 12:17:22PM +0000, Mediouni, Mohamed wrote:
> 
> > On 2. Apr 2024, at 12:29, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > 
> > I think it is OK as a temporary solution for now. But there was some
> > investigation last year as part of some work in Linaro to enable
> > "drivers/acpi/sleep.c" into the build cleaning up some x86-ness in there.
> > acpi_sleep_hibernate_setup() already does this but enabling sleep.c need
> > some careful investigation so that it doesn't break any existing arm64/x86
> > platforms and made need some wordings clarification in the ACPI spec.
> > Today system suspend work via psci std path bypassing the ACPI paths which
> > may not be ideal as none of the ACPI methods are honoured. Some arm64
> > platforms may implement them and expect to be executed in the future,
> > maybe ?
> Current Windows on Arm platforms (seen on SC8280XP) don’t have _GTS
> or _PTS methods, and don’t have sleeping objects either.
>

IMO, SC8280XP is not a very good model platform for ACPI firmware reference.
It uses PEP which Linux doesn't support for good reason and that make it
hard to follow everything on that platform.

> As such, I don’t expect any users for that potential functionality.

I am not 100% sure

> Am I missing something or hibernation signalling to firmware (on ARM64)
> can be made PSCI only indefinitely?

Also bypassing certain operation taken care in sleep.c might result in
missing certain features. Few things IIRC(might be missing things myself
or misunderstood as it has been a while since I looked at the code in
detail): handing of GPE for wakeup, power resource handling during the
resume, power button event to mention few.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel