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, Rafael J. Wysocki wrote:
> 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;
> > +};

One more thing: I'd slightly prefer to use a list.h list instead of the open
coded one here.  It won't cost us much, so IMO it's worth doing for consistency
with the rest of the kernel.

> > +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.

Ah, sorry, scratch that.  I misread the code.

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