On Tuesday, 24 of June 2008, Zhang Rui wrote:
> According to the ACPI spec, ACPI NVS memory region is required to be
> saved/restored by OS during hibernation.
>
> Section 15.3.2 ACPI Spec 3.0b,
> "OSPM will call the _PTS control method some time before entering a sleeping state,
> to allow the platform’s AML code to update this memory image before entering the
> sleeping state. After the system awakes from an S4 state, OSPM will restore this memory
> area and call the _WAK control method to enable the BIOS to reclaim its memory image."
>
> Add the mechanism to save/restore ACPI NVS memory during hibernation.
>
> Note: now Linux save ACPI NVS memory in acpi_hibernation_pre_snapshot, and restore it in
> acpi_hibernation_leave. Both of these functions will be invoked only once during
> the hibernate and resume.
> Note: in Section 14.3 ACPI spec 3.0b, I only get
> "EfiACPIMemoryNVS: The OS and loader must preserve this memory range in
> the working and ACPI S1–S3 states."
> whether we should save/restore this piece of memory is not cleared.
>
> Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
Well, I know what the specification says, but we tried that in the past and it
caused regressions.
> ---
> drivers/acpi/sleep/main.c | 122 +++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/acpi.h | 3 +
> 2 files changed, 124 insertions(+), 1 deletion(-)
>
> Index: linux-2.6/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/sleep/main.c 2008-06-20 09:27:00.000000000 +0800
> +++ linux-2.6/drivers/acpi/sleep/main.c 2008-06-24 14:51:20.000000000 +0800
> @@ -15,6 +15,7 @@
> #include <linux/dmi.h>
> #include <linux/device.h>
> #include <linux/suspend.h>
> +#include <linux/highmem.h>
>
> #include <asm/io.h>
>
> @@ -255,6 +256,109 @@
> #endif /* CONFIG_SUSPEND */
>
> #ifdef CONFIG_HIBERNATION
> +
> +/* ACPI NVS memory need to be saved/stored during hibernation */
> +struct nvs_page {
> + unsigned long pfn;
> + void *data;
> + struct nvs_page *next;
> +};
> +static struct nvs_page *nvs_pages;
> +
> +int acpi_mark_nvs_region(unsigned long start, unsigned long end)
> +{
> + struct nvs_page *tmp;
> +
> + while (start <= end) {
> + tmp = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
We should free the already allocated structures in this case IMO.
> + tmp->pfn = start;
> + tmp->next = nvs_pages;
> + start++;
> + nvs_pages = tmp;
> + }
> + return 0;
> +}
> +
> +static int acpi_hibernation_allocate_nvs_pages(void)
> +{
> + struct nvs_page *tmp = nvs_pages;
> +
> + while (tmp) {
> + tmp->data = (void *)__get_free_page(GFP_KERNEL);
> + if (!tmp->data)
> + return -ENOMEM;
Perhaps call acpi_hibernation_free_nvs_pages() here too.
> + tmp = tmp->next;
> + }
> + return 0;
> +}
> +
> +static int acpi_hibernation_free_nvs_pages(void)
> +{
> + struct nvs_page *tmp = nvs_pages;
> +
> + while (tmp) {
> + if (tmp->data) {
> + free_page((long)tmp->data);
> + tmp->data = NULL;
> + }
> + tmp = tmp->next;
> + }
Make nvs_pages = NULL explicitly, please.
> + return 0;
> +}
> +
> +static int
> +acpi_hibernation_notifier_cb(struct notifier_block *nb,
> + unsigned long val, void *data)
> +{
> + int result = 0;
> +
> + switch (val) {
> + case PM_HIBERNATION_PREPARE:
> + result = acpi_hibernation_allocate_nvs_pages();
> + break;
> + case PM_POST_RESTORE: /* Failed */
> + case PM_POST_HIBERNATION:
> + acpi_hibernation_free_nvs_pages();
> + }
> + return result;
> +}
> +
> +struct notifier_block acpi_hibernation_nb = {
> + .notifier_call = acpi_hibernation_notifier_cb,
> +};
This is over the top, IMO. Please put the appropriate call into
acpi_hibernation_begin() and create acpi_hibernation_end() with the call to
acpi_hibernation_free_nvs_pages() (it must do what acpi_pm_end() does apart
from this).
> +
> +static void acpi_save_nvs_memory(void)
> +{
> + void *kaddr;
> + struct nvs_page *tmp = nvs_pages;
> +
> + pr_debug("ACPI: Saving ACPI NVS memory\n");
> + while (tmp) {
> + /* nvs page might not have a 'struct page' */
> + kaddr = kmap_atomic_pfn(tmp->pfn, KM_USER0);
> + copy_page(tmp->data, kaddr);
> + kunmap_atomic(kaddr, KM_USER0);
> + tmp = tmp->next;
> + }
> + return;
> +}
> +
> +static void acpi_restore_nvs_memory(void)
> +{
> + void *kaddr;
> + struct nvs_page *tmp = nvs_pages;
> +
> + pr_debug("ACPI: Restoring ACPI NVS memory\n");
> + while (tmp) {
> + kaddr = kmap_atomic_pfn(tmp->pfn, KM_USER0);
> + copy_page(kaddr, tmp->data);
> + kunmap_atomic(kaddr, KM_USER0);
> + tmp = tmp->next;
> + }
> +}
> +
> static int acpi_hibernation_begin(void)
> {
> acpi_target_sleep_state = ACPI_STATE_S4;
> @@ -274,6 +378,20 @@
> return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
> }
>
> +static int acpi_hibernation_pre_snapshot(void)
> +{
> + int error = acpi_sleep_prepare(ACPI_STATE_S4);
> +
> + if (error) {
> + acpi_target_sleep_state = ACPI_STATE_S0;
> + return error;
> + }
> +
> + acpi_save_nvs_memory();
> +
> + return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
> +}
> +
> static int acpi_hibernation_enter(void)
> {
> acpi_status status = AE_OK;
> @@ -301,6 +419,7 @@
> acpi_enable();
> /* Reprogram control registers and execute _BFS */
> acpi_leave_sleep_state_prep(ACPI_STATE_S4);
> + acpi_restore_nvs_memory();
> }
>
> static void acpi_hibernation_finish(void)
> @@ -340,7 +459,7 @@
> static struct platform_hibernation_ops acpi_hibernation_ops = {
> .begin = acpi_hibernation_begin,
> .end = acpi_hibernation_end,
> - .pre_snapshot = acpi_hibernation_prepare,
> + .pre_snapshot = acpi_hibernation_pre_snapshot,
> .finish = acpi_hibernation_finish,
> .prepare = acpi_hibernation_prepare,
> .enter = acpi_hibernation_enter,
> @@ -498,6 +617,7 @@
> #ifdef CONFIG_HIBERNATION
> status = acpi_get_sleep_type_data(ACPI_STATE_S4, &type_a, &type_b);
> if (ACPI_SUCCESS(status)) {
> + register_pm_notifier(&acpi_hibernation_nb);
> hibernation_set_ops(&acpi_hibernation_ops);
> sleep_states[ACPI_STATE_S4] = 1;
> printk(" S4");
> Index: linux-2.6/include/linux/acpi.h
> ===================================================================
> --- linux-2.6.orig/include/linux/acpi.h 2008-06-20 09:27:00.000000000 +0800
> +++ linux-2.6/include/linux/acpi.h 2008-06-24 11:11:03.000000000 +0800
> @@ -106,6 +106,9 @@
> void acpi_numa_arch_fixup(void);
> #endif
>
> +#ifdef CONFIG_HIBERNATION
> +int acpi_mark_nvs_region(unsigned long, unsigned long);
> +#endif
> #ifdef CONFIG_ACPI_HOTPLUG_CPU
> /* Arch dependent functions for cpu hotplug support */
> int acpi_map_lsapic(acpi_handle handle, int *pcpu);
The rest looks good to me.
Thanks,
Rafael
--
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
