[PATCH] Store BIOS NVS area over s2ram [V2]

[PATCH] Store BIOS NVS area over s2ram [V2]

[Maxim Levitsky]

Hi,

This is rebased and slightly updated version of Matthew Garrett's
patches.
Tested on my system.

Note that now I got positive feedback from 2 users of this system that
this approach works, and no negative feedback.

Please review.

Best regards,
Maxim Levitsky

[PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Maxim Levitsky]

Saving platform non-volatile state may be required for suspend to RAM as
well as hibernation. Move it to more generic code.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Matthew Garrett <maximlevitsky@gmail.com>
---
 arch/x86/kernel/e820.c       |    2 +-
 drivers/acpi/sleep.c         |   12 ++--
 include/linux/suspend.h      |   26 ++++----
 kernel/power/Kconfig         |    9 ++-
 kernel/power/Makefile        |    2 +-
 kernel/power/hibernate_nvs.c |  136 ------------------------------------------
 kernel/power/nvs.c           |  136 ++++++++++++++++++++++++++++++++++++++++++
 kernel/power/suspend.c       |    6 ++
 8 files changed, 168 insertions(+), 161 deletions(-)
 delete mode 100644 kernel/power/hibernate_nvs.c
 create mode 100644 kernel/power/nvs.c

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 7bca3c6..0d6fc71 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -729,7 +729,7 @@ static int __init e820_mark_nvs_memory(void)
 		struct e820entry *ei = &e820.map[i];
 
 		if (ei->type == E820_NVS)
-			hibernate_nvs_register(ei->addr, ei->size);
+			suspend_nvs_register(ei->addr, ei->size);
 	}
 
 	return 0;
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 3fb4bde..bdb306f 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -404,7 +404,7 @@ static int acpi_hibernation_begin(void)
 {
 	int error;
 
-	error = s4_no_nvs ? 0 : hibernate_nvs_alloc();
+	error = s4_no_nvs ? 0 : suspend_nvs_alloc();
 	if (!error) {
 		acpi_target_sleep_state = ACPI_STATE_S4;
 		acpi_sleep_tts_switch(acpi_target_sleep_state);
@@ -418,7 +418,7 @@ static int acpi_hibernation_pre_snapshot(void)
 	int error = acpi_pm_prepare();
 
 	if (!error)
-		hibernate_nvs_save();
+		suspend_nvs_save();
 
 	return error;
 }
@@ -443,7 +443,7 @@ static int acpi_hibernation_enter(void)
 
 static void acpi_hibernation_finish(void)
 {
-	hibernate_nvs_free();
+	suspend_nvs_free();
 	acpi_ec_unblock_transactions();
 	acpi_pm_finish();
 }
@@ -464,7 +464,7 @@ static void acpi_hibernation_leave(void)
 		panic("ACPI S4 hardware signature mismatch");
 	}
 	/* Restore the NVS memory area */
-	hibernate_nvs_restore();
+	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */
 	acpi_ec_unblock_transactions_early();
 }
@@ -507,7 +507,7 @@ static int acpi_hibernation_begin_old(void)
 
 	if (!error) {
 		if (!s4_no_nvs)
-			error = hibernate_nvs_alloc();
+			error = suspend_nvs_alloc();
 		if (!error)
 			acpi_target_sleep_state = ACPI_STATE_S4;
 	}
@@ -517,7 +517,7 @@ static int acpi_hibernation_begin_old(void)
 static int acpi_hibernation_pre_snapshot_old(void)
 {
 	acpi_pm_freeze();
-	hibernate_nvs_save();
+	suspend_nvs_save();
 	return 0;
 }
 
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 5e781d8..bc7d6bb 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -256,22 +256,22 @@ static inline int hibernate(void) { return -ENOSYS; }
 static inline bool system_entering_hibernation(void) { return false; }
 #endif /* CONFIG_HIBERNATION */
 
-#ifdef CONFIG_HIBERNATION_NVS
-extern int hibernate_nvs_register(unsigned long start, unsigned long size);
-extern int hibernate_nvs_alloc(void);
-extern void hibernate_nvs_free(void);
-extern void hibernate_nvs_save(void);
-extern void hibernate_nvs_restore(void);
-#else /* CONFIG_HIBERNATION_NVS */
-static inline int hibernate_nvs_register(unsigned long a, unsigned long b)
+#ifdef CONFIG_SUSPEND_NVS
+extern int suspend_nvs_register(unsigned long start, unsigned long size);
+extern int suspend_nvs_alloc(void);
+extern void suspend_nvs_free(void);
+extern void suspend_nvs_save(void);
+extern void suspend_nvs_restore(void);
+#else /* CONFIG_SUSPEND_NVS */
+static inline int suspend_nvs_register(unsigned long a, unsigned long b)
 {
 	return 0;
 }
-static inline int hibernate_nvs_alloc(void) { return 0; }
-static inline void hibernate_nvs_free(void) {}
-static inline void hibernate_nvs_save(void) {}
-static inline void hibernate_nvs_restore(void) {}
-#endif /* CONFIG_HIBERNATION_NVS */
+static inline int suspend_nvs_alloc(void) { return 0; }
+static inline void suspend_nvs_free(void) {}
+static inline void suspend_nvs_save(void) {}
+static inline void suspend_nvs_restore(void) {}
+#endif /* CONFIG_SUSPEND_NVS */
 
 #ifdef CONFIG_PM_SLEEP
 void save_processor_state(void);
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 5c36ea9..ca6066a 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -99,9 +99,13 @@ config PM_SLEEP_ADVANCED_DEBUG
 	depends on PM_ADVANCED_DEBUG
 	default n
 
+config SUSPEND_NVS
+       bool
+
 config SUSPEND
 	bool "Suspend to RAM and standby"
 	depends on PM && ARCH_SUSPEND_POSSIBLE
+	select SUSPEND_NVS if HAS_IOMEM
 	default y
 	---help---
 	  Allow the system to enter sleep states in which main memory is
@@ -130,13 +134,10 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
-config HIBERNATION_NVS
-	bool
-
 config HIBERNATION
 	bool "Hibernation (aka 'suspend to disk')"
 	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
-	select HIBERNATION_NVS if HAS_IOMEM
+	select SUSPEND_NVS if HAS_IOMEM
 	---help---
 	  Enable the suspend to disk (STD) functionality, which is usually
 	  called "hibernation" in user interfaces.  STD checkpoints the
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 524e058..f9063c6 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -10,6 +10,6 @@ obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o \
 				   block_io.o
-obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
+obj-$(CONFIG_SUSPEND_NVS)	+= nvs.o
 
 obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
diff --git a/kernel/power/hibernate_nvs.c b/kernel/power/hibernate_nvs.c
deleted file mode 100644
index fdcad9e..0000000
--- a/kernel/power/hibernate_nvs.c
+++ /dev/null
@@ -1,136 +0,0 @@
-/*
- * linux/kernel/power/hibernate_nvs.c - Routines for handling NVS memory
- *
- * Copyright (C) 2008,2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
- *
- * This file is released under the GPLv2.
- */
-
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/list.h>
-#include <linux/mm.h>
-#include <linux/slab.h>
-#include <linux/suspend.h>
-
-/*
- * Platforms, like ACPI, may want us to save some memory used by them during
- * hibernation and to restore the contents of this memory during the subsequent
- * resume.  The code below implements a mechanism allowing us to do that.
- */
-
-struct nvs_page {
-	unsigned long phys_start;
-	unsigned int size;
-	void *kaddr;
-	void *data;
-	struct list_head node;
-};
-
-static LIST_HEAD(nvs_list);
-
-/**
- *	hibernate_nvs_register - register platform NVS memory region to save
- *	@start - physical address of the region
- *	@size - size of the region
- *
- *	The NVS region need not be page-aligned (both ends) and we arrange
- *	things so that the data from page-aligned addresses in this region will
- *	be copied into separate RAM pages.
- */
-int hibernate_nvs_register(unsigned long start, unsigned long size)
-{
-	struct nvs_page *entry, *next;
-
-	while (size > 0) {
-		unsigned int nr_bytes;
-
-		entry = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
-		if (!entry)
-			goto Error;
-
-		list_add_tail(&entry->node, &nvs_list);
-		entry->phys_start = start;
-		nr_bytes = PAGE_SIZE - (start & ~PAGE_MASK);
-		entry->size = (size < nr_bytes) ? size : nr_bytes;
-
-		start += entry->size;
-		size -= entry->size;
-	}
-	return 0;
-
- Error:
-	list_for_each_entry_safe(entry, next, &nvs_list, node) {
-		list_del(&entry->node);
-		kfree(entry);
-	}
-	return -ENOMEM;
-}
-
-/**
- *	hibernate_nvs_free - free data pages allocated for saving NVS regions
- */
-void hibernate_nvs_free(void)
-{
-	struct nvs_page *entry;
-
-	list_for_each_entry(entry, &nvs_list, node)
-		if (entry->data) {
-			free_page((unsigned long)entry->data);
-			entry->data = NULL;
-			if (entry->kaddr) {
-				iounmap(entry->kaddr);
-				entry->kaddr = NULL;
-			}
-		}
-}
-
-/**
- *	hibernate_nvs_alloc - allocate memory necessary for saving NVS regions
- */
-int hibernate_nvs_alloc(void)
-{
-	struct nvs_page *entry;
-
-	list_for_each_entry(entry, &nvs_list, node) {
-		entry->data = (void *)__get_free_page(GFP_KERNEL);
-		if (!entry->data) {
-			hibernate_nvs_free();
-			return -ENOMEM;
-		}
-	}
-	return 0;
-}
-
-/**
- *	hibernate_nvs_save - save NVS memory regions
- */
-void hibernate_nvs_save(void)
-{
-	struct nvs_page *entry;
-
-	printk(KERN_INFO "PM: Saving platform NVS memory\n");
-
-	list_for_each_entry(entry, &nvs_list, node)
-		if (entry->data) {
-			entry->kaddr = ioremap(entry->phys_start, entry->size);
-			memcpy(entry->data, entry->kaddr, entry->size);
-		}
-}
-
-/**
- *	hibernate_nvs_restore - restore NVS memory regions
- *
- *	This function is going to be called with interrupts disabled, so it
- *	cannot iounmap the virtual addresses used to access the NVS region.
- */
-void hibernate_nvs_restore(void)
-{
-	struct nvs_page *entry;
-
-	printk(KERN_INFO "PM: Restoring platform NVS memory\n");
-
-	list_for_each_entry(entry, &nvs_list, node)
-		if (entry->data)
-			memcpy(entry->kaddr, entry->data, entry->size);
-}
diff --git a/kernel/power/nvs.c b/kernel/power/nvs.c
new file mode 100644
index 0000000..30c662f
--- /dev/null
+++ b/kernel/power/nvs.c
@@ -0,0 +1,136 @@
+/*
+ * linux/kernel/power/nvs.c - Routines for handling NVS memory
+ *
+ * Copyright (C) 2008,2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+
+/*
+ * Platforms, like ACPI, may want us to save some memory used by them during
+ * suspend and to restore the contents of this memory during the subsequent
+ * resume.  The code below implements a mechanism allowing us to do that.
+ */
+
+struct nvs_page {
+	unsigned long phys_start;
+	unsigned int size;
+	void *kaddr;
+	void *data;
+	struct list_head node;
+};
+
+static LIST_HEAD(nvs_list);
+
+/**
+ *	suspend_nvs_register - register platform NVS memory region to save
+ *	@start - physical address of the region
+ *	@size - size of the region
+ *
+ *	The NVS region need not be page-aligned (both ends) and we arrange
+ *	things so that the data from page-aligned addresses in this region will
+ *	be copied into separate RAM pages.
+ */
+int suspend_nvs_register(unsigned long start, unsigned long size)
+{
+	struct nvs_page *entry, *next;
+
+	while (size > 0) {
+		unsigned int nr_bytes;
+
+		entry = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
+		if (!entry)
+			goto Error;
+
+		list_add_tail(&entry->node, &nvs_list);
+		entry->phys_start = start;
+		nr_bytes = PAGE_SIZE - (start & ~PAGE_MASK);
+		entry->size = (size < nr_bytes) ? size : nr_bytes;
+
+		start += entry->size;
+		size -= entry->size;
+	}
+	return 0;
+
+ Error:
+	list_for_each_entry_safe(entry, next, &nvs_list, node) {
+		list_del(&entry->node);
+		kfree(entry);
+	}
+	return -ENOMEM;
+}
+
+/**
+ *	suspend_nvs_free - free data pages allocated for saving NVS regions
+ */
+void suspend_nvs_free(void)
+{
+	struct nvs_page *entry;
+
+	list_for_each_entry(entry, &nvs_list, node)
+		if (entry->data) {
+			free_page((unsigned long)entry->data);
+			entry->data = NULL;
+			if (entry->kaddr) {
+				iounmap(entry->kaddr);
+				entry->kaddr = NULL;
+			}
+		}
+}
+
+/**
+ *	suspend_nvs_alloc - allocate memory necessary for saving NVS regions
+ */
+int suspend_nvs_alloc(void)
+{
+	struct nvs_page *entry;
+
+	list_for_each_entry(entry, &nvs_list, node) {
+		entry->data = (void *)__get_free_page(GFP_KERNEL);
+		if (!entry->data) {
+			suspend_nvs_free();
+			return -ENOMEM;
+		}
+	}
+	return 0;
+}
+
+/**
+ *	suspend_nvs_save - save NVS memory regions
+ */
+void suspend_nvs_save(void)
+{
+	struct nvs_page *entry;
+
+	printk(KERN_INFO "PM: Saving platform NVS memory\n");
+
+	list_for_each_entry(entry, &nvs_list, node)
+		if (entry->data) {
+			entry->kaddr = ioremap(entry->phys_start, entry->size);
+			memcpy(entry->data, entry->kaddr, entry->size);
+		}
+}
+
+/**
+ *	suspend_nvs_restore - restore NVS memory regions
+ *
+ *	This function is going to be called with interrupts disabled, so it
+ *	cannot iounmap the virtual addresses used to access the NVS region.
+ */
+void suspend_nvs_restore(void)
+{
+	struct nvs_page *entry;
+
+	printk(KERN_INFO "PM: Restoring platform NVS memory\n");
+
+	list_for_each_entry(entry, &nvs_list, node)
+		if (entry->data)
+			memcpy(entry->kaddr, entry->data, entry->size);
+}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 56e7dbb..f37cb7d 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -16,6 +16,12 @@
 #include <linux/cpu.h>
 #include <linux/syscalls.h>
 #include <linux/gfp.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
 
 #include "power.h"
 
-- 
1.7.0.4

Re: [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Rafael J. Wysocki]

On Tuesday 01 June 2010, Maxim Levitsky wrote:
> Saving platform non-volatile state may be required for suspend to RAM as
> well as hibernation. Move it to more generic code.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> Signed-off-by: Matthew Garrett <maximlevitsky@gmail.com>

You made a mistake here.

Also, why are you resending the Matthews patches?  I think Len has seen them
already.

Rafael


> ---
>  arch/x86/kernel/e820.c       |    2 +-
>  drivers/acpi/sleep.c         |   12 ++--
>  include/linux/suspend.h      |   26 ++++----
>  kernel/power/Kconfig         |    9 ++-
>  kernel/power/Makefile        |    2 +-
>  kernel/power/hibernate_nvs.c |  136 ------------------------------------------
>  kernel/power/nvs.c           |  136 ++++++++++++++++++++++++++++++++++++++++++
>  kernel/power/suspend.c       |    6 ++
>  8 files changed, 168 insertions(+), 161 deletions(-)
>  delete mode 100644 kernel/power/hibernate_nvs.c
>  create mode 100644 kernel/power/nvs.c
> 
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 7bca3c6..0d6fc71 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -729,7 +729,7 @@ static int __init e820_mark_nvs_memory(void)
>  		struct e820entry *ei = &e820.map[i];
>  
>  		if (ei->type == E820_NVS)
> -			hibernate_nvs_register(ei->addr, ei->size);
> +			suspend_nvs_register(ei->addr, ei->size);
>  	}
>  
>  	return 0;
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 3fb4bde..bdb306f 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -404,7 +404,7 @@ static int acpi_hibernation_begin(void)
>  {
>  	int error;
>  
> -	error = s4_no_nvs ? 0 : hibernate_nvs_alloc();
> +	error = s4_no_nvs ? 0 : suspend_nvs_alloc();
>  	if (!error) {
>  		acpi_target_sleep_state = ACPI_STATE_S4;
>  		acpi_sleep_tts_switch(acpi_target_sleep_state);
> @@ -418,7 +418,7 @@ static int acpi_hibernation_pre_snapshot(void)
>  	int error = acpi_pm_prepare();
>  
>  	if (!error)
> -		hibernate_nvs_save();
> +		suspend_nvs_save();
>  
>  	return error;
>  }
> @@ -443,7 +443,7 @@ static int acpi_hibernation_enter(void)
>  
>  static void acpi_hibernation_finish(void)
>  {
> -	hibernate_nvs_free();
> +	suspend_nvs_free();
>  	acpi_ec_unblock_transactions();
>  	acpi_pm_finish();
>  }
> @@ -464,7 +464,7 @@ static void acpi_hibernation_leave(void)
>  		panic("ACPI S4 hardware signature mismatch");
>  	}
>  	/* Restore the NVS memory area */
> -	hibernate_nvs_restore();
> +	suspend_nvs_restore();
>  	/* Allow EC transactions to happen. */
>  	acpi_ec_unblock_transactions_early();
>  }
> @@ -507,7 +507,7 @@ static int acpi_hibernation_begin_old(void)
>  
>  	if (!error) {
>  		if (!s4_no_nvs)
> -			error = hibernate_nvs_alloc();
> +			error = suspend_nvs_alloc();
>  		if (!error)
>  			acpi_target_sleep_state = ACPI_STATE_S4;
>  	}
> @@ -517,7 +517,7 @@ static int acpi_hibernation_begin_old(void)
>  static int acpi_hibernation_pre_snapshot_old(void)
>  {
>  	acpi_pm_freeze();
> -	hibernate_nvs_save();
> +	suspend_nvs_save();
>  	return 0;
>  }
>  
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 5e781d8..bc7d6bb 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -256,22 +256,22 @@ static inline int hibernate(void) { return -ENOSYS; }
>  static inline bool system_entering_hibernation(void) { return false; }
>  #endif /* CONFIG_HIBERNATION */
>  
> -#ifdef CONFIG_HIBERNATION_NVS
> -extern int hibernate_nvs_register(unsigned long start, unsigned long size);
> -extern int hibernate_nvs_alloc(void);
> -extern void hibernate_nvs_free(void);
> -extern void hibernate_nvs_save(void);
> -extern void hibernate_nvs_restore(void);
> -#else /* CONFIG_HIBERNATION_NVS */
> -static inline int hibernate_nvs_register(unsigned long a, unsigned long b)
> +#ifdef CONFIG_SUSPEND_NVS
> +extern int suspend_nvs_register(unsigned long start, unsigned long size);
> +extern int suspend_nvs_alloc(void);
> +extern void suspend_nvs_free(void);
> +extern void suspend_nvs_save(void);
> +extern void suspend_nvs_restore(void);
> +#else /* CONFIG_SUSPEND_NVS */
> +static inline int suspend_nvs_register(unsigned long a, unsigned long b)
>  {
>  	return 0;
>  }
> -static inline int hibernate_nvs_alloc(void) { return 0; }
> -static inline void hibernate_nvs_free(void) {}
> -static inline void hibernate_nvs_save(void) {}
> -static inline void hibernate_nvs_restore(void) {}
> -#endif /* CONFIG_HIBERNATION_NVS */
> +static inline int suspend_nvs_alloc(void) { return 0; }
> +static inline void suspend_nvs_free(void) {}
> +static inline void suspend_nvs_save(void) {}
> +static inline void suspend_nvs_restore(void) {}
> +#endif /* CONFIG_SUSPEND_NVS */
>  
>  #ifdef CONFIG_PM_SLEEP
>  void save_processor_state(void);
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 5c36ea9..ca6066a 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -99,9 +99,13 @@ config PM_SLEEP_ADVANCED_DEBUG
>  	depends on PM_ADVANCED_DEBUG
>  	default n
>  
> +config SUSPEND_NVS
> +       bool
> +
>  config SUSPEND
>  	bool "Suspend to RAM and standby"
>  	depends on PM && ARCH_SUSPEND_POSSIBLE
> +	select SUSPEND_NVS if HAS_IOMEM
>  	default y
>  	---help---
>  	  Allow the system to enter sleep states in which main memory is
> @@ -130,13 +134,10 @@ config SUSPEND_FREEZER
>  
>  	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
>  
> -config HIBERNATION_NVS
> -	bool
> -
>  config HIBERNATION
>  	bool "Hibernation (aka 'suspend to disk')"
>  	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
> -	select HIBERNATION_NVS if HAS_IOMEM
> +	select SUSPEND_NVS if HAS_IOMEM
>  	---help---
>  	  Enable the suspend to disk (STD) functionality, which is usually
>  	  called "hibernation" in user interfaces.  STD checkpoints the
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index 524e058..f9063c6 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -10,6 +10,6 @@ obj-$(CONFIG_SUSPEND)		+= suspend.o
>  obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
>  obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o \
>  				   block_io.o
> -obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
> +obj-$(CONFIG_SUSPEND_NVS)	+= nvs.o
>  
>  obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
> diff --git a/kernel/power/hibernate_nvs.c b/kernel/power/hibernate_nvs.c
> deleted file mode 100644
> index fdcad9e..0000000
> --- a/kernel/power/hibernate_nvs.c
> +++ /dev/null
> @@ -1,136 +0,0 @@
> -/*
> - * linux/kernel/power/hibernate_nvs.c - Routines for handling NVS memory
> - *
> - * Copyright (C) 2008,2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
> - *
> - * This file is released under the GPLv2.
> - */
> -
> -#include <linux/io.h>
> -#include <linux/kernel.h>
> -#include <linux/list.h>
> -#include <linux/mm.h>
> -#include <linux/slab.h>
> -#include <linux/suspend.h>
> -
> -/*
> - * Platforms, like ACPI, may want us to save some memory used by them during
> - * hibernation and to restore the contents of this memory during the subsequent
> - * resume.  The code below implements a mechanism allowing us to do that.
> - */
> -
> -struct nvs_page {
> -	unsigned long phys_start;
> -	unsigned int size;
> -	void *kaddr;
> -	void *data;
> -	struct list_head node;
> -};
> -
> -static LIST_HEAD(nvs_list);
> -
> -/**
> - *	hibernate_nvs_register - register platform NVS memory region to save
> - *	@start - physical address of the region
> - *	@size - size of the region
> - *
> - *	The NVS region need not be page-aligned (both ends) and we arrange
> - *	things so that the data from page-aligned addresses in this region will
> - *	be copied into separate RAM pages.
> - */
> -int hibernate_nvs_register(unsigned long start, unsigned long size)
> -{
> -	struct nvs_page *entry, *next;
> -
> -	while (size > 0) {
> -		unsigned int nr_bytes;
> -
> -		entry = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
> -		if (!entry)
> -			goto Error;
> -
> -		list_add_tail(&entry->node, &nvs_list);
> -		entry->phys_start = start;
> -		nr_bytes = PAGE_SIZE - (start & ~PAGE_MASK);
> -		entry->size = (size < nr_bytes) ? size : nr_bytes;
> -
> -		start += entry->size;
> -		size -= entry->size;
> -	}
> -	return 0;
> -
> - Error:
> -	list_for_each_entry_safe(entry, next, &nvs_list, node) {
> -		list_del(&entry->node);
> -		kfree(entry);
> -	}
> -	return -ENOMEM;
> -}
> -
> -/**
> - *	hibernate_nvs_free - free data pages allocated for saving NVS regions
> - */
> -void hibernate_nvs_free(void)
> -{
> -	struct nvs_page *entry;
> -
> -	list_for_each_entry(entry, &nvs_list, node)
> -		if (entry->data) {
> -			free_page((unsigned long)entry->data);
> -			entry->data = NULL;
> -			if (entry->kaddr) {
> -				iounmap(entry->kaddr);
> -				entry->kaddr = NULL;
> -			}
> -		}
> -}
> -
> -/**
> - *	hibernate_nvs_alloc - allocate memory necessary for saving NVS regions
> - */
> -int hibernate_nvs_alloc(void)
> -{
> -	struct nvs_page *entry;
> -
> -	list_for_each_entry(entry, &nvs_list, node) {
> -		entry->data = (void *)__get_free_page(GFP_KERNEL);
> -		if (!entry->data) {
> -			hibernate_nvs_free();
> -			return -ENOMEM;
> -		}
> -	}
> -	return 0;
> -}
> -
> -/**
> - *	hibernate_nvs_save - save NVS memory regions
> - */
> -void hibernate_nvs_save(void)
> -{
> -	struct nvs_page *entry;
> -
> -	printk(KERN_INFO "PM: Saving platform NVS memory\n");
> -
> -	list_for_each_entry(entry, &nvs_list, node)
> -		if (entry->data) {
> -			entry->kaddr = ioremap(entry->phys_start, entry->size);
> -			memcpy(entry->data, entry->kaddr, entry->size);
> -		}
> -}
> -
> -/**
> - *	hibernate_nvs_restore - restore NVS memory regions
> - *
> - *	This function is going to be called with interrupts disabled, so it
> - *	cannot iounmap the virtual addresses used to access the NVS region.
> - */
> -void hibernate_nvs_restore(void)
> -{
> -	struct nvs_page *entry;
> -
> -	printk(KERN_INFO "PM: Restoring platform NVS memory\n");
> -
> -	list_for_each_entry(entry, &nvs_list, node)
> -		if (entry->data)
> -			memcpy(entry->kaddr, entry->data, entry->size);
> -}
> diff --git a/kernel/power/nvs.c b/kernel/power/nvs.c
> new file mode 100644
> index 0000000..30c662f
> --- /dev/null
> +++ b/kernel/power/nvs.c
> @@ -0,0 +1,136 @@
> +/*
> + * linux/kernel/power/nvs.c - Routines for handling NVS memory
> + *
> + * Copyright (C) 2008,2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
> + *
> + * This file is released under the GPLv2.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/suspend.h>
> +
> +/*
> + * Platforms, like ACPI, may want us to save some memory used by them during
> + * suspend and to restore the contents of this memory during the subsequent
> + * resume.  The code below implements a mechanism allowing us to do that.
> + */
> +
> +struct nvs_page {
> +	unsigned long phys_start;
> +	unsigned int size;
> +	void *kaddr;
> +	void *data;
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(nvs_list);
> +
> +/**
> + *	suspend_nvs_register - register platform NVS memory region to save
> + *	@start - physical address of the region
> + *	@size - size of the region
> + *
> + *	The NVS region need not be page-aligned (both ends) and we arrange
> + *	things so that the data from page-aligned addresses in this region will
> + *	be copied into separate RAM pages.
> + */
> +int suspend_nvs_register(unsigned long start, unsigned long size)
> +{
> +	struct nvs_page *entry, *next;
> +
> +	while (size > 0) {
> +		unsigned int nr_bytes;
> +
> +		entry = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
> +		if (!entry)
> +			goto Error;
> +
> +		list_add_tail(&entry->node, &nvs_list);
> +		entry->phys_start = start;
> +		nr_bytes = PAGE_SIZE - (start & ~PAGE_MASK);
> +		entry->size = (size < nr_bytes) ? size : nr_bytes;
> +
> +		start += entry->size;
> +		size -= entry->size;
> +	}
> +	return 0;
> +
> + Error:
> +	list_for_each_entry_safe(entry, next, &nvs_list, node) {
> +		list_del(&entry->node);
> +		kfree(entry);
> +	}
> +	return -ENOMEM;
> +}
> +
> +/**
> + *	suspend_nvs_free - free data pages allocated for saving NVS regions
> + */
> +void suspend_nvs_free(void)
> +{
> +	struct nvs_page *entry;
> +
> +	list_for_each_entry(entry, &nvs_list, node)
> +		if (entry->data) {
> +			free_page((unsigned long)entry->data);
> +			entry->data = NULL;
> +			if (entry->kaddr) {
> +				iounmap(entry->kaddr);
> +				entry->kaddr = NULL;
> +			}
> +		}
> +}
> +
> +/**
> + *	suspend_nvs_alloc - allocate memory necessary for saving NVS regions
> + */
> +int suspend_nvs_alloc(void)
> +{
> +	struct nvs_page *entry;
> +
> +	list_for_each_entry(entry, &nvs_list, node) {
> +		entry->data = (void *)__get_free_page(GFP_KERNEL);
> +		if (!entry->data) {
> +			suspend_nvs_free();
> +			return -ENOMEM;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/**
> + *	suspend_nvs_save - save NVS memory regions
> + */
> +void suspend_nvs_save(void)
> +{
> +	struct nvs_page *entry;
> +
> +	printk(KERN_INFO "PM: Saving platform NVS memory\n");
> +
> +	list_for_each_entry(entry, &nvs_list, node)
> +		if (entry->data) {
> +			entry->kaddr = ioremap(entry->phys_start, entry->size);
> +			memcpy(entry->data, entry->kaddr, entry->size);
> +		}
> +}
> +
> +/**
> + *	suspend_nvs_restore - restore NVS memory regions
> + *
> + *	This function is going to be called with interrupts disabled, so it
> + *	cannot iounmap the virtual addresses used to access the NVS region.
> + */
> +void suspend_nvs_restore(void)
> +{
> +	struct nvs_page *entry;
> +
> +	printk(KERN_INFO "PM: Restoring platform NVS memory\n");
> +
> +	list_for_each_entry(entry, &nvs_list, node)
> +		if (entry->data)
> +			memcpy(entry->kaddr, entry->data, entry->size);
> +}
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 56e7dbb..f37cb7d 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -16,6 +16,12 @@
>  #include <linux/cpu.h>
>  #include <linux/syscalls.h>
>  #include <linux/gfp.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/suspend.h>
>  
>  #include "power.h"
>  
> 

Re: [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Maxim Levitsky]

On Tue, 2010-06-01 at 23:31 +0200, Rafael J. Wysocki wrote: 
> On Tuesday 01 June 2010, Maxim Levitsky wrote:
> > Saving platform non-volatile state may be required for suspend to RAM as
> > well as hibernation. Move it to more generic code.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > Signed-off-by: Matthew Garrett <maximlevitsky@gmail.com>
> 
> You made a mistake here.
> 
> Also, why are you resending the Matthews patches?  I think Len has seen them
> already.
Yea, a copypaste.

(I was told that if one submits modified patch, it adds his
Signed-off-by.)

I rebased these on top of 
ACPI / EC / PM: Fix race between EC transactions and system suspend'

To be honest, I just want to get some feedback on this.
This was major issue that kept me from using otherwise prefect suspend
to ram.

Thus I am thinking that maybe ready to apply patch will have more
chances to be reviewed....

(Athough I guess I need to wait until the oil spill in gulf of Mexico is
plugged, because it seems to continue to feed the never ending
discussion/flamewar on android suspend issues)....

Best regards,
Maxim Levitsky

> 
> Rafael
> 
> 
> > ---
> >  arch/x86/kernel/e820.c       |    2 +-
> >  drivers/acpi/sleep.c         |   12 ++--
> >  include/linux/suspend.h      |   26 ++++----
> >  kernel/power/Kconfig         |    9 ++-
> >  kernel/power/Makefile        |    2 +-
> >  kernel/power/hibernate_nvs.c |  136 ------------------------------------------
> >  kernel/power/nvs.c           |  136 ++++++++++++++++++++++++++++++++++++++++++
> >  kernel/power/suspend.c       |    6 ++
> >  8 files changed, 168 insertions(+), 161 deletions(-)
> >  delete mode 100644 kernel/power/hibernate_nvs.c
> >  create mode 100644 kernel/power/nvs.c
> > 
> > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> > index 7bca3c6..0d6fc71 100644
> > --- a/arch/x86/kernel/e820.c
> > +++ b/arch/x86/kernel/e820.c
> > @@ -729,7 +729,7 @@ static int __init e820_mark_nvs_memory(void)
> >  		struct e820entry *ei = &e820.map[i];
> >  
> >  		if (ei->type == E820_NVS)
> > -			hibernate_nvs_register(ei->addr, ei->size);
> > +			suspend_nvs_register(ei->addr, ei->size);
> >  	}
> >  
> >  	return 0;
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 3fb4bde..bdb306f 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -404,7 +404,7 @@ static int acpi_hibernation_begin(void)
> >  {
> >  	int error;
> >  
> > -	error = s4_no_nvs ? 0 : hibernate_nvs_alloc();
> > +	error = s4_no_nvs ? 0 : suspend_nvs_alloc();
> >  	if (!error) {
> >  		acpi_target_sleep_state = ACPI_STATE_S4;
> >  		acpi_sleep_tts_switch(acpi_target_sleep_state);
> > @@ -418,7 +418,7 @@ static int acpi_hibernation_pre_snapshot(void)
> >  	int error = acpi_pm_prepare();
> >  
> >  	if (!error)
> > -		hibernate_nvs_save();
> > +		suspend_nvs_save();
> >  
> >  	return error;
> >  }
> > @@ -443,7 +443,7 @@ static int acpi_hibernation_enter(void)
> >  
> >  static void acpi_hibernation_finish(void)
> >  {
> > -	hibernate_nvs_free();
> > +	suspend_nvs_free();
> >  	acpi_ec_unblock_transactions();
> >  	acpi_pm_finish();
> >  }
> > @@ -464,7 +464,7 @@ static void acpi_hibernation_leave(void)
> >  		panic("ACPI S4 hardware signature mismatch");
> >  	}
> >  	/* Restore the NVS memory area */
> > -	hibernate_nvs_restore();
> > +	suspend_nvs_restore();
> >  	/* Allow EC transactions to happen. */
> >  	acpi_ec_unblock_transactions_early();
> >  }
> > @@ -507,7 +507,7 @@ static int acpi_hibernation_begin_old(void)
> >  
> >  	if (!error) {
> >  		if (!s4_no_nvs)
> > -			error = hibernate_nvs_alloc();
> > +			error = suspend_nvs_alloc();
> >  		if (!error)
> >  			acpi_target_sleep_state = ACPI_STATE_S4;
> >  	}
> > @@ -517,7 +517,7 @@ static int acpi_hibernation_begin_old(void)
> >  static int acpi_hibernation_pre_snapshot_old(void)
> >  {
> >  	acpi_pm_freeze();
> > -	hibernate_nvs_save();
> > +	suspend_nvs_save();
> >  	return 0;
> >  }
> >  
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 5e781d8..bc7d6bb 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -256,22 +256,22 @@ static inline int hibernate(void) { return -ENOSYS; }
> >  static inline bool system_entering_hibernation(void) { return false; }
> >  #endif /* CONFIG_HIBERNATION */
> >  
> > -#ifdef CONFIG_HIBERNATION_NVS
> > -extern int hibernate_nvs_register(unsigned long start, unsigned long size);
> > -extern int hibernate_nvs_alloc(void);
> > -extern void hibernate_nvs_free(void);
> > -extern void hibernate_nvs_save(void);
> > -extern void hibernate_nvs_restore(void);
> > -#else /* CONFIG_HIBERNATION_NVS */
> > -static inline int hibernate_nvs_register(unsigned long a, unsigned long b)
> > +#ifdef CONFIG_SUSPEND_NVS
> > +extern int suspend_nvs_register(unsigned long start, unsigned long size);
> > +extern int suspend_nvs_alloc(void);
> > +extern void suspend_nvs_free(void);
> > +extern void suspend_nvs_save(void);
> > +extern void suspend_nvs_restore(void);
> > +#else /* CONFIG_SUSPEND_NVS */
> > +static inline int suspend_nvs_register(unsigned long a, unsigned long b)
> >  {
> >  	return 0;
> >  }
> > -static inline int hibernate_nvs_alloc(void) { return 0; }
> > -static inline void hibernate_nvs_free(void) {}
> > -static inline void hibernate_nvs_save(void) {}
> > -static inline void hibernate_nvs_restore(void) {}
> > -#endif /* CONFIG_HIBERNATION_NVS */
> > +static inline int suspend_nvs_alloc(void) { return 0; }
> > +static inline void suspend_nvs_free(void) {}
> > +static inline void suspend_nvs_save(void) {}
> > +static inline void suspend_nvs_restore(void) {}
> > +#endif /* CONFIG_SUSPEND_NVS */
> >  
> >  #ifdef CONFIG_PM_SLEEP
> >  void save_processor_state(void);
> > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > index 5c36ea9..ca6066a 100644
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -99,9 +99,13 @@ config PM_SLEEP_ADVANCED_DEBUG
> >  	depends on PM_ADVANCED_DEBUG
> >  	default n
> >  
> > +config SUSPEND_NVS
> > +       bool
> > +
> >  config SUSPEND
> >  	bool "Suspend to RAM and standby"
> >  	depends on PM && ARCH_SUSPEND_POSSIBLE
> > +	select SUSPEND_NVS if HAS_IOMEM
> >  	default y
> >  	---help---
> >  	  Allow the system to enter sleep states in which main memory is
> > @@ -130,13 +134,10 @@ config SUSPEND_FREEZER
> >  
> >  	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
> >  
> > -config HIBERNATION_NVS
> > -	bool
> > -
> >  config HIBERNATION
> >  	bool "Hibernation (aka 'suspend to disk')"
> >  	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
> > -	select HIBERNATION_NVS if HAS_IOMEM
> > +	select SUSPEND_NVS if HAS_IOMEM
> >  	---help---
> >  	  Enable the suspend to disk (STD) functionality, which is usually
> >  	  called "hibernation" in user interfaces.  STD checkpoints the
> > diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> > index 524e058..f9063c6 100644
> > --- a/kernel/power/Makefile
> > +++ b/kernel/power/Makefile
> > @@ -10,6 +10,6 @@ obj-$(CONFIG_SUSPEND)		+= suspend.o
> >  obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
> >  obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o \
> >  				   block_io.o
> > -obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
> > +obj-$(CONFIG_SUSPEND_NVS)	+= nvs.o
> >  
> >  obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
> > diff --git a/kernel/power/hibernate_nvs.c b/kernel/power/hibernate_nvs.c
> > deleted file mode 100644
> > index fdcad9e..0000000
> > --- a/kernel/power/hibernate_nvs.c
> > +++ /dev/null
> > @@ -1,136 +0,0 @@
> > -/*
> > - * linux/kernel/power/hibernate_nvs.c - Routines for handling NVS memory
> > - *
> > - * Copyright (C) 2008,2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
> > - *
> > - * This file is released under the GPLv2.
> > - */
> > -
> > -#include <linux/io.h>
> > -#include <linux/kernel.h>
> > -#include <linux/list.h>
> > -#include <linux/mm.h>
> > -#include <linux/slab.h>
> > -#include <linux/suspend.h>
> > -
> > -/*
> > - * Platforms, like ACPI, may want us to save some memory used by them during
> > - * hibernation and to restore the contents of this memory during the subsequent
> > - * resume.  The code below implements a mechanism allowing us to do that.
> > - */
> > -
> > -struct nvs_page {
> > -	unsigned long phys_start;
> > -	unsigned int size;
> > -	void *kaddr;
> > -	void *data;
> > -	struct list_head node;
> > -};
> > -
> > -static LIST_HEAD(nvs_list);
> > -
> > -/**
> > - *	hibernate_nvs_register - register platform NVS memory region to save
> > - *	@start - physical address of the region
> > - *	@size - size of the region
> > - *
> > - *	The NVS region need not be page-aligned (both ends) and we arrange
> > - *	things so that the data from page-aligned addresses in this region will
> > - *	be copied into separate RAM pages.
> > - */
> > -int hibernate_nvs_register(unsigned long start, unsigned long size)
> > -{
> > -	struct nvs_page *entry, *next;
> > -
> > -	while (size > 0) {
> > -		unsigned int nr_bytes;
> > -
> > -		entry = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
> > -		if (!entry)
> > -			goto Error;
> > -
> > -		list_add_tail(&entry->node, &nvs_list);
> > -		entry->phys_start = start;
> > -		nr_bytes = PAGE_SIZE - (start & ~PAGE_MASK);
> > -		entry->size = (size < nr_bytes) ? size : nr_bytes;
> > -
> > -		start += entry->size;
> > -		size -= entry->size;
> > -	}
> > -	return 0;
> > -
> > - Error:
> > -	list_for_each_entry_safe(entry, next, &nvs_list, node) {
> > -		list_del(&entry->node);
> > -		kfree(entry);
> > -	}
> > -	return -ENOMEM;
> > -}
> > -
> > -/**
> > - *	hibernate_nvs_free - free data pages allocated for saving NVS regions
> > - */
> > -void hibernate_nvs_free(void)
> > -{
> > -	struct nvs_page *entry;
> > -
> > -	list_for_each_entry(entry, &nvs_list, node)
> > -		if (entry->data) {
> > -			free_page((unsigned long)entry->data);
> > -			entry->data = NULL;
> > -			if (entry->kaddr) {
> > -				iounmap(entry->kaddr);
> > -				entry->kaddr = NULL;
> > -			}
> > -		}
> > -}
> > -
> > -/**
> > - *	hibernate_nvs_alloc - allocate memory necessary for saving NVS regions
> > - */
> > -int hibernate_nvs_alloc(void)
> > -{
> > -	struct nvs_page *entry;
> > -
> > -	list_for_each_entry(entry, &nvs_list, node) {
> > -		entry->data = (void *)__get_free_page(GFP_KERNEL);
> > -		if (!entry->data) {
> > -			hibernate_nvs_free();
> > -			return -ENOMEM;
> > -		}
> > -	}
> > -	return 0;
> > -}
> > -
> > -/**
> > - *	hibernate_nvs_save - save NVS memory regions
> > - */
> > -void hibernate_nvs_save(void)
> > -{
> > -	struct nvs_page *entry;
> > -
> > -	printk(KERN_INFO "PM: Saving platform NVS memory\n");
> > -
> > -	list_for_each_entry(entry, &nvs_list, node)
> > -		if (entry->data) {
> > -			entry->kaddr = ioremap(entry->phys_start, entry->size);
> > -			memcpy(entry->data, entry->kaddr, entry->size);
> > -		}
> > -}
> > -
> > -/**
> > - *	hibernate_nvs_restore - restore NVS memory regions
> > - *
> > - *	This function is going to be called with interrupts disabled, so it
> > - *	cannot iounmap the virtual addresses used to access the NVS region.
> > - */
> > -void hibernate_nvs_restore(void)
> > -{
> > -	struct nvs_page *entry;
> > -
> > -	printk(KERN_INFO "PM: Restoring platform NVS memory\n");
> > -
> > -	list_for_each_entry(entry, &nvs_list, node)
> > -		if (entry->data)
> > -			memcpy(entry->kaddr, entry->data, entry->size);
> > -}
> > diff --git a/kernel/power/nvs.c b/kernel/power/nvs.c
> > new file mode 100644
> > index 0000000..30c662f
> > --- /dev/null
> > +++ b/kernel/power/nvs.c
> > @@ -0,0 +1,136 @@
> > +/*
> > + * linux/kernel/power/nvs.c - Routines for handling NVS memory
> > + *
> > + * Copyright (C) 2008,2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
> > + *
> > + * This file is released under the GPLv2.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/mm.h>
> > +#include <linux/slab.h>
> > +#include <linux/suspend.h>
> > +
> > +/*
> > + * Platforms, like ACPI, may want us to save some memory used by them during
> > + * suspend and to restore the contents of this memory during the subsequent
> > + * resume.  The code below implements a mechanism allowing us to do that.
> > + */
> > +
> > +struct nvs_page {
> > +	unsigned long phys_start;
> > +	unsigned int size;
> > +	void *kaddr;
> > +	void *data;
> > +	struct list_head node;
> > +};
> > +
> > +static LIST_HEAD(nvs_list);
> > +
> > +/**
> > + *	suspend_nvs_register - register platform NVS memory region to save
> > + *	@start - physical address of the region
> > + *	@size - size of the region
> > + *
> > + *	The NVS region need not be page-aligned (both ends) and we arrange
> > + *	things so that the data from page-aligned addresses in this region will
> > + *	be copied into separate RAM pages.
> > + */
> > +int suspend_nvs_register(unsigned long start, unsigned long size)
> > +{
> > +	struct nvs_page *entry, *next;
> > +
> > +	while (size > 0) {
> > +		unsigned int nr_bytes;
> > +
> > +		entry = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
> > +		if (!entry)
> > +			goto Error;
> > +
> > +		list_add_tail(&entry->node, &nvs_list);
> > +		entry->phys_start = start;
> > +		nr_bytes = PAGE_SIZE - (start & ~PAGE_MASK);
> > +		entry->size = (size < nr_bytes) ? size : nr_bytes;
> > +
> > +		start += entry->size;
> > +		size -= entry->size;
> > +	}
> > +	return 0;
> > +
> > + Error:
> > +	list_for_each_entry_safe(entry, next, &nvs_list, node) {
> > +		list_del(&entry->node);
> > +		kfree(entry);
> > +	}
> > +	return -ENOMEM;
> > +}
> > +
> > +/**
> > + *	suspend_nvs_free - free data pages allocated for saving NVS regions
> > + */
> > +void suspend_nvs_free(void)
> > +{
> > +	struct nvs_page *entry;
> > +
> > +	list_for_each_entry(entry, &nvs_list, node)
> > +		if (entry->data) {
> > +			free_page((unsigned long)entry->data);
> > +			entry->data = NULL;
> > +			if (entry->kaddr) {
> > +				iounmap(entry->kaddr);
> > +				entry->kaddr = NULL;
> > +			}
> > +		}
> > +}
> > +
> > +/**
> > + *	suspend_nvs_alloc - allocate memory necessary for saving NVS regions
> > + */
> > +int suspend_nvs_alloc(void)
> > +{
> > +	struct nvs_page *entry;
> > +
> > +	list_for_each_entry(entry, &nvs_list, node) {
> > +		entry->data = (void *)__get_free_page(GFP_KERNEL);
> > +		if (!entry->data) {
> > +			suspend_nvs_free();
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> > + *	suspend_nvs_save - save NVS memory regions
> > + */
> > +void suspend_nvs_save(void)
> > +{
> > +	struct nvs_page *entry;
> > +
> > +	printk(KERN_INFO "PM: Saving platform NVS memory\n");
> > +
> > +	list_for_each_entry(entry, &nvs_list, node)
> > +		if (entry->data) {
> > +			entry->kaddr = ioremap(entry->phys_start, entry->size);
> > +			memcpy(entry->data, entry->kaddr, entry->size);
> > +		}
> > +}
> > +
> > +/**
> > + *	suspend_nvs_restore - restore NVS memory regions
> > + *
> > + *	This function is going to be called with interrupts disabled, so it
> > + *	cannot iounmap the virtual addresses used to access the NVS region.
> > + */
> > +void suspend_nvs_restore(void)
> > +{
> > +	struct nvs_page *entry;
> > +
> > +	printk(KERN_INFO "PM: Restoring platform NVS memory\n");
> > +
> > +	list_for_each_entry(entry, &nvs_list, node)
> > +		if (entry->data)
> > +			memcpy(entry->kaddr, entry->data, entry->size);
> > +}
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 56e7dbb..f37cb7d 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -16,6 +16,12 @@
> >  #include <linux/cpu.h>
> >  #include <linux/syscalls.h>
> >  #include <linux/gfp.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/mm.h>
> > +#include <linux/slab.h>
> > +#include <linux/suspend.h>
> >  
> >  #include "power.h"
> >  
> > 
> 
> --
> 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

Re: [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Rafael J. Wysocki]

On Tuesday 01 June 2010, Maxim Levitsky wrote:
> On Tue, 2010-06-01 at 23:31 +0200, Rafael J. Wysocki wrote: 
> > On Tuesday 01 June 2010, Maxim Levitsky wrote:
> > > Saving platform non-volatile state may be required for suspend to RAM as
> > > well as hibernation. Move it to more generic code.
> > > 
> > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > > Signed-off-by: Matthew Garrett <maximlevitsky@gmail.com>
> > 
> > You made a mistake here.
> > 
> > Also, why are you resending the Matthews patches?  I think Len has seen them
> > already.
> Yea, a copypaste.
> 
> (I was told that if one submits modified patch, it adds his
> Signed-off-by.)
> 
> I rebased these on top of 
> ACPI / EC / PM: Fix race between EC transactions and system suspend'
> 
> To be honest, I just want to get some feedback on this.
> This was major issue that kept me from using otherwise prefect suspend
> to ram.

I think this is a change we should try, but there is a chance it will break
some systems.

> Thus I am thinking that maybe ready to apply patch will have more
> chances to be reviewed....

Not really (as far as I'm concerned at least). :-)

Rafael

Re: [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Maxim Levitsky]

On Wed, 2010-06-02 at 00:40 +0200, Rafael J. Wysocki wrote: 
> On Tuesday 01 June 2010, Maxim Levitsky wrote:
> > On Tue, 2010-06-01 at 23:31 +0200, Rafael J. Wysocki wrote: 
> > > On Tuesday 01 June 2010, Maxim Levitsky wrote:
> > > > Saving platform non-volatile state may be required for suspend to RAM as
> > > > well as hibernation. Move it to more generic code.
> > > > 
> > > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > > > Signed-off-by: Matthew Garrett <maximlevitsky@gmail.com>
> > > 
> > > You made a mistake here.
> > > 
> > > Also, why are you resending the Matthews patches?  I think Len has seen them
> > > already.
> > Yea, a copypaste.
> > 
> > (I was told that if one submits modified patch, it adds his
> > Signed-off-by.)
> > 
> > I rebased these on top of 
> > ACPI / EC / PM: Fix race between EC transactions and system suspend'
> > 
> > To be honest, I just want to get some feedback on this.
> > This was major issue that kept me from using otherwise prefect suspend
> > to ram.
> 
> I think this is a change we should try, but there is a chance it will break
> some systems.
I doubt that. BIOSes are tested against windows, and since it has this
behavior, all systems this patch could break wouldn't work in windows
ether.

I was doing all kinds of attempts to fix this bug, and finally I found
the cause for it.

Best regards,
Maxim Levitsky

Re: [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Rafael J. Wysocki]

On Wednesday 02 June 2010, Maxim Levitsky wrote:
> On Wed, 2010-06-02 at 00:40 +0200, Rafael J. Wysocki wrote: 
> > On Tuesday 01 June 2010, Maxim Levitsky wrote:
> > > On Tue, 2010-06-01 at 23:31 +0200, Rafael J. Wysocki wrote: 
> > > > On Tuesday 01 June 2010, Maxim Levitsky wrote:
> > > > > Saving platform non-volatile state may be required for suspend to RAM as
> > > > > well as hibernation. Move it to more generic code.
> > > > > 
> > > > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > > > > Signed-off-by: Matthew Garrett <maximlevitsky@gmail.com>
> > > > 
> > > > You made a mistake here.
> > > > 
> > > > Also, why are you resending the Matthews patches?  I think Len has seen them
> > > > already.
> > > Yea, a copypaste.
> > > 
> > > (I was told that if one submits modified patch, it adds his
> > > Signed-off-by.)
> > > 
> > > I rebased these on top of 
> > > ACPI / EC / PM: Fix race between EC transactions and system suspend'
> > > 
> > > To be honest, I just want to get some feedback on this.
> > > This was major issue that kept me from using otherwise prefect suspend
> > > to ram.
> > 
> > I think this is a change we should try, but there is a chance it will break
> > some systems.
> I doubt that. BIOSes are tested against windows, and since it has this
> behavior, all systems this patch could break wouldn't work in windows
> ether.

Still, some systems were broken by the saving-restoring NVS over hibernation.

> I was doing all kinds of attempts to fix this bug, and finally I found
> the cause for it.

I know and I'm not objecting to the patches.  I'm only saying we should be
careful with them.

Rafael

Re: [linux-pm] [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Pavel Machek]

Hi!

> > > To be honest, I just want to get some feedback on this.
> > > This was major issue that kept me from using otherwise prefect suspend
> > > to ram.
> > 
> > I think this is a change we should try, but there is a chance it will break
> > some systems.
> I doubt that. BIOSes are tested against windows, and since it has this
> behavior, all systems this patch could break wouldn't work in windows
> ether.
> 
> I was doing all kinds of attempts to fix this bug, and finally I found
> the cause for it.

Well... should we only do it in 'platform' hibernation mode? There are
machines that predate windows/acpi and still should hibernate.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Matthew Garrett]

On Thu, Jun 10, 2010 at 03:58:16PM +0200, Pavel Machek wrote:

> Well... should we only do it in 'platform' hibernation mode? There are
> machines that predate windows/acpi and still should hibernate.

If they predate acpi then they won't declare any NVS regions.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [linux-pm] [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Henrique de Moraes Holschuh]

On Thu, 10 Jun 2010, Matthew Garrett wrote:
> On Thu, Jun 10, 2010 at 03:58:16PM +0200, Pavel Machek wrote:
> > Well... should we only do it in 'platform' hibernation mode? There are
> > machines that predate windows/acpi and still should hibernate.
> 
> If they predate acpi then they won't declare any NVS regions.

What if they're ACPI and APM capable, and in APM mode?  Would the code need
to guard against that?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [linux-pm] [PATCH 1/2] suspend: Move NVS save/restore code to generic suspend functionality

[Matthew Garrett]

On Fri, Jun 11, 2010 at 10:32:19AM -0300, Henrique de Moraes Holschuh wrote:
> On Thu, 10 Jun 2010, Matthew Garrett wrote:
> > On Thu, Jun 10, 2010 at 03:58:16PM +0200, Pavel Machek wrote:
> > > Well... should we only do it in 'platform' hibernation mode? There are
> > > machines that predate windows/acpi and still should hibernate.
> > 
> > If they predate acpi then they won't declare any NVS regions.
> 
> What if they're ACPI and APM capable, and in APM mode?  Would the code need
> to guard against that?

I can't imagine any case where that would cause problems, but if it does 
then it's an easy fix.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 2/2] ACPI: Store NVS state even when entering suspend to RAM

[Maxim Levitsky]

https://bugzilla.kernel.org/show_bug.cgi?id=13931 describes a bug where
a system fails to successfully resume after the second suspend. Maxim
Levitsky discovered that this could be rectified by forcibly saving
and restoring the ACPI non-volatile state. The spec indicates that this
is only required for S4, but testing the behaviour of Windows by adding
an ACPI NVS region to qemu's e820 map and registering a custom memory
read/write handler reveals that it's saved and restored even over suspend
to RAM. We should mimic that behaviour to avoid other broken platforms.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/acpi/sleep.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index bdb306f..5d26d8e 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -114,6 +114,8 @@ static int __acpi_pm_prepare(void)
 {
 	int error = acpi_sleep_prepare(acpi_target_sleep_state);
 
+	suspend_nvs_save();
+
 	if (error)
 		acpi_target_sleep_state = ACPI_STATE_S0;
 	return error;
@@ -143,6 +145,8 @@ static void acpi_pm_finish(void)
 {
 	u32 acpi_state = acpi_target_sleep_state;
 
+	suspend_nvs_free();
+
 	if (acpi_state == ACPI_STATE_S0)
 		return;
 
@@ -192,6 +196,11 @@ static int acpi_suspend_begin(suspend_state_t pm_state)
 	u32 acpi_state = acpi_suspend_states[pm_state];
 	int error = 0;
 
+	error = suspend_nvs_alloc();
+
+	if (error)
+		return error;
+
 	if (sleep_states[acpi_state]) {
 		acpi_target_sleep_state = acpi_state;
 		acpi_sleep_tts_switch(acpi_target_sleep_state);
@@ -269,6 +278,8 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	if (acpi_state == ACPI_STATE_S3)
 		acpi_restore_state_mem();
 
+	suspend_nvs_restore();
+
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
@@ -443,7 +454,6 @@ static int acpi_hibernation_enter(void)
 
 static void acpi_hibernation_finish(void)
 {
-	suspend_nvs_free();
 	acpi_ec_unblock_transactions();
 	acpi_pm_finish();
 }
@@ -479,7 +489,7 @@ static struct platform_hibernation_ops acpi_hibernation_ops = {
 	.begin = acpi_hibernation_begin,
 	.end = acpi_pm_end,
 	.pre_snapshot = acpi_hibernation_pre_snapshot,
-	.finish = acpi_hibernation_finish,
+	.finish = acpi_pm_finish,
 	.prepare = acpi_pm_prepare,
 	.enter = acpi_hibernation_enter,
 	.leave = acpi_hibernation_leave,
-- 
1.7.0.4