Re: [RFC] [PATCH 1/3] ACPI: save/restore ACPI NVS memory during hibernation

["Rafael J. Wysocki" <rjw@sisk.pl>]

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@intel.com>

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html