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

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

On Wednesday, 25 of June 2008, Zhang Rui wrote:
> 
> On Tue, 2008-06-24 at 18:18 +0800, 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.
> Yes, Shaohua generated the prototype patches a couple of years ago.
> But this is slightly different from the original one, e.g. not touching
> ACPI DATA memory, saving ACPI NVS memory after _PTS and restore it
> before _WAK strictly, etc.
> We generate this patch set and hope it be helpful for some hibernate
> specific issues.
> 
> > > +
> > > +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).
> Pages for ACPI NVS must be allocated before swsusp_shrink_memory.

Saying "must" in this context is way too strong IMO. :-)

First, you can allocate memory after swsusp_shrink_memory(), although you
can't allocate too much memory, where how much is "too much" really depends on
the requested size of the image.  It certanly cannot be more than the I/O and
spare pages reserve which is 5 MB at present.  I don't think there's so much
NVS memory to save in any case.

Still, if you really want to avoid the memory problem, it's better to change
the ordering between swsusp_shrink_memory() and platform_begin() in
kernel/power/disk.c:hibernation_snapshot() than to add a new notifier.

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