diff mbox series

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

Message ID 20240312134148.727454-2-dwmw2@infradead.org (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [1/2] ACPICA: Detect FACS even for hardware reduced platforms | expand

Commit Message

David Woodhouse March 12, 2024, 1:41 p.m. UTC 
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(+)

Comments

David Woodhouse April 2, 2024, 9:29 a.m. UTC | #1 
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);
Sudeep Holla April 2, 2024, 10:29 a.m. UTC | #2 
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
Mediouni, Mohamed April 2, 2024, 12:17 p.m. UTC | #3 
> 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
Sudeep Holla April 2, 2024, 2:12 p.m. UTC | #4 
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
diff mbox series

Patch

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