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