All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC v1 0/3] hibernation support on ARM
@ 2014-02-19  1:52 ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel

Patches adding support for hibernation on ARM
  - ARM hibernation / suspend-to-disk
  - Fix hang in freeze_processes during hibernation
  - add an irq disabled version of soft_restart

Patches based on v3.13 tag, verified hibernation on beaglebone black on a branch
based on 3.13 merged with initial omap support from Russ Dill which be found here:
http://git.linaro.org/git-ro/people/sebastian.capella/linux.git hibernation_3.13_russMerge

[PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
 arch/arm/include/asm/system_misc.h |    1 +
 arch/arm/kernel/process.c          |   15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

 Adds the ability to run soft restart with local_irq/fiq_disable
 Useful in hibernation code paths

[PATCH RFC v1 2/3] Fix hibernation restore hang in freeze_processes
 drivers/base/firmware_class.c |    1 +
 1 file changed, 1 insertion(+)

 Add handling for PM_RESTORE_PREPARE notifier

[PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
 arch/arm/include/asm/memory.h |    1 +
 arch/arm/kernel/Makefile      |    1 +
 arch/arm/kernel/hibernate.c   |  106 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/Kconfig           |    5 ++
 include/linux/suspend.h       |    2 +
 5 files changed, 115 insertions(+)

 Adds support for ARM based hibernation



Additional notes:

There are two checkpatch warnings added by this patch.  These follow
behavior in existing hibernation implementations on other platforms.

    WARNING: externs should be avoided in .c files
    #116: FILE: arch/arm/kernel/hibernate.c:26:
    +extern const void __nosave_begin, __nosave_end;

  This extern is picking up the linker nosave region definitions, only
  used in hibernate.  Follows same extern line used mips, powerpc, s390,
  sh, sparc, x86 & unicore32

    WARNING: externs should be avoided in .c files
    #191: FILE: arch/arm/kernel/hibernate.c:101:
    +extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);

  This extern is used in the arch/arm/ in hibernate, process and bL_switcher






^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 0/3] hibernation support on ARM
@ 2014-02-19  1:52 ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-arm-kernel

Patches adding support for hibernation on ARM
  - ARM hibernation / suspend-to-disk
  - Fix hang in freeze_processes during hibernation
  - add an irq disabled version of soft_restart

Patches based on v3.13 tag, verified hibernation on beaglebone black on a branch
based on 3.13 merged with initial omap support from Russ Dill which be found here:
http://git.linaro.org/git-ro/people/sebastian.capella/linux.git hibernation_3.13_russMerge

[PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
 arch/arm/include/asm/system_misc.h |    1 +
 arch/arm/kernel/process.c          |   15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

 Adds the ability to run soft restart with local_irq/fiq_disable
 Useful in hibernation code paths

[PATCH RFC v1 2/3] Fix hibernation restore hang in freeze_processes
 drivers/base/firmware_class.c |    1 +
 1 file changed, 1 insertion(+)

 Add handling for PM_RESTORE_PREPARE notifier

[PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
 arch/arm/include/asm/memory.h |    1 +
 arch/arm/kernel/Makefile      |    1 +
 arch/arm/kernel/hibernate.c   |  106 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/Kconfig           |    5 ++
 include/linux/suspend.h       |    2 +
 5 files changed, 115 insertions(+)

 Adds support for ARM based hibernation



Additional notes:

There are two checkpatch warnings added by this patch.  These follow
behavior in existing hibernation implementations on other platforms.

    WARNING: externs should be avoided in .c files
    #116: FILE: arch/arm/kernel/hibernate.c:26:
    +extern const void __nosave_begin, __nosave_end;

  This extern is picking up the linker nosave region definitions, only
  used in hibernate.  Follows same extern line used mips, powerpc, s390,
  sh, sparc, x86 & unicore32

    WARNING: externs should be avoided in .c files
    #191: FILE: arch/arm/kernel/hibernate.c:101:
    +extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);

  This extern is used in the arch/arm/ in hibernate, process and bL_switcher

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-19  1:52 ` Sebastian Capella
@ 2014-02-19  1:52   ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel
  Cc: Russ Dill, Sebastian Capella, Russell King, Andrew Morton,
	Thomas Gleixner, Will Deacon, Robin Holt

From: Russ Dill <Russ.Dill@ti.com>

This adds the ability to run soft_restart with local_irq/fiq_disable
already called. This is helpful for the hibernation code paths.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Robin Holt <holt@sgi.com>
---
 arch/arm/include/asm/system_misc.h |    1 +
 arch/arm/kernel/process.c          |   15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
index a3d61ad..eca8dd4 100644
--- a/arch/arm/include/asm/system_misc.h
+++ b/arch/arm/include/asm/system_misc.h
@@ -10,6 +10,7 @@
 
 extern void cpu_init(void);
 
+void soft_restart_noirq(unsigned long);
 void soft_restart(unsigned long);
 extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 extern void (*arm_pm_idle)(void);
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 92f7b15..685bc4a 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -95,14 +95,10 @@ static void __soft_restart(void *addr)
 	BUG();
 }
 
-void soft_restart(unsigned long addr)
+void soft_restart_noirq(unsigned long addr)
 {
 	u64 *stack = soft_restart_stack + ARRAY_SIZE(soft_restart_stack);
 
-	/* Disable interrupts first */
-	local_irq_disable();
-	local_fiq_disable();
-
 	/* Disable the L2 if we're the last man standing. */
 	if (num_online_cpus() == 1)
 		outer_disable();
@@ -114,6 +110,15 @@ void soft_restart(unsigned long addr)
 	BUG();
 }
 
+void soft_restart(unsigned long addr)
+{
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	soft_restart_noirq(addr);
+}
+
 static void null_restart(enum reboot_mode reboot_mode, const char *cmd)
 {
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-19  1:52   ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-arm-kernel

From: Russ Dill <Russ.Dill@ti.com>

This adds the ability to run soft_restart with local_irq/fiq_disable
already called. This is helpful for the hibernation code paths.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Robin Holt <holt@sgi.com>
---
 arch/arm/include/asm/system_misc.h |    1 +
 arch/arm/kernel/process.c          |   15 ++++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
index a3d61ad..eca8dd4 100644
--- a/arch/arm/include/asm/system_misc.h
+++ b/arch/arm/include/asm/system_misc.h
@@ -10,6 +10,7 @@
 
 extern void cpu_init(void);
 
+void soft_restart_noirq(unsigned long);
 void soft_restart(unsigned long);
 extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 extern void (*arm_pm_idle)(void);
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 92f7b15..685bc4a 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -95,14 +95,10 @@ static void __soft_restart(void *addr)
 	BUG();
 }
 
-void soft_restart(unsigned long addr)
+void soft_restart_noirq(unsigned long addr)
 {
 	u64 *stack = soft_restart_stack + ARRAY_SIZE(soft_restart_stack);
 
-	/* Disable interrupts first */
-	local_irq_disable();
-	local_fiq_disable();
-
 	/* Disable the L2 if we're the last man standing. */
 	if (num_online_cpus() == 1)
 		outer_disable();
@@ -114,6 +110,15 @@ void soft_restart(unsigned long addr)
 	BUG();
 }
 
+void soft_restart(unsigned long addr)
+{
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
+
+	soft_restart_noirq(addr);
+}
+
 static void null_restart(enum reboot_mode reboot_mode, const char *cmd)
 {
 }
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 2/3] Fix hibernation restore hang in freeze_processes
  2014-02-19  1:52 ` Sebastian Capella
@ 2014-02-19  1:52   ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel
  Cc: Sebastian Capella, Russ Dill, Ming Lei, Greg Kroah-Hartman

During restore, pm_notifier chain are called with
PM_RESTORE_PREPARE.  The firmware_class driver handler
fw_pm_notify does not have a handler for this.  As a result,
it keeps a reader on the kmod.c umhelper_sem.  During
freeze_processes, the call to __usermodehelper_disable tries to
take a write lock on this semaphore and hangs waiting.

Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Russ Dill <Russ.Dill@ti.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index eb8fb94..e2b51f8 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1541,6 +1541,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 	switch (mode) {
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
+	case PM_RESTORE_PREPARE:
 		kill_requests_without_uevent();
 		device_cache_fw_images();
 		break;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 2/3] Fix hibernation restore hang in freeze_processes
@ 2014-02-19  1:52   ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-arm-kernel

During restore, pm_notifier chain are called with
PM_RESTORE_PREPARE.  The firmware_class driver handler
fw_pm_notify does not have a handler for this.  As a result,
it keeps a reader on the kmod.c umhelper_sem.  During
freeze_processes, the call to __usermodehelper_disable tries to
take a write lock on this semaphore and hangs waiting.

Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Russ Dill <Russ.Dill@ti.com>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index eb8fb94..e2b51f8 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1541,6 +1541,7 @@ static int fw_pm_notify(struct notifier_block *notify_block,
 	switch (mode) {
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
+	case PM_RESTORE_PREPARE:
 		kill_requests_without_uevent();
 		device_cache_fw_images();
 		break;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19  1:52 ` Sebastian Capella
@ 2014-02-19  1:52   ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel
  Cc: Russ Dill, Rafael J. Wysocki, Sebastian Capella, Russell King,
	Len Brown, Pavel Machek, Nicolas Pitre, Santosh Shilimkar,
	Will Deacon, Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-König, Stephen Boyd

From: Russ Dill <Russ.Dill@ti.com>

Enable hibernation for ARM architectures and provide ARM
architecture specific calls used during hibernation.

The swsusp hibernation framework depends on the
platform first having functional suspend/resume.

Then, in order to enable hibernation on a given platform, a
platform_hibernation_ops structure may need to be registered with
the system in order to save/restore any SoC-specific / cpu specific
state needing (re)init over a suspend-to-disk/resume-from-disk cycle.

For example:

     - "secure" SoCs that have different sets of control registers
       and/or different CR reg access patterns.

     - SoCs with L2 caches as the activation sequence there is
       SoC-dependent; a full off-on cycle for L2 is not done
       by the hibernation support code.

     - SoCs requiring steps on wakeup _before_ the "generic" parts
       done by cpu_suspend / cpu_resume can work correctly.

     - SoCs having persistent state which is maintained during suspend
       and resume, but will be lost during the power off cycle after
       suspend-to-disk.

This is a rebase/rework of Frank Hofmann's v5 hibernation patchset.

Cc: Russ Dill <Russ.Dill@ti.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Cyril Chemparathy <cyril@ti.com>
Cc: Jonathan Austin <jonathan.austin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/include/asm/memory.h |    1 +
 arch/arm/kernel/Makefile      |    1 +
 arch/arm/kernel/hibernate.c   |  106 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/Kconfig           |    5 ++
 include/linux/suspend.h       |    2 +
 5 files changed, 115 insertions(+)
 create mode 100644 arch/arm/kernel/hibernate.c

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 8756e4b..1079ea8 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -291,6 +291,7 @@ static inline void *phys_to_virt(phys_addr_t x)
  */
 #define __pa(x)			__virt_to_phys((unsigned long)(x))
 #define __va(x)			((void *)__phys_to_virt((phys_addr_t)(x)))
+#define __pa_symbol(x)		__pa(RELOC_HIDE((unsigned long)(x), 0))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 
 extern phys_addr_t (*arch_virt_to_idmap)(unsigned long x);
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index a30fc9b..8afa848 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_ARTHUR)		+= arthur.o
 obj-$(CONFIG_ISA_DMA)		+= dma-isa.o
 obj-$(CONFIG_PCI)		+= bios32.o isa.o
 obj-$(CONFIG_ARM_CPU_SUSPEND)	+= sleep.o suspend.o
+obj-$(CONFIG_HIBERNATION)	+= hibernate.o
 obj-$(CONFIG_SMP)		+= smp.o
 ifdef CONFIG_MMU
 obj-$(CONFIG_SMP)		+= smp_tlb.o
diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
new file mode 100644
index 0000000..16f406f
--- /dev/null
+++ b/arch/arm/kernel/hibernate.c
@@ -0,0 +1,106 @@
+/*
+ * Hibernation support specific for ARM
+ *
+ * Derived from work on ARM hibernation support by:
+ *
+ * Ubuntu project, hibernation support for mach-dove
+ * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
+ * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
+ *  https://lkml.org/lkml/2010/6/18/4
+ *  https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
+ *  https://patchwork.kernel.org/patch/96442/
+ *
+ * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/mm.h>
+#include <linux/suspend.h>
+#include <asm/tlbflush.h>
+#include <asm/cacheflush.h>
+#include <asm/system_misc.h>
+#include <asm/idmap.h>
+#include <asm/suspend.h>
+
+extern const void __nosave_begin, __nosave_end;
+
+int pfn_is_nosave(unsigned long pfn)
+{
+	unsigned long nosave_begin_pfn =
+			__pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
+	unsigned long nosave_end_pfn =
+			PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
+
+	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
+}
+
+void notrace save_processor_state(void)
+{
+	WARN_ON(num_online_cpus() != 1);
+	flush_thread();
+	local_fiq_disable();
+}
+
+void notrace restore_processor_state(void)
+{
+	local_fiq_enable();
+}
+
+/*
+ * Snapshot kernel memory and reset the system.
+ * After resume, the hibernation snapshot is written out.
+ */
+static int notrace __swsusp_arch_save_image(unsigned long unused)
+{
+	int ret;
+
+	ret = swsusp_save();
+	if (ret == 0)
+		soft_restart(virt_to_phys(cpu_resume));
+	return ret;
+}
+
+/*
+ * Save the current CPU state before suspend / poweroff.
+ */
+int notrace swsusp_arch_suspend(void)
+{
+	return cpu_suspend(0, __swsusp_arch_save_image);
+}
+
+/*
+ * The framework loads the hibernation image into a linked list anchored
+ * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
+ * destinations.
+ *
+ * To make this work if resume is triggered from initramfs, the
+ * pagetables need to be switched to allow writes to kernel mem.
+ */
+static void notrace __swsusp_arch_restore_image(void *unused)
+{
+	struct pbe *pbe;
+
+	cpu_switch_mm(idmap_pgd, &init_mm);
+	for (pbe = restore_pblist; pbe; pbe = pbe->next)
+		copy_page(pbe->orig_address, pbe->address);
+
+	soft_restart_noirq(virt_to_phys(cpu_resume));
+}
+
+static u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
+
+/*
+ * Resume from the hibernation image.
+ * Due to the kernel heap / data restore, stack contents change underneath
+ * and that would make function calls impossible; switch to a temporary
+ * stack within the nosave region to avoid that problem.
+ */
+int __naked swsusp_arch_resume(void)
+{
+	extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
+	cpu_init();	/* get a clean PSR */
+	call_with_stack(__swsusp_arch_restore_image, 0,
+		__swsusp_resume_stk + sizeof(__swsusp_resume_stk));
+	return 0;
+}
diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 1f8fed9..83707702 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -611,6 +611,11 @@ config CPU_USE_DOMAINS
 config IO_36
 	bool
 
+config ARCH_HIBERNATION_POSSIBLE
+	bool
+	depends on MMU
+	default y if CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || CPU_XSCALE || CPU_XSC3 || CPU_V6 || CPU_V6K || CPU_V7
+
 comment "Processor Features"
 
 config ARM_LPAE
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index f73cabf..38bbf95 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -320,6 +320,8 @@ extern unsigned long get_safe_page(gfp_t gfp_mask);
 extern void hibernation_set_ops(const struct platform_hibernation_ops *ops);
 extern int hibernate(void);
 extern bool system_entering_hibernation(void);
+asmlinkage int swsusp_save(void);
+extern struct pbe *restore_pblist;
 #else /* CONFIG_HIBERNATION */
 static inline void register_nosave_region(unsigned long b, unsigned long e) {}
 static inline void register_nosave_region_late(unsigned long b, unsigned long e) {}
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-19  1:52   ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19  1:52 UTC (permalink / raw)
  To: linux-arm-kernel

From: Russ Dill <Russ.Dill@ti.com>

Enable hibernation for ARM architectures and provide ARM
architecture specific calls used during hibernation.

The swsusp hibernation framework depends on the
platform first having functional suspend/resume.

Then, in order to enable hibernation on a given platform, a
platform_hibernation_ops structure may need to be registered with
the system in order to save/restore any SoC-specific / cpu specific
state needing (re)init over a suspend-to-disk/resume-from-disk cycle.

For example:

     - "secure" SoCs that have different sets of control registers
       and/or different CR reg access patterns.

     - SoCs with L2 caches as the activation sequence there is
       SoC-dependent; a full off-on cycle for L2 is not done
       by the hibernation support code.

     - SoCs requiring steps on wakeup _before_ the "generic" parts
       done by cpu_suspend / cpu_resume can work correctly.

     - SoCs having persistent state which is maintained during suspend
       and resume, but will be lost during the power off cycle after
       suspend-to-disk.

This is a rebase/rework of Frank Hofmann's v5 hibernation patchset.

Cc: Russ Dill <Russ.Dill@ti.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Cyril Chemparathy <cyril@ti.com>
Cc: Jonathan Austin <jonathan.austin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: "Uwe Kleine-K?nig" <u.kleine-koenig@pengutronix.de>
Cc: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/include/asm/memory.h |    1 +
 arch/arm/kernel/Makefile      |    1 +
 arch/arm/kernel/hibernate.c   |  106 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/Kconfig           |    5 ++
 include/linux/suspend.h       |    2 +
 5 files changed, 115 insertions(+)
 create mode 100644 arch/arm/kernel/hibernate.c

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 8756e4b..1079ea8 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -291,6 +291,7 @@ static inline void *phys_to_virt(phys_addr_t x)
  */
 #define __pa(x)			__virt_to_phys((unsigned long)(x))
 #define __va(x)			((void *)__phys_to_virt((phys_addr_t)(x)))
+#define __pa_symbol(x)		__pa(RELOC_HIDE((unsigned long)(x), 0))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 
 extern phys_addr_t (*arch_virt_to_idmap)(unsigned long x);
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index a30fc9b..8afa848 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_ARTHUR)		+= arthur.o
 obj-$(CONFIG_ISA_DMA)		+= dma-isa.o
 obj-$(CONFIG_PCI)		+= bios32.o isa.o
 obj-$(CONFIG_ARM_CPU_SUSPEND)	+= sleep.o suspend.o
+obj-$(CONFIG_HIBERNATION)	+= hibernate.o
 obj-$(CONFIG_SMP)		+= smp.o
 ifdef CONFIG_MMU
 obj-$(CONFIG_SMP)		+= smp_tlb.o
diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
new file mode 100644
index 0000000..16f406f
--- /dev/null
+++ b/arch/arm/kernel/hibernate.c
@@ -0,0 +1,106 @@
+/*
+ * Hibernation support specific for ARM
+ *
+ * Derived from work on ARM hibernation support by:
+ *
+ * Ubuntu project, hibernation support for mach-dove
+ * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
+ * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
+ *  https://lkml.org/lkml/2010/6/18/4
+ *  https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
+ *  https://patchwork.kernel.org/patch/96442/
+ *
+ * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/mm.h>
+#include <linux/suspend.h>
+#include <asm/tlbflush.h>
+#include <asm/cacheflush.h>
+#include <asm/system_misc.h>
+#include <asm/idmap.h>
+#include <asm/suspend.h>
+
+extern const void __nosave_begin, __nosave_end;
+
+int pfn_is_nosave(unsigned long pfn)
+{
+	unsigned long nosave_begin_pfn =
+			__pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
+	unsigned long nosave_end_pfn =
+			PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
+
+	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
+}
+
+void notrace save_processor_state(void)
+{
+	WARN_ON(num_online_cpus() != 1);
+	flush_thread();
+	local_fiq_disable();
+}
+
+void notrace restore_processor_state(void)
+{
+	local_fiq_enable();
+}
+
+/*
+ * Snapshot kernel memory and reset the system.
+ * After resume, the hibernation snapshot is written out.
+ */
+static int notrace __swsusp_arch_save_image(unsigned long unused)
+{
+	int ret;
+
+	ret = swsusp_save();
+	if (ret == 0)
+		soft_restart(virt_to_phys(cpu_resume));
+	return ret;
+}
+
+/*
+ * Save the current CPU state before suspend / poweroff.
+ */
+int notrace swsusp_arch_suspend(void)
+{
+	return cpu_suspend(0, __swsusp_arch_save_image);
+}
+
+/*
+ * The framework loads the hibernation image into a linked list anchored
+ *@restore_pblist, for swsusp_arch_resume() to copy back to the proper
+ * destinations.
+ *
+ * To make this work if resume is triggered from initramfs, the
+ * pagetables need to be switched to allow writes to kernel mem.
+ */
+static void notrace __swsusp_arch_restore_image(void *unused)
+{
+	struct pbe *pbe;
+
+	cpu_switch_mm(idmap_pgd, &init_mm);
+	for (pbe = restore_pblist; pbe; pbe = pbe->next)
+		copy_page(pbe->orig_address, pbe->address);
+
+	soft_restart_noirq(virt_to_phys(cpu_resume));
+}
+
+static u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
+
+/*
+ * Resume from the hibernation image.
+ * Due to the kernel heap / data restore, stack contents change underneath
+ * and that would make function calls impossible; switch to a temporary
+ * stack within the nosave region to avoid that problem.
+ */
+int __naked swsusp_arch_resume(void)
+{
+	extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
+	cpu_init();	/* get a clean PSR */
+	call_with_stack(__swsusp_arch_restore_image, 0,
+		__swsusp_resume_stk + sizeof(__swsusp_resume_stk));
+	return 0;
+}
diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 1f8fed9..83707702 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -611,6 +611,11 @@ config CPU_USE_DOMAINS
 config IO_36
 	bool
 
+config ARCH_HIBERNATION_POSSIBLE
+	bool
+	depends on MMU
+	default y if CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || CPU_XSCALE || CPU_XSC3 || CPU_V6 || CPU_V6K || CPU_V7
+
 comment "Processor Features"
 
 config ARM_LPAE
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index f73cabf..38bbf95 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -320,6 +320,8 @@ extern unsigned long get_safe_page(gfp_t gfp_mask);
 extern void hibernation_set_ops(const struct platform_hibernation_ops *ops);
 extern int hibernate(void);
 extern bool system_entering_hibernation(void);
+asmlinkage int swsusp_save(void);
+extern struct pbe *restore_pblist;
 #else /* CONFIG_HIBERNATION */
 static inline void register_nosave_region(unsigned long b, unsigned long e) {}
 static inline void register_nosave_region_late(unsigned long b, unsigned long e) {}
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19  1:52   ` Sebastian Capella
  (?)
@ 2014-02-19 16:12     ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-19 16:12 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Russell King, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:

[...]

> diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> new file mode 100644
> index 0000000..16f406f
> --- /dev/null
> +++ b/arch/arm/kernel/hibernate.c
> @@ -0,0 +1,106 @@
> +/*
> + * Hibernation support specific for ARM
> + *
> + * Derived from work on ARM hibernation support by:
> + *
> + * Ubuntu project, hibernation support for mach-dove
> + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
> + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
> + *  https://lkml.org/lkml/2010/6/18/4
> + *  https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
> + *  https://patchwork.kernel.org/patch/96442/
> + *
> + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/suspend.h>
> +#include <asm/tlbflush.h>
> +#include <asm/cacheflush.h>
> +#include <asm/system_misc.h>
> +#include <asm/idmap.h>
> +#include <asm/suspend.h>
> +
> +extern const void __nosave_begin, __nosave_end;
> +
> +int pfn_is_nosave(unsigned long pfn)
> +{
> +	unsigned long nosave_begin_pfn =
> +			__pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
> +	unsigned long nosave_end_pfn =
> +			PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
> +
> +	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> +}
> +
> +void notrace save_processor_state(void)
> +{
> +	WARN_ON(num_online_cpus() != 1);
> +	flush_thread();

Can you explain to me please why we need to call flush_thread() here ?
At this point in time syscore_suspend() was already called and CPU
peripheral state saved through CPU PM notifiers.

> +	local_fiq_disable();

To me it looks like we are using this hook to disable fiqs, since it is
not done in generic code.

> +}
> +
> +void notrace restore_processor_state(void)
> +{
> +	local_fiq_enable();
> +}
> +
> +/*
> + * Snapshot kernel memory and reset the system.
> + * After resume, the hibernation snapshot is written out.
> + */
> +static int notrace __swsusp_arch_save_image(unsigned long unused)
> +{
> +	int ret;
> +
> +	ret = swsusp_save();
> +	if (ret == 0)
> +		soft_restart(virt_to_phys(cpu_resume));

By the time the suspend finisher (ie this function) is run, the
processor state has been saved and I think that's all you have to do,
function can just return after calling swsusp_save(), unless I am missing
something.

I do not understand why a soft_restart is required here. On a side note,
finisher is called with irqs disabled so, since you added a function for
soft restart noirq, it should be used, if needed, but I have to understand
why in the first place.

> +	return ret;
> +}
> +
> +/*
> + * Save the current CPU state before suspend / poweroff.
> + */
> +int notrace swsusp_arch_suspend(void)
> +{
> +	return cpu_suspend(0, __swsusp_arch_save_image);

If the goal of soft_restart is to return 0 on success from this call,
you can still do that without requiring a soft_restart in the first
place. IIUC all you want to achieve is to save processor context
registers so that when you resume from image you will actually return
from here.

> +}
> +
> +/*
> + * The framework loads the hibernation image into a linked list anchored
> + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> + * destinations.
> + *
> + * To make this work if resume is triggered from initramfs, the
> + * pagetables need to be switched to allow writes to kernel mem.

Can you elaborate a bit more on this please ?

> + */
> +static void notrace __swsusp_arch_restore_image(void *unused)
> +{
> +	struct pbe *pbe;
> +
> +	cpu_switch_mm(idmap_pgd, &init_mm);

Same here, thanks.

> +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> +		copy_page(pbe->orig_address, pbe->address);
> +
> +	soft_restart_noirq(virt_to_phys(cpu_resume));

This soft_restart is justified so that you resume from the context saved
when creating the image.

> +}
> +
> +static u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
> +
> +/*
> + * Resume from the hibernation image.
> + * Due to the kernel heap / data restore, stack contents change underneath
> + * and that would make function calls impossible; switch to a temporary
> + * stack within the nosave region to avoid that problem.
> + */
> +int __naked swsusp_arch_resume(void)
> +{
> +	extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);

Ok, a function with attribute __naked that still calls C functions, is
attr __naked really needed here ?

> +	cpu_init();	/* get a clean PSR */

cpu_init is called in the cpu_resume path, why is this call needed here ?

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-19 16:12     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-19 16:12 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Russell King, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:

[...]

> diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> new file mode 100644
> index 0000000..16f406f
> --- /dev/null
> +++ b/arch/arm/kernel/hibernate.c
> @@ -0,0 +1,106 @@
> +/*
> + * Hibernation support specific for ARM
> + *
> + * Derived from work on ARM hibernation support by:
> + *
> + * Ubuntu project, hibernation support for mach-dove
> + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
> + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
> + *  https://lkml.org/lkml/2010/6/18/4
> + *  https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
> + *  https://patchwork.kernel.org/patch/96442/
> + *
> + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/suspend.h>
> +#include <asm/tlbflush.h>
> +#include <asm/cacheflush.h>
> +#include <asm/system_misc.h>
> +#include <asm/idmap.h>
> +#include <asm/suspend.h>
> +
> +extern const void __nosave_begin, __nosave_end;
> +
> +int pfn_is_nosave(unsigned long pfn)
> +{
> +	unsigned long nosave_begin_pfn =
> +			__pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
> +	unsigned long nosave_end_pfn =
> +			PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
> +
> +	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> +}
> +
> +void notrace save_processor_state(void)
> +{
> +	WARN_ON(num_online_cpus() != 1);
> +	flush_thread();

Can you explain to me please why we need to call flush_thread() here ?
At this point in time syscore_suspend() was already called and CPU
peripheral state saved through CPU PM notifiers.

> +	local_fiq_disable();

To me it looks like we are using this hook to disable fiqs, since it is
not done in generic code.

> +}
> +
> +void notrace restore_processor_state(void)
> +{
> +	local_fiq_enable();
> +}
> +
> +/*
> + * Snapshot kernel memory and reset the system.
> + * After resume, the hibernation snapshot is written out.
> + */
> +static int notrace __swsusp_arch_save_image(unsigned long unused)
> +{
> +	int ret;
> +
> +	ret = swsusp_save();
> +	if (ret == 0)
> +		soft_restart(virt_to_phys(cpu_resume));

By the time the suspend finisher (ie this function) is run, the
processor state has been saved and I think that's all you have to do,
function can just return after calling swsusp_save(), unless I am missing
something.

I do not understand why a soft_restart is required here. On a side note,
finisher is called with irqs disabled so, since you added a function for
soft restart noirq, it should be used, if needed, but I have to understand
why in the first place.

> +	return ret;
> +}
> +
> +/*
> + * Save the current CPU state before suspend / poweroff.
> + */
> +int notrace swsusp_arch_suspend(void)
> +{
> +	return cpu_suspend(0, __swsusp_arch_save_image);

If the goal of soft_restart is to return 0 on success from this call,
you can still do that without requiring a soft_restart in the first
place. IIUC all you want to achieve is to save processor context
registers so that when you resume from image you will actually return
from here.

> +}
> +
> +/*
> + * The framework loads the hibernation image into a linked list anchored
> + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> + * destinations.
> + *
> + * To make this work if resume is triggered from initramfs, the
> + * pagetables need to be switched to allow writes to kernel mem.

Can you elaborate a bit more on this please ?

> + */
> +static void notrace __swsusp_arch_restore_image(void *unused)
> +{
> +	struct pbe *pbe;
> +
> +	cpu_switch_mm(idmap_pgd, &init_mm);

Same here, thanks.

> +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> +		copy_page(pbe->orig_address, pbe->address);
> +
> +	soft_restart_noirq(virt_to_phys(cpu_resume));

This soft_restart is justified so that you resume from the context saved
when creating the image.

> +}
> +
> +static u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
> +
> +/*
> + * Resume from the hibernation image.
> + * Due to the kernel heap / data restore, stack contents change underneath
> + * and that would make function calls impossible; switch to a temporary
> + * stack within the nosave region to avoid that problem.
> + */
> +int __naked swsusp_arch_resume(void)
> +{
> +	extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);

Ok, a function with attribute __naked that still calls C functions, is
attr __naked really needed here ?

> +	cpu_init();	/* get a clean PSR */

cpu_init is called in the cpu_resume path, why is this call needed here ?

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-19 16:12     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-19 16:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:

[...]

> diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> new file mode 100644
> index 0000000..16f406f
> --- /dev/null
> +++ b/arch/arm/kernel/hibernate.c
> @@ -0,0 +1,106 @@
> +/*
> + * Hibernation support specific for ARM
> + *
> + * Derived from work on ARM hibernation support by:
> + *
> + * Ubuntu project, hibernation support for mach-dove
> + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
> + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
> + *  https://lkml.org/lkml/2010/6/18/4
> + *  https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
> + *  https://patchwork.kernel.org/patch/96442/
> + *
> + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/suspend.h>
> +#include <asm/tlbflush.h>
> +#include <asm/cacheflush.h>
> +#include <asm/system_misc.h>
> +#include <asm/idmap.h>
> +#include <asm/suspend.h>
> +
> +extern const void __nosave_begin, __nosave_end;
> +
> +int pfn_is_nosave(unsigned long pfn)
> +{
> +	unsigned long nosave_begin_pfn =
> +			__pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
> +	unsigned long nosave_end_pfn =
> +			PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
> +
> +	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> +}
> +
> +void notrace save_processor_state(void)
> +{
> +	WARN_ON(num_online_cpus() != 1);
> +	flush_thread();

Can you explain to me please why we need to call flush_thread() here ?
At this point in time syscore_suspend() was already called and CPU
peripheral state saved through CPU PM notifiers.

> +	local_fiq_disable();

To me it looks like we are using this hook to disable fiqs, since it is
not done in generic code.

> +}
> +
> +void notrace restore_processor_state(void)
> +{
> +	local_fiq_enable();
> +}
> +
> +/*
> + * Snapshot kernel memory and reset the system.
> + * After resume, the hibernation snapshot is written out.
> + */
> +static int notrace __swsusp_arch_save_image(unsigned long unused)
> +{
> +	int ret;
> +
> +	ret = swsusp_save();
> +	if (ret == 0)
> +		soft_restart(virt_to_phys(cpu_resume));

By the time the suspend finisher (ie this function) is run, the
processor state has been saved and I think that's all you have to do,
function can just return after calling swsusp_save(), unless I am missing
something.

I do not understand why a soft_restart is required here. On a side note,
finisher is called with irqs disabled so, since you added a function for
soft restart noirq, it should be used, if needed, but I have to understand
why in the first place.

> +	return ret;
> +}
> +
> +/*
> + * Save the current CPU state before suspend / poweroff.
> + */
> +int notrace swsusp_arch_suspend(void)
> +{
> +	return cpu_suspend(0, __swsusp_arch_save_image);

If the goal of soft_restart is to return 0 on success from this call,
you can still do that without requiring a soft_restart in the first
place. IIUC all you want to achieve is to save processor context
registers so that when you resume from image you will actually return
from here.

> +}
> +
> +/*
> + * The framework loads the hibernation image into a linked list anchored
> + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> + * destinations.
> + *
> + * To make this work if resume is triggered from initramfs, the
> + * pagetables need to be switched to allow writes to kernel mem.

Can you elaborate a bit more on this please ?

> + */
> +static void notrace __swsusp_arch_restore_image(void *unused)
> +{
> +	struct pbe *pbe;
> +
> +	cpu_switch_mm(idmap_pgd, &init_mm);

Same here, thanks.

> +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> +		copy_page(pbe->orig_address, pbe->address);
> +
> +	soft_restart_noirq(virt_to_phys(cpu_resume));

This soft_restart is justified so that you resume from the context saved
when creating the image.

> +}
> +
> +static u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
> +
> +/*
> + * Resume from the hibernation image.
> + * Due to the kernel heap / data restore, stack contents change underneath
> + * and that would make function calls impossible; switch to a temporary
> + * stack within the nosave region to avoid that problem.
> + */
> +int __naked swsusp_arch_resume(void)
> +{
> +	extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);

Ok, a function with attribute __naked that still calls C functions, is
attr __naked really needed here ?

> +	cpu_init();	/* get a clean PSR */

cpu_init is called in the cpu_resume path, why is this call needed here ?

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19 16:12     ` Lorenzo Pieralisi
  (?)
@ 2014-02-19 19:10       ` Russ Dill
  -1 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-19 19:10 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Sebastian Capella
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Rafael J. Wysocki, Russell King, Len Brown, Pavel Machek,
	Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin,
	Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote:

+ *  https://patchwork.kernel.org/patch/96442/


I think the idea here is to get the CPU into a state so that later
when we resume from the resume kernel, the actual CPU state matches
the state we have in kernel. The main thing flush_thread does is clear
out any and all FP state.

The may be part of the patchset that is OBE.



cpu_resume makes many assumptions about the state of the state of the
CPU, the primary being that the MMU is disabled, but also that all
caches and IRQs are disabled. soft_restart does all this for us.



ah, you are saying just return from __swsusp_arch_save_image and allow
cpu_suspend_abort to be called, placing the result of swsusp_save
somewhere else. This may work and would reduce the complexity of the
code slightly.



This is taken from the previous iteration of the patchset, I think the
comment is OBE.



But this is still required to select the right mapping for our copying.



I don't remember why I needed to prevent gcc from manipulating the
stack here.



This is another holdover from previous patch versions that may be OBE.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMFAaYACgkQfNRrxBinbdsWTwCgowSU7VPqGcM4ks39FtAihsBe
vfYAn3gxZEu753xyESuye0y1z+wbWJRp
=JZ1s
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-19 19:10       ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-19 19:10 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Sebastian Capella
  Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin,
	linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki,
	linux-kernel, Santosh Shilimkar, Pavel Machek, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd, linux-arm-kernel

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote:

+ *  https://patchwork.kernel.org/patch/96442/


I think the idea here is to get the CPU into a state so that later
when we resume from the resume kernel, the actual CPU state matches
the state we have in kernel. The main thing flush_thread does is clear
out any and all FP state.

The may be part of the patchset that is OBE.



cpu_resume makes many assumptions about the state of the state of the
CPU, the primary being that the MMU is disabled, but also that all
caches and IRQs are disabled. soft_restart does all this for us.



ah, you are saying just return from __swsusp_arch_save_image and allow
cpu_suspend_abort to be called, placing the result of swsusp_save
somewhere else. This may work and would reduce the complexity of the
code slightly.



This is taken from the previous iteration of the patchset, I think the
comment is OBE.



But this is still required to select the right mapping for our copying.



I don't remember why I needed to prevent gcc from manipulating the
stack here.



This is another holdover from previous patch versions that may be OBE.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMFAaYACgkQfNRrxBinbdsWTwCgowSU7VPqGcM4ks39FtAihsBe
vfYAn3gxZEu753xyESuye0y1z+wbWJRp
=JZ1s
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-19 19:10       ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-19 19:10 UTC (permalink / raw)
  To: linux-arm-kernel

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote:

+ *  https://patchwork.kernel.org/patch/96442/


I think the idea here is to get the CPU into a state so that later
when we resume from the resume kernel, the actual CPU state matches
the state we have in kernel. The main thing flush_thread does is clear
out any and all FP state.

The may be part of the patchset that is OBE.



cpu_resume makes many assumptions about the state of the state of the
CPU, the primary being that the MMU is disabled, but also that all
caches and IRQs are disabled. soft_restart does all this for us.



ah, you are saying just return from __swsusp_arch_save_image and allow
cpu_suspend_abort to be called, placing the result of swsusp_save
somewhere else. This may work and would reduce the complexity of the
code slightly.



This is taken from the previous iteration of the patchset, I think the
comment is OBE.



But this is still required to select the right mapping for our copying.



I don't remember why I needed to prevent gcc from manipulating the
stack here.



This is another holdover from previous patch versions that may be OBE.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMFAaYACgkQfNRrxBinbdsWTwCgowSU7VPqGcM4ks39FtAihsBe
vfYAn3gxZEu753xyESuye0y1z+wbWJRp
=JZ1s
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19 16:12     ` Lorenzo Pieralisi
@ 2014-02-19 19:33       ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19 19:33 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin,
	linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki,
	linux-kernel, Uwe Kleine-K?nig, Russ Dill, Pavel Machek,
	Cyril Chemparathy, Santosh Shilimkar, Catalin Marinas,
	Stephen Boyd, linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> [...]
> > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> > new file mode 100644
> > index 0000000..16f406f
> > --- /dev/null
> > +++ b/arch/arm/kernel/hibernate.c
> > +void notrace save_processor_state(void)
> > +{
> > +     WARN_ON(num_online_cpus() != 1);
> > +     flush_thread();
> 
> Can you explain to me please why we need to call flush_thread() here ?
> At this point in time syscore_suspend() was already called and CPU
> peripheral state saved through CPU PM notifiers.

Copying Russ' response here: 

"I think the idea here is to get the CPU into a state so that later
when we resume from the resume kernel, the actual CPU state matches
the state we have in kernel. The main thing flush_thread does is clear
out any and all FP state." - Russ Dill

> 
> > +     local_fiq_disable();
> 
> To me it looks like we are using this hook to disable fiqs, since it is
> not done in generic code.
> 
> > +
> > +/*
> > + * Snapshot kernel memory and reset the system.
> > + * After resume, the hibernation snapshot is written out.
> > + */
> > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > +{
> > +     int ret;
> > +
> > +     ret = swsusp_save();
> > +     if (ret == 0)
> > +             soft_restart(virt_to_phys(cpu_resume));
> 
> By the time the suspend finisher (ie this function) is run, the
> processor state has been saved and I think that's all you have to do,
> function can just return after calling swsusp_save(), unless I am missing
> something.
> 
> I do not understand why a soft_restart is required here.

Let me try skipping the restart, it sounds like a good idea, thanks!
I'll let you know.

> On a side note,
> finisher is called with irqs disabled so, since you added a function for
> soft restart noirq, it should be used, if needed, but I have to understand
> why in the first place.

Thanks, will have a look at this also

> > +/*
> > + * Save the current CPU state before suspend / poweroff.
> > + */
> > +int notrace swsusp_arch_suspend(void)
> > +{
> > +     return cpu_suspend(0, __swsusp_arch_save_image);
> 
> If the goal of soft_restart is to return 0 on success from this call,
> you can still do that without requiring a soft_restart in the first
> place. IIUC all you want to achieve is to save processor context
> registers so that when you resume from image you will actually return
> from here.

I think you're right, but I need to verify.  Here, we want to snapshot the
system's suspended state to the hibernation buffer, that way when we restore
the image, we resume from the saved suspended state.  This state is
already saved to the buffer after the swsusp_save call in
__swsusp_arch_save_image.  We resume and then write the saved buffer to
the resume partition.  Once we're done saving the image to the buffer I
can't think why we actually need the restart.  There's only one tricky bit
that I can think of, and that's the use of the nosave area to distinguish
the path of completing hibernation entry from resume from hibernation,
but I believe this would be handled correctly without a restart.
Let me try it out and see.

> > +/*
> > + * The framework loads the hibernation image into a linked list anchored
> > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > + * destinations.
> > + *
> > + * To make this work if resume is triggered from initramfs, the
> > + * pagetables need to be switched to allow writes to kernel mem.
> 
> Can you elaborate a bit more on this please ?
> 
> > + */
> > +static void notrace __swsusp_arch_restore_image(void *unused)
> > +{
> > +     struct pbe *pbe;
> > +
> > +     cpu_switch_mm(idmap_pgd, &init_mm);
> 
> Same here, thanks.

At restore time, we take the save buffer data and restore it to the same
physical locations used in the previous execution.  This will require having
write access to all of memory, which may not be generally granted by the
current mm.  So we switch to 1-1 init_mm to restore memory.

At hibernation entry, the system is writing only to save buffer memory,
so we don't have this problem.  It has read access to all of the memory,
and can copy to the save buffer.

Hopefully my comment above addressed this, but please let me know if I
need to provide more info.

> > +/*
> > + * Resume from the hibernation image.
> > + * Due to the kernel heap / data restore, stack contents change underneath
> > + * and that would make function calls impossible; switch to a temporary
> > + * stack within the nosave region to avoid that problem.
> > + */
> > +int __naked swsusp_arch_resume(void)
> > +{
> > +     extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
> 
> Ok, a function with attribute __naked that still calls C functions, is
> attr __naked really needed here ?

I will need to look at this more carefully and get back to you.  I can


> 
> > +     cpu_init();     /* get a clean PSR */
> 
> cpu_init is called in the cpu_resume path, why is this call needed here ?

I'll look more into this and get back with further comments.

Thanks Lorenzo!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-19 19:33       ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-19 19:33 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> [...]
> > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> > new file mode 100644
> > index 0000000..16f406f
> > --- /dev/null
> > +++ b/arch/arm/kernel/hibernate.c
> > +void notrace save_processor_state(void)
> > +{
> > +     WARN_ON(num_online_cpus() != 1);
> > +     flush_thread();
> 
> Can you explain to me please why we need to call flush_thread() here ?
> At this point in time syscore_suspend() was already called and CPU
> peripheral state saved through CPU PM notifiers.

Copying Russ' response here: 

"I think the idea here is to get the CPU into a state so that later
when we resume from the resume kernel, the actual CPU state matches
the state we have in kernel. The main thing flush_thread does is clear
out any and all FP state." - Russ Dill

> 
> > +     local_fiq_disable();
> 
> To me it looks like we are using this hook to disable fiqs, since it is
> not done in generic code.
> 
> > +
> > +/*
> > + * Snapshot kernel memory and reset the system.
> > + * After resume, the hibernation snapshot is written out.
> > + */
> > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > +{
> > +     int ret;
> > +
> > +     ret = swsusp_save();
> > +     if (ret == 0)
> > +             soft_restart(virt_to_phys(cpu_resume));
> 
> By the time the suspend finisher (ie this function) is run, the
> processor state has been saved and I think that's all you have to do,
> function can just return after calling swsusp_save(), unless I am missing
> something.
> 
> I do not understand why a soft_restart is required here.

Let me try skipping the restart, it sounds like a good idea, thanks!
I'll let you know.

> On a side note,
> finisher is called with irqs disabled so, since you added a function for
> soft restart noirq, it should be used, if needed, but I have to understand
> why in the first place.

Thanks, will have a look at this also

> > +/*
> > + * Save the current CPU state before suspend / poweroff.
> > + */
> > +int notrace swsusp_arch_suspend(void)
> > +{
> > +     return cpu_suspend(0, __swsusp_arch_save_image);
> 
> If the goal of soft_restart is to return 0 on success from this call,
> you can still do that without requiring a soft_restart in the first
> place. IIUC all you want to achieve is to save processor context
> registers so that when you resume from image you will actually return
> from here.

I think you're right, but I need to verify.  Here, we want to snapshot the
system's suspended state to the hibernation buffer, that way when we restore
the image, we resume from the saved suspended state.  This state is
already saved to the buffer after the swsusp_save call in
__swsusp_arch_save_image.  We resume and then write the saved buffer to
the resume partition.  Once we're done saving the image to the buffer I
can't think why we actually need the restart.  There's only one tricky bit
that I can think of, and that's the use of the nosave area to distinguish
the path of completing hibernation entry from resume from hibernation,
but I believe this would be handled correctly without a restart.
Let me try it out and see.

> > +/*
> > + * The framework loads the hibernation image into a linked list anchored
> > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > + * destinations.
> > + *
> > + * To make this work if resume is triggered from initramfs, the
> > + * pagetables need to be switched to allow writes to kernel mem.
> 
> Can you elaborate a bit more on this please ?
> 
> > + */
> > +static void notrace __swsusp_arch_restore_image(void *unused)
> > +{
> > +     struct pbe *pbe;
> > +
> > +     cpu_switch_mm(idmap_pgd, &init_mm);
> 
> Same here, thanks.

At restore time, we take the save buffer data and restore it to the same
physical locations used in the previous execution.  This will require having
write access to all of memory, which may not be generally granted by the
current mm.  So we switch to 1-1 init_mm to restore memory.

At hibernation entry, the system is writing only to save buffer memory,
so we don't have this problem.  It has read access to all of the memory,
and can copy to the save buffer.

Hopefully my comment above addressed this, but please let me know if I
need to provide more info.

> > +/*
> > + * Resume from the hibernation image.
> > + * Due to the kernel heap / data restore, stack contents change underneath
> > + * and that would make function calls impossible; switch to a temporary
> > + * stack within the nosave region to avoid that problem.
> > + */
> > +int __naked swsusp_arch_resume(void)
> > +{
> > +     extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
> 
> Ok, a function with attribute __naked that still calls C functions, is
> attr __naked really needed here ?

I will need to look at this more carefully and get back to you.  I can


> 
> > +     cpu_init();     /* get a clean PSR */
> 
> cpu_init is called in the cpu_resume path, why is this call needed here ?

I'll look more into this and get back with further comments.

Thanks Lorenzo!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19 19:10       ` Russ Dill
  (?)
@ 2014-02-20 10:37         ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-20 10:37 UTC (permalink / raw)
  To: Russ Dill
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Rafael J. Wysocki, Russell King, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

On Wed, Feb 19, 2014 at 07:10:31PM +0000, Russ Dill wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote:
> 
> + *  https://patchwork.kernel.org/patch/96442/
> 

I am guessing the snippets of code your comments refer to.

> I think the idea here is to get the CPU into a state so that later
> when we resume from the resume kernel, the actual CPU state matches
> the state we have in kernel. The main thing flush_thread does is clear
> out any and all FP state.

Which has already been saved through syscore_suspend()....

> The may be part of the patchset that is OBE.

It has to be updated then.

> cpu_resume makes many assumptions about the state of the state of the
> CPU, the primary being that the MMU is disabled, but also that all
> caches and IRQs are disabled. soft_restart does all this for us.
> 
> 
> 
> ah, you are saying just return from __swsusp_arch_save_image and allow
> cpu_suspend_abort to be called, placing the result of swsusp_save
> somewhere else. This may work and would reduce the complexity of the
> code slightly.

Yes. Basically you are doing a soft reboot just to return 0.

> This is taken from the previous iteration of the patchset, I think the
> comment is OBE.

Updated it please then.

> But this is still required to select the right mapping for our copying.

/me confused. Please describe what switching to idmap is meant to
achieve. In the patch above the copied swapper pgdir is not even used, I
would like to understand why this is done.

> I don't remember why I needed to prevent gcc from manipulating the
> stack here.

That's not a good reason to mark a function with attr __naked. If it is
needed we leave it there, if it is not it has to go.

> This is another holdover from previous patch versions that may be OBE.

See above.

Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-20 10:37         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-20 10:37 UTC (permalink / raw)
  To: Russ Dill
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Rafael J. Wysocki, Russell King, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

On Wed, Feb 19, 2014 at 07:10:31PM +0000, Russ Dill wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote:
> 
> + *  https://patchwork.kernel.org/patch/96442/
> 

I am guessing the snippets of code your comments refer to.

> I think the idea here is to get the CPU into a state so that later
> when we resume from the resume kernel, the actual CPU state matches
> the state we have in kernel. The main thing flush_thread does is clear
> out any and all FP state.

Which has already been saved through syscore_suspend()....

> The may be part of the patchset that is OBE.

It has to be updated then.

> cpu_resume makes many assumptions about the state of the state of the
> CPU, the primary being that the MMU is disabled, but also that all
> caches and IRQs are disabled. soft_restart does all this for us.
> 
> 
> 
> ah, you are saying just return from __swsusp_arch_save_image and allow
> cpu_suspend_abort to be called, placing the result of swsusp_save
> somewhere else. This may work and would reduce the complexity of the
> code slightly.

Yes. Basically you are doing a soft reboot just to return 0.

> This is taken from the previous iteration of the patchset, I think the
> comment is OBE.

Updated it please then.

> But this is still required to select the right mapping for our copying.

/me confused. Please describe what switching to idmap is meant to
achieve. In the patch above the copied swapper pgdir is not even used, I
would like to understand why this is done.

> I don't remember why I needed to prevent gcc from manipulating the
> stack here.

That's not a good reason to mark a function with attr __naked. If it is
needed we leave it there, if it is not it has to go.

> This is another holdover from previous patch versions that may be OBE.

See above.

Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-20 10:37         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-20 10:37 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Feb 19, 2014 at 07:10:31PM +0000, Russ Dill wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote:
> 
> + *  https://patchwork.kernel.org/patch/96442/
> 

I am guessing the snippets of code your comments refer to.

> I think the idea here is to get the CPU into a state so that later
> when we resume from the resume kernel, the actual CPU state matches
> the state we have in kernel. The main thing flush_thread does is clear
> out any and all FP state.

Which has already been saved through syscore_suspend()....

> The may be part of the patchset that is OBE.

It has to be updated then.

> cpu_resume makes many assumptions about the state of the state of the
> CPU, the primary being that the MMU is disabled, but also that all
> caches and IRQs are disabled. soft_restart does all this for us.
> 
> 
> 
> ah, you are saying just return from __swsusp_arch_save_image and allow
> cpu_suspend_abort to be called, placing the result of swsusp_save
> somewhere else. This may work and would reduce the complexity of the
> code slightly.

Yes. Basically you are doing a soft reboot just to return 0.

> This is taken from the previous iteration of the patchset, I think the
> comment is OBE.

Updated it please then.

> But this is still required to select the right mapping for our copying.

/me confused. Please describe what switching to idmap is meant to
achieve. In the patch above the copied swapper pgdir is not even used, I
would like to understand why this is done.

> I don't remember why I needed to prevent gcc from manipulating the
> stack here.

That's not a good reason to mark a function with attr __naked. If it is
needed we leave it there, if it is not it has to go.

> This is another holdover from previous patch versions that may be OBE.

See above.

Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19 19:33       ` Sebastian Capella
  (?)
@ 2014-02-20 16:27         ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-20 16:27 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Russell King, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

Hi Sebastian,

On Wed, Feb 19, 2014 at 07:33:15PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > [...]
> > > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> > > new file mode 100644
> > > index 0000000..16f406f
> > > --- /dev/null
> > > +++ b/arch/arm/kernel/hibernate.c
> > > +void notrace save_processor_state(void)
> > > +{
> > > +     WARN_ON(num_online_cpus() != 1);
> > > +     flush_thread();
> > 
> > Can you explain to me please why we need to call flush_thread() here ?
> > At this point in time syscore_suspend() was already called and CPU
> > peripheral state saved through CPU PM notifiers.
> 
> Copying Russ' response here: 
> 
> "I think the idea here is to get the CPU into a state so that later
> when we resume from the resume kernel, the actual CPU state matches
> the state we have in kernel. The main thing flush_thread does is clear
> out any and all FP state." - Russ Dill

See my reply to Russ.

[...]

> > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > +{
> > > +     struct pbe *pbe;
> > > +
> > > +     cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > Same here, thanks.
> 
> At restore time, we take the save buffer data and restore it to the same
> physical locations used in the previous execution.  This will require having
> write access to all of memory, which may not be generally granted by the
> current mm.  So we switch to 1-1 init_mm to restore memory.

I still do not understand why switching to idmap, which is a clone of
init_mm + 1:1 kernel mappings is required here. Why idmap ?

And while at it, can't the idmap be overwritten _while_ copying back the
resume kernel ? Is it safe to use idmap page tables while copying ?

I had a look at x86 and there idmap page tables used to resume are created
on the fly using safe pages, on ARM idmap is created at boot.

I am grokking the code to understand what is really needed here, will get
back to you asap but I would like things to be clarified in the interim.

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-20 16:27         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-20 16:27 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Russell King, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

Hi Sebastian,

On Wed, Feb 19, 2014 at 07:33:15PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > [...]
> > > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> > > new file mode 100644
> > > index 0000000..16f406f
> > > --- /dev/null
> > > +++ b/arch/arm/kernel/hibernate.c
> > > +void notrace save_processor_state(void)
> > > +{
> > > +     WARN_ON(num_online_cpus() != 1);
> > > +     flush_thread();
> > 
> > Can you explain to me please why we need to call flush_thread() here ?
> > At this point in time syscore_suspend() was already called and CPU
> > peripheral state saved through CPU PM notifiers.
> 
> Copying Russ' response here: 
> 
> "I think the idea here is to get the CPU into a state so that later
> when we resume from the resume kernel, the actual CPU state matches
> the state we have in kernel. The main thing flush_thread does is clear
> out any and all FP state." - Russ Dill

See my reply to Russ.

[...]

> > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > +{
> > > +     struct pbe *pbe;
> > > +
> > > +     cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > Same here, thanks.
> 
> At restore time, we take the save buffer data and restore it to the same
> physical locations used in the previous execution.  This will require having
> write access to all of memory, which may not be generally granted by the
> current mm.  So we switch to 1-1 init_mm to restore memory.

I still do not understand why switching to idmap, which is a clone of
init_mm + 1:1 kernel mappings is required here. Why idmap ?

And while at it, can't the idmap be overwritten _while_ copying back the
resume kernel ? Is it safe to use idmap page tables while copying ?

I had a look at x86 and there idmap page tables used to resume are created
on the fly using safe pages, on ARM idmap is created at boot.

I am grokking the code to understand what is really needed here, will get
back to you asap but I would like things to be clarified in the interim.

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-20 16:27         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-20 16:27 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Sebastian,

On Wed, Feb 19, 2014 at 07:33:15PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > [...]
> > > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> > > new file mode 100644
> > > index 0000000..16f406f
> > > --- /dev/null
> > > +++ b/arch/arm/kernel/hibernate.c
> > > +void notrace save_processor_state(void)
> > > +{
> > > +     WARN_ON(num_online_cpus() != 1);
> > > +     flush_thread();
> > 
> > Can you explain to me please why we need to call flush_thread() here ?
> > At this point in time syscore_suspend() was already called and CPU
> > peripheral state saved through CPU PM notifiers.
> 
> Copying Russ' response here: 
> 
> "I think the idea here is to get the CPU into a state so that later
> when we resume from the resume kernel, the actual CPU state matches
> the state we have in kernel. The main thing flush_thread does is clear
> out any and all FP state." - Russ Dill

See my reply to Russ.

[...]

> > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > +{
> > > +     struct pbe *pbe;
> > > +
> > > +     cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > Same here, thanks.
> 
> At restore time, we take the save buffer data and restore it to the same
> physical locations used in the previous execution.  This will require having
> write access to all of memory, which may not be generally granted by the
> current mm.  So we switch to 1-1 init_mm to restore memory.

I still do not understand why switching to idmap, which is a clone of
init_mm + 1:1 kernel mappings is required here. Why idmap ?

And while at it, can't the idmap be overwritten _while_ copying back the
resume kernel ? Is it safe to use idmap page tables while copying ?

I had a look at x86 and there idmap page tables used to resume are created
on the fly using safe pages, on ARM idmap is created at boot.

I am grokking the code to understand what is really needed here, will get
back to you asap but I would like things to be clarified in the interim.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19 16:12     ` Lorenzo Pieralisi
@ 2014-02-21  1:01       ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-21  1:01 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin,
	linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki,
	linux-kernel, Uwe Kleine-K?nig, Russ Dill, Pavel Machek,
	Cyril Chemparathy, Santosh Shilimkar, Catalin Marinas,
	Stephen Boyd, linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> 
> [...]
> 
> > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> > new file mode 100644
> > index 0000000..16f406f
> > --- /dev/null
> > +++ b/arch/arm/kernel/hibernate.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Hibernation support specific for ARM
> > + *
> > + * Derived from work on ARM hibernation support by:
> > + *
> > + * Ubuntu project, hibernation support for mach-dove
> > + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
> > + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
> > + *  https://lkml.org/lkml/2010/6/18/4
> > + *  https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
> > + *  https://patchwork.kernel.org/patch/96442/
> > + *
> > + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> > + *
> > + * License terms: GNU General Public License (GPL) version 2
> > + */
> > +
> > +#include <linux/mm.h>
> > +#include <linux/suspend.h>
> > +#include <asm/tlbflush.h>
> > +#include <asm/cacheflush.h>
> > +#include <asm/system_misc.h>
> > +#include <asm/idmap.h>
> > +#include <asm/suspend.h>
> > +
> > +extern const void __nosave_begin, __nosave_end;
> > +
> > +int pfn_is_nosave(unsigned long pfn)
> > +{
> > +     unsigned long nosave_begin_pfn =
> > +                     __pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
> > +     unsigned long nosave_end_pfn =
> > +                     PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
> > +
> > +     return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> > +}
> > +
> > +void notrace save_processor_state(void)
> > +{
> > +     WARN_ON(num_online_cpus() != 1);
> > +     flush_thread();
> 
> Can you explain to me please why we need to call flush_thread() here ?
> At this point in time syscore_suspend() was already called and CPU
> peripheral state saved through CPU PM notifiers.

Removed this, and everything seems to function correctly.  I'll remove this
in v2.

> > +     local_fiq_disable();
> 
> To me it looks like we are using this hook to disable fiqs, since it is
> not done in generic code.

Correct.

> > +
> > +/*
> > + * Snapshot kernel memory and reset the system.
> > + * After resume, the hibernation snapshot is written out.
> > + */
> > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > +{
> > +     int ret;
> > +
> > +     ret = swsusp_save();
> > +     if (ret == 0)
> > +             soft_restart(virt_to_phys(cpu_resume));
> 
> By the time the suspend finisher (ie this function) is run, the
> processor state has been saved and I think that's all you have to do,
> function can just return after calling swsusp_save(), unless I am missing
> something.
> 
> I do not understand why a soft_restart is required here. On a side note,
> finisher is called with irqs disabled so, since you added a function for
> soft restart noirq, it should be used, if needed, but I have to understand
> why in the first place.
	[...]
> If the goal of soft_restart is to return 0 on success from this call,
> you can still do that without requiring a soft_restart in the first
> place. IIUC all you want to achieve is to save processor context
> registers so that when you resume from image you will actually return
> from here.

Returning 0 here produces a return value of 1 to the caller of
cpu_suspend.  The return value is replaced with a literal 1 in
the cpu_suspend_abort handling.  I don't see this being used anywhere;
most places seem to consider it a suspend failure/panic/bug.  It only
happens if the finisher returns 0.

If I check for this literal in swsusp_arch_suspend, everything appears to work well.
The check is a little ugly looking.  I'll plan to add this to v2.
Please let me know if you have any suggestions.

> > +}
> > +
> > +/*
> > + * The framework loads the hibernation image into a linked list anchored
> > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > + * destinations.
> > + *
> > + * To make this work if resume is triggered from initramfs, the
> > + * pagetables need to be switched to allow writes to kernel mem.
> 
> Can you elaborate a bit more on this please ?
> 
> > + */
> > +static void notrace __swsusp_arch_restore_image(void *unused)
> > +{
> > +     struct pbe *pbe;
> > +
> > +     cpu_switch_mm(idmap_pgd, &init_mm);
> 
> Same here, thanks.

Let's follow this at the head of your comments, since we've had more
discussion on this..

> > +}
> > +
> > +static u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
> > +
> > +/*
> > + * Resume from the hibernation image.
> > + * Due to the kernel heap / data restore, stack contents change underneath
> > + * and that would make function calls impossible; switch to a temporary
> > + * stack within the nosave region to avoid that problem.
> > + */
> > +int __naked swsusp_arch_resume(void)
> > +{
> > +     extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
> 
> Ok, a function with attribute __naked that still calls C functions, is
> attr __naked really needed here ?

With the cpu_init gone, the c functions are no longer called.  I've
removed the __naked and see no issues;  I'll test overnight and see.

BTW, this function is prototyped in kernel/power/power.h as:
  extern asmlinkage int swsusp_arch_resume(void);  I'm not sure
  whether/how to use this, but thought I'd bring it up.

> 
> > +     cpu_init();     /* get a clean PSR */
> 
> cpu_init is called in the cpu_resume path, why is this call needed here ?

Removed and behaves well.

I'm running overnight testing with these changes and will add them to
v2 patch after.

Thanks for your help and comments Lorenzo

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-21  1:01       ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-21  1:01 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> 
> [...]
> 
> > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c
> > new file mode 100644
> > index 0000000..16f406f
> > --- /dev/null
> > +++ b/arch/arm/kernel/hibernate.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Hibernation support specific for ARM
> > + *
> > + * Derived from work on ARM hibernation support by:
> > + *
> > + * Ubuntu project, hibernation support for mach-dove
> > + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu)
> > + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.)
> > + *  https://lkml.org/lkml/2010/6/18/4
> > + *  https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html
> > + *  https://patchwork.kernel.org/patch/96442/
> > + *
> > + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> > + *
> > + * License terms: GNU General Public License (GPL) version 2
> > + */
> > +
> > +#include <linux/mm.h>
> > +#include <linux/suspend.h>
> > +#include <asm/tlbflush.h>
> > +#include <asm/cacheflush.h>
> > +#include <asm/system_misc.h>
> > +#include <asm/idmap.h>
> > +#include <asm/suspend.h>
> > +
> > +extern const void __nosave_begin, __nosave_end;
> > +
> > +int pfn_is_nosave(unsigned long pfn)
> > +{
> > +     unsigned long nosave_begin_pfn =
> > +                     __pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
> > +     unsigned long nosave_end_pfn =
> > +                     PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
> > +
> > +     return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> > +}
> > +
> > +void notrace save_processor_state(void)
> > +{
> > +     WARN_ON(num_online_cpus() != 1);
> > +     flush_thread();
> 
> Can you explain to me please why we need to call flush_thread() here ?
> At this point in time syscore_suspend() was already called and CPU
> peripheral state saved through CPU PM notifiers.

Removed this, and everything seems to function correctly.  I'll remove this
in v2.

> > +     local_fiq_disable();
> 
> To me it looks like we are using this hook to disable fiqs, since it is
> not done in generic code.

Correct.

> > +
> > +/*
> > + * Snapshot kernel memory and reset the system.
> > + * After resume, the hibernation snapshot is written out.
> > + */
> > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > +{
> > +     int ret;
> > +
> > +     ret = swsusp_save();
> > +     if (ret == 0)
> > +             soft_restart(virt_to_phys(cpu_resume));
> 
> By the time the suspend finisher (ie this function) is run, the
> processor state has been saved and I think that's all you have to do,
> function can just return after calling swsusp_save(), unless I am missing
> something.
> 
> I do not understand why a soft_restart is required here. On a side note,
> finisher is called with irqs disabled so, since you added a function for
> soft restart noirq, it should be used, if needed, but I have to understand
> why in the first place.
	[...]
> If the goal of soft_restart is to return 0 on success from this call,
> you can still do that without requiring a soft_restart in the first
> place. IIUC all you want to achieve is to save processor context
> registers so that when you resume from image you will actually return
> from here.

Returning 0 here produces a return value of 1 to the caller of
cpu_suspend.  The return value is replaced with a literal 1 in
the cpu_suspend_abort handling.  I don't see this being used anywhere;
most places seem to consider it a suspend failure/panic/bug.  It only
happens if the finisher returns 0.

If I check for this literal in swsusp_arch_suspend, everything appears to work well.
The check is a little ugly looking.  I'll plan to add this to v2.
Please let me know if you have any suggestions.

> > +}
> > +
> > +/*
> > + * The framework loads the hibernation image into a linked list anchored
> > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > + * destinations.
> > + *
> > + * To make this work if resume is triggered from initramfs, the
> > + * pagetables need to be switched to allow writes to kernel mem.
> 
> Can you elaborate a bit more on this please ?
> 
> > + */
> > +static void notrace __swsusp_arch_restore_image(void *unused)
> > +{
> > +     struct pbe *pbe;
> > +
> > +     cpu_switch_mm(idmap_pgd, &init_mm);
> 
> Same here, thanks.

Let's follow this at the head of your comments, since we've had more
discussion on this..

> > +}
> > +
> > +static u8 __swsusp_resume_stk[PAGE_SIZE/2] __nosavedata;
> > +
> > +/*
> > + * Resume from the hibernation image.
> > + * Due to the kernel heap / data restore, stack contents change underneath
> > + * and that would make function calls impossible; switch to a temporary
> > + * stack within the nosave region to avoid that problem.
> > + */
> > +int __naked swsusp_arch_resume(void)
> > +{
> > +     extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
> 
> Ok, a function with attribute __naked that still calls C functions, is
> attr __naked really needed here ?

With the cpu_init gone, the c functions are no longer called.  I've
removed the __naked and see no issues;  I'll test overnight and see.

BTW, this function is prototyped in kernel/power/power.h as:
  extern asmlinkage int swsusp_arch_resume(void);  I'm not sure
  whether/how to use this, but thought I'd bring it up.

> 
> > +     cpu_init();     /* get a clean PSR */
> 
> cpu_init is called in the cpu_resume path, why is this call needed here ?

Removed and behaves well.

I'm running overnight testing with these changes and will add them to
v2 patch after.

Thanks for your help and comments Lorenzo

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-20 16:27         ` Lorenzo Pieralisi
@ 2014-02-21 18:39           ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-21 18:39 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin,
	linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki,
	linux-kernel, Uwe Kleine-K?nig, Russ Dill, Pavel Machek,
	Cyril Chemparathy, Santosh Shilimkar, Catalin Marinas,
	Stephen Boyd, linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
> Hi Sebastian,
> 
> On Wed, Feb 19, 2014 at 07:33:15PM +0000, Sebastian Capella wrote:
> > Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > > +{
> > > > +     struct pbe *pbe;
> > > > +
> > > > +     cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > At restore time, we take the save buffer data and restore it to the same
> > physical locations used in the previous execution.  This will require having
> > write access to all of memory, which may not be generally granted by the
> > current mm.  So we switch to 1-1 init_mm to restore memory.

I can try removing it and seeing if there are side effects.

> 
> I still do not understand why switching to idmap, which is a clone of
> init_mm + 1:1 kernel mappings is required here. Why idmap ?
> And while at it, can't the idmap be overwritten _while_ copying back the
> resume kernel ? Is it safe to use idmap page tables while copying ?

Thanks for finding this!  I'm concerned about this now.  I still
haven't seen where this is handled.  I think we do need to switch
to a page table in safe memory, or at least make sure we're not going to
overwrite the page tables we're using.  I'll focus on this today

> I had a look at x86 and there idmap page tables used to resume are created
> on the fly using safe pages, on ARM idmap is created at boot.

After looking more at the arm code here's what I see:
- swsusp_read, will restore all hibernation pages to the same physical
  pages occupied pre-hibernation if the physical page is free. (get_buffer)
  - Since the page is free, nothing is dependent on it, it's writable
    and available, so we directly save to it, and it's ready for our
    restore.
  - if the original page is not free, we save the page to a different
    physical page that was not part of the physical page set
    pre-hibernation(safe_pages).  These pages are added to the
    restore_pblist along with the original intended physical address
    of the page.
- highmem is handled separately (pages that were not free are swapped
  and the memory map updated) (restore_himem)  Not sure if this affects
  anything, but I thought I'd mention it JIC.
- once we've read all of the hibernation image off of flash, we have a
  bunch of pages that are in the wrong place, linked at restore_pblist.
  We enter the restore image finisher, where everything is frozen,
  irq/fiq disabled and we're ready to suspend, and now we have to copy
  the pages in the wrong places back to their original physical pages.

In this state I believe:
  - our stack is safe because it's in nosave.
  - userspace is frozen, those pages are 'don't care', as we're
    discarding its state.
  - our instructions and globals are ok, because they're in the
    same place as last boot and are not getting restored.  None are in
    modules.
     - Globals: restore_pblist, idmap_pgd, init_mm, swapper_pg_dir
  - restore_pblist chain is allocated from "safe pages"
  - idmap_pgd table is allocated from freepages without worring about
    safe pages -- this is where there may be concern.  Also, not sure
    about 2nd or 3rd level pages if needed.  I'll look into this more.

static void notrace __swsusp_arch_restore_image(void *unused)
{
        struct pbe *pbe;    

        cpu_switch_mm(idmap_pgd, &init_mm);
        for (pbe = restore_pblist; pbe; pbe = pbe->next)
                copy_page(pbe->orig_address, pbe->address);
        
        soft_restart_noirq(virt_to_phys(cpu_resume));
}

As far as I see, where we are in swsusp_arch_init, the remaining risk
area is this page table.  Everything else looks ok?  Do you see any
more gaps?

Thanks Lorenzo!!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-21 18:39           ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-21 18:39 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
> Hi Sebastian,
> 
> On Wed, Feb 19, 2014 at 07:33:15PM +0000, Sebastian Capella wrote:
> > Quoting Lorenzo Pieralisi (2014-02-19 08:12:54)
> > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > > +{
> > > > +     struct pbe *pbe;
> > > > +
> > > > +     cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > At restore time, we take the save buffer data and restore it to the same
> > physical locations used in the previous execution.  This will require having
> > write access to all of memory, which may not be generally granted by the
> > current mm.  So we switch to 1-1 init_mm to restore memory.

I can try removing it and seeing if there are side effects.

> 
> I still do not understand why switching to idmap, which is a clone of
> init_mm + 1:1 kernel mappings is required here. Why idmap ?
> And while at it, can't the idmap be overwritten _while_ copying back the
> resume kernel ? Is it safe to use idmap page tables while copying ?

Thanks for finding this!  I'm concerned about this now.  I still
haven't seen where this is handled.  I think we do need to switch
to a page table in safe memory, or at least make sure we're not going to
overwrite the page tables we're using.  I'll focus on this today

> I had a look at x86 and there idmap page tables used to resume are created
> on the fly using safe pages, on ARM idmap is created at boot.

After looking more at the arm code here's what I see:
- swsusp_read, will restore all hibernation pages to the same physical
  pages occupied pre-hibernation if the physical page is free. (get_buffer)
  - Since the page is free, nothing is dependent on it, it's writable
    and available, so we directly save to it, and it's ready for our
    restore.
  - if the original page is not free, we save the page to a different
    physical page that was not part of the physical page set
    pre-hibernation(safe_pages).  These pages are added to the
    restore_pblist along with the original intended physical address
    of the page.
- highmem is handled separately (pages that were not free are swapped
  and the memory map updated) (restore_himem)  Not sure if this affects
  anything, but I thought I'd mention it JIC.
- once we've read all of the hibernation image off of flash, we have a
  bunch of pages that are in the wrong place, linked at restore_pblist.
  We enter the restore image finisher, where everything is frozen,
  irq/fiq disabled and we're ready to suspend, and now we have to copy
  the pages in the wrong places back to their original physical pages.

In this state I believe:
  - our stack is safe because it's in nosave.
  - userspace is frozen, those pages are 'don't care', as we're
    discarding its state.
  - our instructions and globals are ok, because they're in the
    same place as last boot and are not getting restored.  None are in
    modules.
     - Globals: restore_pblist, idmap_pgd, init_mm, swapper_pg_dir
  - restore_pblist chain is allocated from "safe pages"
  - idmap_pgd table is allocated from freepages without worring about
    safe pages -- this is where there may be concern.  Also, not sure
    about 2nd or 3rd level pages if needed.  I'll look into this more.

static void notrace __swsusp_arch_restore_image(void *unused)
{
        struct pbe *pbe;    

        cpu_switch_mm(idmap_pgd, &init_mm);
        for (pbe = restore_pblist; pbe; pbe = pbe->next)
                copy_page(pbe->orig_address, pbe->address);
        
        soft_restart_noirq(virt_to_phys(cpu_resume));
}

As far as I see, where we are in swsusp_arch_init, the remaining risk
area is this page table.  Everything else looks ok?  Do you see any
more gaps?

Thanks Lorenzo!!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-21 18:39           ` Sebastian Capella
@ 2014-02-21 23:59             ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-21 23:59 UTC (permalink / raw)
  To: Sebastian Capella, Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin,
	linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki,
	linux-kernel, Uwe Kleine-K?nig, Russ Dill, Pavel Machek,
	Catalin Marinas, Santosh Shilimkar, Stephen Boyd,
	linux-arm-kernel

- Cyril Chemparathy as his email is bouncing back to me. 

Quoting Sebastian Capella (2014-02-21 10:39:56)
> Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
> > > > > +     cpu_switch_mm(idmap_pgd, &init_mm);
  [ ... ]
> I can try removing it and seeing if there are side effects.

FYI, It's definitely hanging with this removed, still looking.

> In this state I believe:
>   - our stack is safe because it's in nosave.
>   - userspace is frozen, those pages are 'don't care', as we're
>     discarding its state.
>   - our instructions and globals are ok, because they're in the
>     same place as last boot and are not getting restored.  None are in
>     modules.
>      - Globals: restore_pblist, idmap_pgd, init_mm, swapper_pg_dir

Sorry, this last bit about the globals is not correct.  Of course
they're getting restored.  What I meant to say is that they shouldn't
be affected by the mmu switch as they remain at the same address.

For our purposes, I think we won't care about their state as we're not
referencing them after we start restoring.  We start the list
with restore_pblist, but after following the initial pointer, we don't
reference it again.  From then on, we're walking safe memory lists.

Sorry for the misleading statements.

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-21 23:59             ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-21 23:59 UTC (permalink / raw)
  To: linux-arm-kernel

- Cyril Chemparathy as his email is bouncing back to me. 

Quoting Sebastian Capella (2014-02-21 10:39:56)
> Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
> > > > > +     cpu_switch_mm(idmap_pgd, &init_mm);
  [ ... ]
> I can try removing it and seeing if there are side effects.

FYI, It's definitely hanging with this removed, still looking.

> In this state I believe:
>   - our stack is safe because it's in nosave.
>   - userspace is frozen, those pages are 'don't care', as we're
>     discarding its state.
>   - our instructions and globals are ok, because they're in the
>     same place as last boot and are not getting restored.  None are in
>     modules.
>      - Globals: restore_pblist, idmap_pgd, init_mm, swapper_pg_dir

Sorry, this last bit about the globals is not correct.  Of course
they're getting restored.  What I meant to say is that they shouldn't
be affected by the mmu switch as they remain at the same address.

For our purposes, I think we won't care about their state as we're not
referencing them after we start restoring.  We start the list
with restore_pblist, but after following the initial pointer, we don't
reference it again.  From then on, we're walking safe memory lists.

Sorry for the misleading statements.

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-21 23:59             ` Sebastian Capella
@ 2014-02-22  4:37               ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-22  4:37 UTC (permalink / raw)
  To: Sebastian Capella, Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin,
	linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki,
	linux-kernel, Uwe Kleine-K?nig, Russ Dill, Pavel Machek,
	Catalin Marinas, Santosh Shilimkar, Stephen Boyd,
	linux-arm-kernel

Quoting Sebastian Capella (2014-02-21 15:59:11)
> - Cyril Chemparathy as his email is bouncing back to me. 
> 
> Quoting Sebastian Capella (2014-02-21 10:39:56)
> > Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
> > > > > > +     cpu_switch_mm(idmap_pgd, &init_mm);
>   [ ... ]
> > I can try removing it and seeing if there are side effects.
> 
> FYI, It's definitely hanging with this removed, still looking.

I see when we reach this call, the 1st level page table @ TTBR0 is located
at different memory locations each run (expected).  If we omit the
cpu_switch_mm we're corrupting the page table causing the observed
intermittent failures.  I believe these match the corruption you expected.

The reason this doesn't happen when I leave the call is that idmap_pgd
is always at the same memory location.  I expect this is because it's
allocated during init.  I've seen the same address 50/50 times for
idmap_pgd.  I don't think it is correct to rely on this behavior.

Would it be appropriate to use the swapper_pg_dir directly in place
of idmap_pgd?
  - I do not see any modification to the swapper_pg_dir contents in the
    code that would change from init time.
  - swapper_pg_dir is always at the same offset.

Ideally we should have no issue with overwriting it with identical data.

I've run a couple hundred test loops using swapper_pg_dir and so far
there are no failures.

Thanks,

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22  4:37               ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-22  4:37 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Sebastian Capella (2014-02-21 15:59:11)
> - Cyril Chemparathy as his email is bouncing back to me. 
> 
> Quoting Sebastian Capella (2014-02-21 10:39:56)
> > Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
> > > > > > +     cpu_switch_mm(idmap_pgd, &init_mm);
>   [ ... ]
> > I can try removing it and seeing if there are side effects.
> 
> FYI, It's definitely hanging with this removed, still looking.

I see when we reach this call, the 1st level page table @ TTBR0 is located
at different memory locations each run (expected).  If we omit the
cpu_switch_mm we're corrupting the page table causing the observed
intermittent failures.  I believe these match the corruption you expected.

The reason this doesn't happen when I leave the call is that idmap_pgd
is always at the same memory location.  I expect this is because it's
allocated during init.  I've seen the same address 50/50 times for
idmap_pgd.  I don't think it is correct to rely on this behavior.

Would it be appropriate to use the swapper_pg_dir directly in place
of idmap_pgd?
  - I do not see any modification to the swapper_pg_dir contents in the
    code that would change from init time.
  - swapper_pg_dir is always at the same offset.

Ideally we should have no issue with overwriting it with identical data.

I've run a couple hundred test loops using swapper_pg_dir and so far
there are no failures.

Thanks,

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22  4:37               ` Sebastian Capella
  (?)
@ 2014-02-22  6:46                 ` Russ Dill
  -1 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-22  6:46 UTC (permalink / raw)
  To: Sebastian Capella, Lorenzo Pieralisi
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Rafael J. Wysocki, Russell King, Len Brown, Pavel Machek,
	Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin,
	Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

On 02/21/2014 08:37 PM, Sebastian Capella wrote:
> Quoting Sebastian Capella (2014-02-21 15:59:11)
>> - Cyril Chemparathy as his email is bouncing back to me. 
>>
>> Quoting Sebastian Capella (2014-02-21 10:39:56)
>>> Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
>>>>>>> +     cpu_switch_mm(idmap_pgd, &init_mm);
>>   [ ... ]
>>> I can try removing it and seeing if there are side effects.
>>
>> FYI, It's definitely hanging with this removed, still looking.
> 
> I see when we reach this call, the 1st level page table @ TTBR0 is located
> at different memory locations each run (expected).  If we omit the
> cpu_switch_mm we're corrupting the page table causing the observed
> intermittent failures.  I believe these match the corruption you expected.
> 
> The reason this doesn't happen when I leave the call is that idmap_pgd
> is always at the same memory location.  I expect this is because it's
> allocated during init.  I've seen the same address 50/50 times for
> idmap_pgd.  I don't think it is correct to rely on this behavior.
> 
> Would it be appropriate to use the swapper_pg_dir directly in place
> of idmap_pgd?
>   - I do not see any modification to the swapper_pg_dir contents in the
>     code that would change from init time.
>   - swapper_pg_dir is always at the same offset.
> 
> Ideally we should have no issue with overwriting it with identical data.
> 
> I've run a couple hundred test loops using swapper_pg_dir and so far
> there are no failures.

If there is worry about this, you could setup a page mapping in a
__nosave region, preventing it from being overwritten.


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22  6:46                 ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-22  6:46 UTC (permalink / raw)
  To: Sebastian Capella, Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin,
	linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki,
	linux-kernel, Santosh Shilimkar, Pavel Machek, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd, linux-arm-kernel

On 02/21/2014 08:37 PM, Sebastian Capella wrote:
> Quoting Sebastian Capella (2014-02-21 15:59:11)
>> - Cyril Chemparathy as his email is bouncing back to me. 
>>
>> Quoting Sebastian Capella (2014-02-21 10:39:56)
>>> Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
>>>>>>> +     cpu_switch_mm(idmap_pgd, &init_mm);
>>   [ ... ]
>>> I can try removing it and seeing if there are side effects.
>>
>> FYI, It's definitely hanging with this removed, still looking.
> 
> I see when we reach this call, the 1st level page table @ TTBR0 is located
> at different memory locations each run (expected).  If we omit the
> cpu_switch_mm we're corrupting the page table causing the observed
> intermittent failures.  I believe these match the corruption you expected.
> 
> The reason this doesn't happen when I leave the call is that idmap_pgd
> is always at the same memory location.  I expect this is because it's
> allocated during init.  I've seen the same address 50/50 times for
> idmap_pgd.  I don't think it is correct to rely on this behavior.
> 
> Would it be appropriate to use the swapper_pg_dir directly in place
> of idmap_pgd?
>   - I do not see any modification to the swapper_pg_dir contents in the
>     code that would change from init time.
>   - swapper_pg_dir is always at the same offset.
> 
> Ideally we should have no issue with overwriting it with identical data.
> 
> I've run a couple hundred test loops using swapper_pg_dir and so far
> there are no failures.

If there is worry about this, you could setup a page mapping in a
__nosave region, preventing it from being overwritten.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22  6:46                 ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-22  6:46 UTC (permalink / raw)
  To: linux-arm-kernel

On 02/21/2014 08:37 PM, Sebastian Capella wrote:
> Quoting Sebastian Capella (2014-02-21 15:59:11)
>> - Cyril Chemparathy as his email is bouncing back to me. 
>>
>> Quoting Sebastian Capella (2014-02-21 10:39:56)
>>> Quoting Lorenzo Pieralisi (2014-02-20 08:27:55)
>>>>>>> +     cpu_switch_mm(idmap_pgd, &init_mm);
>>   [ ... ]
>>> I can try removing it and seeing if there are side effects.
>>
>> FYI, It's definitely hanging with this removed, still looking.
> 
> I see when we reach this call, the 1st level page table @ TTBR0 is located
> at different memory locations each run (expected).  If we omit the
> cpu_switch_mm we're corrupting the page table causing the observed
> intermittent failures.  I believe these match the corruption you expected.
> 
> The reason this doesn't happen when I leave the call is that idmap_pgd
> is always at the same memory location.  I expect this is because it's
> allocated during init.  I've seen the same address 50/50 times for
> idmap_pgd.  I don't think it is correct to rely on this behavior.
> 
> Would it be appropriate to use the swapper_pg_dir directly in place
> of idmap_pgd?
>   - I do not see any modification to the swapper_pg_dir contents in the
>     code that would change from init time.
>   - swapper_pg_dir is always at the same offset.
> 
> Ideally we should have no issue with overwriting it with identical data.
> 
> I've run a couple hundred test loops using swapper_pg_dir and so far
> there are no failures.

If there is worry about this, you could setup a page mapping in a
__nosave region, preventing it from being overwritten.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-20 16:27         ` Lorenzo Pieralisi
  (?)
@ 2014-02-22 10:16           ` Russell King - ARM Linux
  -1 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:16 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> I still do not understand why switching to idmap, which is a clone of
> init_mm + 1:1 kernel mappings is required here. Why idmap ?
> 
> And while at it, can't the idmap be overwritten _while_ copying back the
> resume kernel ? Is it safe to use idmap page tables while copying ?
> 
> I had a look at x86 and there idmap page tables used to resume are created
> on the fly using safe pages, on ARM idmap is created at boot.

That's fine.

Remember, you're required to boot exactly the same kernel image when
resuming as the kernel which created the suspend image.  Unless you
have random allocations going on, you should get the same layout for
the idmap stuff at each boot.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 10:16           ` Russell King - ARM Linux
  0 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:16 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> I still do not understand why switching to idmap, which is a clone of
> init_mm + 1:1 kernel mappings is required here. Why idmap ?
> 
> And while at it, can't the idmap be overwritten _while_ copying back the
> resume kernel ? Is it safe to use idmap page tables while copying ?
> 
> I had a look at x86 and there idmap page tables used to resume are created
> on the fly using safe pages, on ARM idmap is created at boot.

That's fine.

Remember, you're required to boot exactly the same kernel image when
resuming as the kernel which created the suspend image.  Unless you
have random allocations going on, you should get the same layout for
the idmap stuff at each boot.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 10:16           ` Russell King - ARM Linux
  0 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> I still do not understand why switching to idmap, which is a clone of
> init_mm + 1:1 kernel mappings is required here. Why idmap ?
> 
> And while at it, can't the idmap be overwritten _while_ copying back the
> resume kernel ? Is it safe to use idmap page tables while copying ?
> 
> I had a look at x86 and there idmap page tables used to resume are created
> on the fly using safe pages, on ARM idmap is created at boot.

That's fine.

Remember, you're required to boot exactly the same kernel image when
resuming as the kernel which created the suspend image.  Unless you
have random allocations going on, you should get the same layout for
the idmap stuff at each boot.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22  6:46                 ` Russ Dill
  (?)
@ 2014-02-22 10:22                   ` Russell King - ARM Linux
  -1 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:22 UTC (permalink / raw)
  To: Russ Dill
  Cc: Sebastian Capella, Lorenzo Pieralisi, linux-kernel, linux-pm,
	linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

On Fri, Feb 21, 2014 at 10:46:59PM -0800, Russ Dill wrote:
> If there is worry about this, you could setup a page mapping in a
> __nosave region, preventing it from being overwritten.

Why would we need _another_ set of pages tables?  Aren't two (swapper_pg_dir
and the idmap one) enough?

You do need to switch to the idmap one if you're going to call cpu_resume
at its physical address, because that requires the identity mapping in
order to work.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 10:22                   ` Russell King - ARM Linux
  0 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:22 UTC (permalink / raw)
  To: Russ Dill
  Cc: Sebastian Capella, Lorenzo Pieralisi, linux-kernel, linux-pm,
	linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

On Fri, Feb 21, 2014 at 10:46:59PM -0800, Russ Dill wrote:
> If there is worry about this, you could setup a page mapping in a
> __nosave region, preventing it from being overwritten.

Why would we need _another_ set of pages tables?  Aren't two (swapper_pg_dir
and the idmap one) enough?

You do need to switch to the idmap one if you're going to call cpu_resume
at its physical address, because that requires the identity mapping in
order to work.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 10:22                   ` Russell King - ARM Linux
  0 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Feb 21, 2014 at 10:46:59PM -0800, Russ Dill wrote:
> If there is worry about this, you could setup a page mapping in a
> __nosave region, preventing it from being overwritten.

Why would we need _another_ set of pages tables?  Aren't two (swapper_pg_dir
and the idmap one) enough?

You do need to switch to the idmap one if you're going to call cpu_resume
at its physical address, because that requires the identity mapping in
order to work.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-19  1:52   ` Sebastian Capella
@ 2014-02-22 10:26     ` Russell King - ARM Linux
  -1 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:26 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Andrew Morton, Thomas Gleixner, Will Deacon,
	Robin Holt

On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> From: Russ Dill <Russ.Dill@ti.com>
> 
> This adds the ability to run soft_restart with local_irq/fiq_disable
> already called. This is helpful for the hibernation code paths.

I'd rather keep this simple.  There's no problem with calling soft_restart
with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
there should be harmless.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-22 10:26     ` Russell King - ARM Linux
  0 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> From: Russ Dill <Russ.Dill@ti.com>
> 
> This adds the ability to run soft_restart with local_irq/fiq_disable
> already called. This is helpful for the hibernation code paths.

I'd rather keep this simple.  There's no problem with calling soft_restart
with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
there should be harmless.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-19 16:12     ` Lorenzo Pieralisi
  (?)
@ 2014-02-22 10:38       ` Russell King - ARM Linux
  -1 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:38 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > +/*
> > + * Snapshot kernel memory and reset the system.
> > + * After resume, the hibernation snapshot is written out.
> > + */
> > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > +{
> > +	int ret;
> > +
> > +	ret = swsusp_save();
> > +	if (ret == 0)
> > +		soft_restart(virt_to_phys(cpu_resume));
> 
> By the time the suspend finisher (ie this function) is run, the
> processor state has been saved and I think that's all you have to do,
> function can just return after calling swsusp_save(), unless I am missing
> something.
> 
> I do not understand why a soft_restart is required here. On a side note,
> finisher is called with irqs disabled so, since you added a function for
> soft restart noirq, it should be used, if needed, but I have to understand
> why in the first place.

It's required because you can't just return from the finisher.  A normal
return from the finisher will always be interpreted as an abort rather
than success (because the state has to be unwound.)

This is the only way to get a zero return from cpu_suspend().

> > +/*
> > + * The framework loads the hibernation image into a linked list anchored
> > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > + * destinations.
> > + *
> > + * To make this work if resume is triggered from initramfs, the
> > + * pagetables need to be switched to allow writes to kernel mem.
> 
> Can you elaborate a bit more on this please ?
> 
> > + */
> > +static void notrace __swsusp_arch_restore_image(void *unused)
> > +{
> > +	struct pbe *pbe;
> > +
> > +	cpu_switch_mm(idmap_pgd, &init_mm);
> 
> Same here, thanks.
> 
> > +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> > +		copy_page(pbe->orig_address, pbe->address);
> > +
> > +	soft_restart_noirq(virt_to_phys(cpu_resume));
> 
> This soft_restart is justified so that you resume from the context saved
> when creating the image.

You need the idmap_pgd in place to call cpu_resume at it's physical
address.  Other page tables just won't do here.  It's well established
that this page table must be in place for the resume paths to work.

So yes, the comments above the function are wrong.  idmap_pgd must be
in place for cpu_resume() to work.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 10:38       ` Russell King - ARM Linux
  0 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:38 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > +/*
> > + * Snapshot kernel memory and reset the system.
> > + * After resume, the hibernation snapshot is written out.
> > + */
> > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > +{
> > +	int ret;
> > +
> > +	ret = swsusp_save();
> > +	if (ret == 0)
> > +		soft_restart(virt_to_phys(cpu_resume));
> 
> By the time the suspend finisher (ie this function) is run, the
> processor state has been saved and I think that's all you have to do,
> function can just return after calling swsusp_save(), unless I am missing
> something.
> 
> I do not understand why a soft_restart is required here. On a side note,
> finisher is called with irqs disabled so, since you added a function for
> soft restart noirq, it should be used, if needed, but I have to understand
> why in the first place.

It's required because you can't just return from the finisher.  A normal
return from the finisher will always be interpreted as an abort rather
than success (because the state has to be unwound.)

This is the only way to get a zero return from cpu_suspend().

> > +/*
> > + * The framework loads the hibernation image into a linked list anchored
> > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > + * destinations.
> > + *
> > + * To make this work if resume is triggered from initramfs, the
> > + * pagetables need to be switched to allow writes to kernel mem.
> 
> Can you elaborate a bit more on this please ?
> 
> > + */
> > +static void notrace __swsusp_arch_restore_image(void *unused)
> > +{
> > +	struct pbe *pbe;
> > +
> > +	cpu_switch_mm(idmap_pgd, &init_mm);
> 
> Same here, thanks.
> 
> > +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> > +		copy_page(pbe->orig_address, pbe->address);
> > +
> > +	soft_restart_noirq(virt_to_phys(cpu_resume));
> 
> This soft_restart is justified so that you resume from the context saved
> when creating the image.

You need the idmap_pgd in place to call cpu_resume at it's physical
address.  Other page tables just won't do here.  It's well established
that this page table must be in place for the resume paths to work.

So yes, the comments above the function are wrong.  idmap_pgd must be
in place for cpu_resume() to work.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 10:38       ` Russell King - ARM Linux
  0 siblings, 0 replies; 91+ messages in thread
From: Russell King - ARM Linux @ 2014-02-22 10:38 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > +/*
> > + * Snapshot kernel memory and reset the system.
> > + * After resume, the hibernation snapshot is written out.
> > + */
> > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > +{
> > +	int ret;
> > +
> > +	ret = swsusp_save();
> > +	if (ret == 0)
> > +		soft_restart(virt_to_phys(cpu_resume));
> 
> By the time the suspend finisher (ie this function) is run, the
> processor state has been saved and I think that's all you have to do,
> function can just return after calling swsusp_save(), unless I am missing
> something.
> 
> I do not understand why a soft_restart is required here. On a side note,
> finisher is called with irqs disabled so, since you added a function for
> soft restart noirq, it should be used, if needed, but I have to understand
> why in the first place.

It's required because you can't just return from the finisher.  A normal
return from the finisher will always be interpreted as an abort rather
than success (because the state has to be unwound.)

This is the only way to get a zero return from cpu_suspend().

> > +/*
> > + * The framework loads the hibernation image into a linked list anchored
> > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > + * destinations.
> > + *
> > + * To make this work if resume is triggered from initramfs, the
> > + * pagetables need to be switched to allow writes to kernel mem.
> 
> Can you elaborate a bit more on this please ?
> 
> > + */
> > +static void notrace __swsusp_arch_restore_image(void *unused)
> > +{
> > +	struct pbe *pbe;
> > +
> > +	cpu_switch_mm(idmap_pgd, &init_mm);
> 
> Same here, thanks.
> 
> > +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> > +		copy_page(pbe->orig_address, pbe->address);
> > +
> > +	soft_restart_noirq(virt_to_phys(cpu_resume));
> 
> This soft_restart is justified so that you resume from the context saved
> when creating the image.

You need the idmap_pgd in place to call cpu_resume at it's physical
address.  Other page tables just won't do here.  It's well established
that this page table must be in place for the resume paths to work.

So yes, the comments above the function are wrong.  idmap_pgd must be
in place for cpu_resume() to work.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22 10:38       ` Russell King - ARM Linux
  (?)
@ 2014-02-22 12:09         ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-22 12:09 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > +/*
> > > + * Snapshot kernel memory and reset the system.
> > > + * After resume, the hibernation snapshot is written out.
> > > + */
> > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = swsusp_save();
> > > +	if (ret == 0)
> > > +		soft_restart(virt_to_phys(cpu_resume));
> > 
> > By the time the suspend finisher (ie this function) is run, the
> > processor state has been saved and I think that's all you have to do,
> > function can just return after calling swsusp_save(), unless I am missing
> > something.
> > 
> > I do not understand why a soft_restart is required here. On a side note,
> > finisher is called with irqs disabled so, since you added a function for
> > soft restart noirq, it should be used, if needed, but I have to understand
> > why in the first place.
> 
> It's required because you can't just return from the finisher.  A normal
> return from the finisher will always be interpreted as an abort rather
> than success (because the state has to be unwound.)
> 
> This is the only way to get a zero return from cpu_suspend().

Yes, that's the only reason why this code is jumping to cpu_resume, since
all it is needed is to snapshot the CPU context and by the time the
finisher is called that's done. Wanted to say that soft reboot is not
useful (cache flushing and resume with MMU off), but what you are saying
is correct. We might be saving swsusp_save return value in a global
variable and just return from the finisher, but that's horrible and
given the amount of time it takes to snapshot the image to disk the
cost of this soft reboot will be dwarfed by that.

I wanted to ask and clarify why the code was written like this though, given
its complexity.

> > > +/*
> > > + * The framework loads the hibernation image into a linked list anchored
> > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > > + * destinations.
> > > + *
> > > + * To make this work if resume is triggered from initramfs, the
> > > + * pagetables need to be switched to allow writes to kernel mem.
> > 
> > Can you elaborate a bit more on this please ?
> > 
> > > + */
> > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > +{
> > > +	struct pbe *pbe;
> > > +
> > > +	cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > Same here, thanks.
> > 
> > > +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> > > +		copy_page(pbe->orig_address, pbe->address);
> > > +
> > > +	soft_restart_noirq(virt_to_phys(cpu_resume));
> > 
> > This soft_restart is justified so that you resume from the context saved
> > when creating the image.
> 
> You need the idmap_pgd in place to call cpu_resume at it's physical
> address.  Other page tables just won't do here.  It's well established
> that this page table must be in place for the resume paths to work.

Well, we do not need idmap page tables for copying the restore_pblist,
but we do need a set of tables that won't be corrupted by the copy and
idmap does the trick (I was confused because 1:1 mappings are not needed
for the copy itself).

The switch to idmap is done for us in soft_reboot anyway before jumping to
cpu_resume and that's required, as you said.

Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 12:09         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-22 12:09 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > +/*
> > > + * Snapshot kernel memory and reset the system.
> > > + * After resume, the hibernation snapshot is written out.
> > > + */
> > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = swsusp_save();
> > > +	if (ret == 0)
> > > +		soft_restart(virt_to_phys(cpu_resume));
> > 
> > By the time the suspend finisher (ie this function) is run, the
> > processor state has been saved and I think that's all you have to do,
> > function can just return after calling swsusp_save(), unless I am missing
> > something.
> > 
> > I do not understand why a soft_restart is required here. On a side note,
> > finisher is called with irqs disabled so, since you added a function for
> > soft restart noirq, it should be used, if needed, but I have to understand
> > why in the first place.
> 
> It's required because you can't just return from the finisher.  A normal
> return from the finisher will always be interpreted as an abort rather
> than success (because the state has to be unwound.)
> 
> This is the only way to get a zero return from cpu_suspend().

Yes, that's the only reason why this code is jumping to cpu_resume, since
all it is needed is to snapshot the CPU context and by the time the
finisher is called that's done. Wanted to say that soft reboot is not
useful (cache flushing and resume with MMU off), but what you are saying
is correct. We might be saving swsusp_save return value in a global
variable and just return from the finisher, but that's horrible and
given the amount of time it takes to snapshot the image to disk the
cost of this soft reboot will be dwarfed by that.

I wanted to ask and clarify why the code was written like this though, given
its complexity.

> > > +/*
> > > + * The framework loads the hibernation image into a linked list anchored
> > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > > + * destinations.
> > > + *
> > > + * To make this work if resume is triggered from initramfs, the
> > > + * pagetables need to be switched to allow writes to kernel mem.
> > 
> > Can you elaborate a bit more on this please ?
> > 
> > > + */
> > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > +{
> > > +	struct pbe *pbe;
> > > +
> > > +	cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > Same here, thanks.
> > 
> > > +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> > > +		copy_page(pbe->orig_address, pbe->address);
> > > +
> > > +	soft_restart_noirq(virt_to_phys(cpu_resume));
> > 
> > This soft_restart is justified so that you resume from the context saved
> > when creating the image.
> 
> You need the idmap_pgd in place to call cpu_resume at it's physical
> address.  Other page tables just won't do here.  It's well established
> that this page table must be in place for the resume paths to work.

Well, we do not need idmap page tables for copying the restore_pblist,
but we do need a set of tables that won't be corrupted by the copy and
idmap does the trick (I was confused because 1:1 mappings are not needed
for the copy itself).

The switch to idmap is done for us in soft_reboot anyway before jumping to
cpu_resume and that's required, as you said.

Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 12:09         ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-22 12:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > +/*
> > > + * Snapshot kernel memory and reset the system.
> > > + * After resume, the hibernation snapshot is written out.
> > > + */
> > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = swsusp_save();
> > > +	if (ret == 0)
> > > +		soft_restart(virt_to_phys(cpu_resume));
> > 
> > By the time the suspend finisher (ie this function) is run, the
> > processor state has been saved and I think that's all you have to do,
> > function can just return after calling swsusp_save(), unless I am missing
> > something.
> > 
> > I do not understand why a soft_restart is required here. On a side note,
> > finisher is called with irqs disabled so, since you added a function for
> > soft restart noirq, it should be used, if needed, but I have to understand
> > why in the first place.
> 
> It's required because you can't just return from the finisher.  A normal
> return from the finisher will always be interpreted as an abort rather
> than success (because the state has to be unwound.)
> 
> This is the only way to get a zero return from cpu_suspend().

Yes, that's the only reason why this code is jumping to cpu_resume, since
all it is needed is to snapshot the CPU context and by the time the
finisher is called that's done. Wanted to say that soft reboot is not
useful (cache flushing and resume with MMU off), but what you are saying
is correct. We might be saving swsusp_save return value in a global
variable and just return from the finisher, but that's horrible and
given the amount of time it takes to snapshot the image to disk the
cost of this soft reboot will be dwarfed by that.

I wanted to ask and clarify why the code was written like this though, given
its complexity.

> > > +/*
> > > + * The framework loads the hibernation image into a linked list anchored
> > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper
> > > + * destinations.
> > > + *
> > > + * To make this work if resume is triggered from initramfs, the
> > > + * pagetables need to be switched to allow writes to kernel mem.
> > 
> > Can you elaborate a bit more on this please ?
> > 
> > > + */
> > > +static void notrace __swsusp_arch_restore_image(void *unused)
> > > +{
> > > +	struct pbe *pbe;
> > > +
> > > +	cpu_switch_mm(idmap_pgd, &init_mm);
> > 
> > Same here, thanks.
> > 
> > > +	for (pbe = restore_pblist; pbe; pbe = pbe->next)
> > > +		copy_page(pbe->orig_address, pbe->address);
> > > +
> > > +	soft_restart_noirq(virt_to_phys(cpu_resume));
> > 
> > This soft_restart is justified so that you resume from the context saved
> > when creating the image.
> 
> You need the idmap_pgd in place to call cpu_resume at it's physical
> address.  Other page tables just won't do here.  It's well established
> that this page table must be in place for the resume paths to work.

Well, we do not need idmap page tables for copying the restore_pblist,
but we do need a set of tables that won't be corrupted by the copy and
idmap does the trick (I was confused because 1:1 mappings are not needed
for the copy itself).

The switch to idmap is done for us in soft_reboot anyway before jumping to
cpu_resume and that's required, as you said.

Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22 10:16           ` Russell King - ARM Linux
  (?)
@ 2014-02-22 12:13             ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-22 12:13 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Sat, Feb 22, 2014 at 10:16:55AM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> > I still do not understand why switching to idmap, which is a clone of
> > init_mm + 1:1 kernel mappings is required here. Why idmap ?
> > 
> > And while at it, can't the idmap be overwritten _while_ copying back the
> > resume kernel ? Is it safe to use idmap page tables while copying ?
> > 
> > I had a look at x86 and there idmap page tables used to resume are created
> > on the fly using safe pages, on ARM idmap is created at boot.
> 
> That's fine.
> 
> Remember, you're required to boot exactly the same kernel image when
> resuming as the kernel which created the suspend image.  Unless you
> have random allocations going on, you should get the same layout for
> the idmap stuff at each boot.

Thanks Russell, now that's clear. We do need a copy of page tables
that are not tampered with while copying, and idmap works well for
that.

Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 12:13             ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-22 12:13 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Len Brown,
	Pavel Machek, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Sat, Feb 22, 2014 at 10:16:55AM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> > I still do not understand why switching to idmap, which is a clone of
> > init_mm + 1:1 kernel mappings is required here. Why idmap ?
> > 
> > And while at it, can't the idmap be overwritten _while_ copying back the
> > resume kernel ? Is it safe to use idmap page tables while copying ?
> > 
> > I had a look at x86 and there idmap page tables used to resume are created
> > on the fly using safe pages, on ARM idmap is created at boot.
> 
> That's fine.
> 
> Remember, you're required to boot exactly the same kernel image when
> resuming as the kernel which created the suspend image.  Unless you
> have random allocations going on, you should get the same layout for
> the idmap stuff at each boot.

Thanks Russell, now that's clear. We do need a copy of page tables
that are not tampered with while copying, and idmap works well for
that.

Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 12:13             ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-22 12:13 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Feb 22, 2014 at 10:16:55AM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> > I still do not understand why switching to idmap, which is a clone of
> > init_mm + 1:1 kernel mappings is required here. Why idmap ?
> > 
> > And while at it, can't the idmap be overwritten _while_ copying back the
> > resume kernel ? Is it safe to use idmap page tables while copying ?
> > 
> > I had a look at x86 and there idmap page tables used to resume are created
> > on the fly using safe pages, on ARM idmap is created at boot.
> 
> That's fine.
> 
> Remember, you're required to boot exactly the same kernel image when
> resuming as the kernel which created the suspend image.  Unless you
> have random allocations going on, you should get the same layout for
> the idmap stuff at each boot.

Thanks Russell, now that's clear. We do need a copy of page tables
that are not tampered with while copying, and idmap works well for
that.

Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22 12:09         ` Lorenzo Pieralisi
  (?)
@ 2014-02-22 22:28           ` Pavel Machek
  -1 siblings, 0 replies; 91+ messages in thread
From: Pavel Machek @ 2014-02-22 22:28 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Russell King - ARM Linux, Sebastian Capella, linux-kernel,
	linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill,
	Rafael J. Wysocki, Len Brown, Nicolas Pitre, Santosh Shilimkar,
	Will Deacon, Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

Hi!

> > return from the finisher will always be interpreted as an abort rather
> > than success (because the state has to be unwound.)
> > 
> > This is the only way to get a zero return from cpu_suspend().
> 
> Yes, that's the only reason why this code is jumping to cpu_resume, since
> all it is needed is to snapshot the CPU context and by the time the
> finisher is called that's done. Wanted to say that soft reboot is not
> useful (cache flushing and resume with MMU off), but what you are saying
> is correct. We might be saving swsusp_save return value in a global
> variable and just return from the finisher, but that's horrible and
> given the amount of time it takes to snapshot the image to disk the
> cost of this soft reboot will be dwarfed by that.

I feel bad for the "global variable" trick on x86, and if you can
avoid it, please do!

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

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 22:28           ` Pavel Machek
  0 siblings, 0 replies; 91+ messages in thread
From: Pavel Machek @ 2014-02-22 22:28 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Russell King - ARM Linux, Sebastian Capella, linux-kernel,
	linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill,
	Rafael J. Wysocki, Len Brown, Nicolas Pitre, Santosh Shilimkar,
	Will Deacon, Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

Hi!

> > return from the finisher will always be interpreted as an abort rather
> > than success (because the state has to be unwound.)
> > 
> > This is the only way to get a zero return from cpu_suspend().
> 
> Yes, that's the only reason why this code is jumping to cpu_resume, since
> all it is needed is to snapshot the CPU context and by the time the
> finisher is called that's done. Wanted to say that soft reboot is not
> useful (cache flushing and resume with MMU off), but what you are saying
> is correct. We might be saving swsusp_save return value in a global
> variable and just return from the finisher, but that's horrible and
> given the amount of time it takes to snapshot the image to disk the
> cost of this soft reboot will be dwarfed by that.

I feel bad for the "global variable" trick on x86, and if you can
avoid it, please do!

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

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 22:28           ` Pavel Machek
  0 siblings, 0 replies; 91+ messages in thread
From: Pavel Machek @ 2014-02-22 22:28 UTC (permalink / raw)
  To: linux-arm-kernel

Hi!

> > return from the finisher will always be interpreted as an abort rather
> > than success (because the state has to be unwound.)
> > 
> > This is the only way to get a zero return from cpu_suspend().
> 
> Yes, that's the only reason why this code is jumping to cpu_resume, since
> all it is needed is to snapshot the CPU context and by the time the
> finisher is called that's done. Wanted to say that soft reboot is not
> useful (cache flushing and resume with MMU off), but what you are saying
> is correct. We might be saving swsusp_save return value in a global
> variable and just return from the finisher, but that's horrible and
> given the amount of time it takes to snapshot the image to disk the
> cost of this soft reboot will be dwarfed by that.

I feel bad for the "global variable" trick on x86, and if you can
avoid it, please do!

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

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22 10:16           ` Russell King - ARM Linux
  (?)
@ 2014-02-22 22:30             ` Pavel Machek
  -1 siblings, 0 replies; 91+ messages in thread
From: Pavel Machek @ 2014-02-22 22:30 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Lorenzo Pieralisi, Sebastian Capella, linux-kernel, linux-pm,
	linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki,
	Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Sat 2014-02-22 10:16:55, Russell King - ARM Linux wrote:
> On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> > I still do not understand why switching to idmap, which is a clone of
> > init_mm + 1:1 kernel mappings is required here. Why idmap ?
> > 
> > And while at it, can't the idmap be overwritten _while_ copying back the
> > resume kernel ? Is it safe to use idmap page tables while copying ?
> > 
> > I had a look at x86 and there idmap page tables used to resume are created
> > on the fly using safe pages, on ARM idmap is created at boot.
> 
> That's fine.
> 
> Remember, you're required to boot exactly the same kernel image when
> resuming as the kernel which created the suspend image.  Unless you
> have random allocations going on, you should get the same layout for
> the idmap stuff at each boot.

Actually, x86-64 is able to resume from different kernel these days,
and some day you may want to do it on arm, too. But it is definitely
not needed for inital merge.

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

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 22:30             ` Pavel Machek
  0 siblings, 0 replies; 91+ messages in thread
From: Pavel Machek @ 2014-02-22 22:30 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Lorenzo Pieralisi, Sebastian Capella, linux-kernel, linux-pm,
	linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki,
	Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon,
	Cyril Chemparathy, Jonathan Austin, Catalin Marinas,
	Uwe Kleine-K?nig, Stephen Boyd

On Sat 2014-02-22 10:16:55, Russell King - ARM Linux wrote:
> On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> > I still do not understand why switching to idmap, which is a clone of
> > init_mm + 1:1 kernel mappings is required here. Why idmap ?
> > 
> > And while at it, can't the idmap be overwritten _while_ copying back the
> > resume kernel ? Is it safe to use idmap page tables while copying ?
> > 
> > I had a look at x86 and there idmap page tables used to resume are created
> > on the fly using safe pages, on ARM idmap is created at boot.
> 
> That's fine.
> 
> Remember, you're required to boot exactly the same kernel image when
> resuming as the kernel which created the suspend image.  Unless you
> have random allocations going on, you should get the same layout for
> the idmap stuff at each boot.

Actually, x86-64 is able to resume from different kernel these days,
and some day you may want to do it on arm, too. But it is definitely
not needed for inital merge.

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

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-22 22:30             ` Pavel Machek
  0 siblings, 0 replies; 91+ messages in thread
From: Pavel Machek @ 2014-02-22 22:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat 2014-02-22 10:16:55, Russell King - ARM Linux wrote:
> On Thu, Feb 20, 2014 at 04:27:55PM +0000, Lorenzo Pieralisi wrote:
> > I still do not understand why switching to idmap, which is a clone of
> > init_mm + 1:1 kernel mappings is required here. Why idmap ?
> > 
> > And while at it, can't the idmap be overwritten _while_ copying back the
> > resume kernel ? Is it safe to use idmap page tables while copying ?
> > 
> > I had a look at x86 and there idmap page tables used to resume are created
> > on the fly using safe pages, on ARM idmap is created at boot.
> 
> That's fine.
> 
> Remember, you're required to boot exactly the same kernel image when
> resuming as the kernel which created the suspend image.  Unless you
> have random allocations going on, you should get the same layout for
> the idmap stuff at each boot.

Actually, x86-64 is able to resume from different kernel these days,
and some day you may want to do it on arm, too. But it is definitely
not needed for inital merge.

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

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22 12:09         ` Lorenzo Pieralisi
  (?)
@ 2014-02-23 19:52           ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-23 19:52 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Russell King - ARM Linux
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Len Brown, Pavel Machek,
	Nicolas Pitre, Santosh Shilimkar, Will Deacon, Cyril Chemparathy,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > > + cpu_switch_mm(idmap_pgd, &init_mm);
> >
> > You need the idmap_pgd in place to call cpu_resume at it's physical
> > address.  Other page tables just won't do here.  It's well established
> > that this page table must be in place for the resume paths to work.
> 
> Well, we do not need idmap page tables for copying the restore_pblist,
> but we do need a set of tables that won't be corrupted by the copy and
> idmap does the trick (I was confused because 1:1 mappings are not needed
> for the copy itself).
> 
> The switch to idmap is done for us in soft_reboot anyway before jumping to
> cpu_resume and that's required, as you said.

Ok, so I'll leave the cpu_switch_mm as is for the next patchset.

Thanks Lorenzo, Russ and Russell!

Sebastian



^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-23 19:52           ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-23 19:52 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Russell King - ARM Linux
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Len Brown, Pavel Machek,
	Nicolas Pitre, Santosh Shilimkar, Will Deacon, Cyril Chemparathy,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > > + cpu_switch_mm(idmap_pgd, &init_mm);
> >
> > You need the idmap_pgd in place to call cpu_resume at it's physical
> > address.  Other page tables just won't do here.  It's well established
> > that this page table must be in place for the resume paths to work.
> 
> Well, we do not need idmap page tables for copying the restore_pblist,
> but we do need a set of tables that won't be corrupted by the copy and
> idmap does the trick (I was confused because 1:1 mappings are not needed
> for the copy itself).
> 
> The switch to idmap is done for us in soft_reboot anyway before jumping to
> cpu_resume and that's required, as you said.

Ok, so I'll leave the cpu_switch_mm as is for the next patchset.

Thanks Lorenzo, Russ and Russell!

Sebastian



^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-23 19:52           ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-23 19:52 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > > + cpu_switch_mm(idmap_pgd, &init_mm);
> >
> > You need the idmap_pgd in place to call cpu_resume at it's physical
> > address.  Other page tables just won't do here.  It's well established
> > that this page table must be in place for the resume paths to work.
> 
> Well, we do not need idmap page tables for copying the restore_pblist,
> but we do need a set of tables that won't be corrupted by the copy and
> idmap does the trick (I was confused because 1:1 mappings are not needed
> for the copy itself).
> 
> The switch to idmap is done for us in soft_reboot anyway before jumping to
> cpu_resume and that's required, as you said.

Ok, so I'll leave the cpu_switch_mm as is for the next patchset.

Thanks Lorenzo, Russ and Russell!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-22 12:09         ` Lorenzo Pieralisi
  (?)
@ 2014-02-23 20:02           ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-23 20:02 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Russell King - ARM Linux
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Len Brown, Pavel Machek,
	Nicolas Pitre, Santosh Shilimkar, Will Deacon, Cyril Chemparathy,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > +/*
> > > > + * Snapshot kernel memory and reset the system.
> > > > + * After resume, the hibernation snapshot is written out.
> > > > + */
> > > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = swsusp_save();
> > > > + if (ret == 0)
> > > > +         soft_restart(virt_to_phys(cpu_resume));
> > > 
> > > By the time the suspend finisher (ie this function) is run, the
> > > processor state has been saved and I think that's all you have to do,
> > > function can just return after calling swsusp_save(), unless I am missing
> > > something.
> > > 
> > > I do not understand why a soft_restart is required here. On a side note,
> > > finisher is called with irqs disabled so, since you added a function for
> > > soft restart noirq, it should be used, if needed, but I have to understand
> > > why in the first place.
> > 
> > It's required because you can't just return from the finisher.  A normal
> > return from the finisher will always be interpreted as an abort rather
> > than success (because the state has to be unwound.)
> > 
> > This is the only way to get a zero return from cpu_suspend().
> 
> Yes, that's the only reason why this code is jumping to cpu_resume, since
> all it is needed is to snapshot the CPU context and by the time the
> finisher is called that's done. Wanted to say that soft reboot is not
> useful (cache flushing and resume with MMU off), but what you are saying
> is correct. We might be saving swsusp_save return value in a global
> variable and just return from the finisher, but that's horrible and
> given the amount of time it takes to snapshot the image to disk the
> cost of this soft reboot will be dwarfed by that.
> 
> I wanted to ask and clarify why the code was written like this though, given
> its complexity.

We could also return a constant > 1.  __cpu_suspend code will replace
a 0 return with 1 for paths exiting suspend, but will not change return
values != 0.  

cpu_suspend_abort:
        ldmia   sp!, {r1 - r3}          @ pop phys pgd, virt SP, phys
	resume fn
	teq     r0, #0
	moveq   r0, #1                  @ force non-zero value
	mov     sp, r2
	ldmfd   sp!, {r4 - r11, pc}

We could take advantage of that if we wanted, but Lorenzo pointed out
also that the relative benefit is very low since the cost of
resuming is >> soft_restart. 

I'll go with leaving the soft_restart as is unless someone feels
strongly against.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-23 20:02           ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-23 20:02 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Russell King - ARM Linux
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Russ Dill, Rafael J. Wysocki, Len Brown, Pavel Machek,
	Nicolas Pitre, Santosh Shilimkar, Will Deacon, Cyril Chemparathy,
	Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd

Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > +/*
> > > > + * Snapshot kernel memory and reset the system.
> > > > + * After resume, the hibernation snapshot is written out.
> > > > + */
> > > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = swsusp_save();
> > > > + if (ret == 0)
> > > > +         soft_restart(virt_to_phys(cpu_resume));
> > > 
> > > By the time the suspend finisher (ie this function) is run, the
> > > processor state has been saved and I think that's all you have to do,
> > > function can just return after calling swsusp_save(), unless I am missing
> > > something.
> > > 
> > > I do not understand why a soft_restart is required here. On a side note,
> > > finisher is called with irqs disabled so, since you added a function for
> > > soft restart noirq, it should be used, if needed, but I have to understand
> > > why in the first place.
> > 
> > It's required because you can't just return from the finisher.  A normal
> > return from the finisher will always be interpreted as an abort rather
> > than success (because the state has to be unwound.)
> > 
> > This is the only way to get a zero return from cpu_suspend().
> 
> Yes, that's the only reason why this code is jumping to cpu_resume, since
> all it is needed is to snapshot the CPU context and by the time the
> finisher is called that's done. Wanted to say that soft reboot is not
> useful (cache flushing and resume with MMU off), but what you are saying
> is correct. We might be saving swsusp_save return value in a global
> variable and just return from the finisher, but that's horrible and
> given the amount of time it takes to snapshot the image to disk the
> cost of this soft reboot will be dwarfed by that.
> 
> I wanted to ask and clarify why the code was written like this though, given
> its complexity.

We could also return a constant > 1.  __cpu_suspend code will replace
a 0 return with 1 for paths exiting suspend, but will not change return
values != 0.  

cpu_suspend_abort:
        ldmia   sp!, {r1 - r3}          @ pop phys pgd, virt SP, phys
	resume fn
	teq     r0, #0
	moveq   r0, #1                  @ force non-zero value
	mov     sp, r2
	ldmfd   sp!, {r4 - r11, pc}

We could take advantage of that if we wanted, but Lorenzo pointed out
also that the relative benefit is very low since the cost of
resuming is >> soft_restart. 

I'll go with leaving the soft_restart as is unless someone feels
strongly against.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-23 20:02           ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-23 20:02 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > +/*
> > > > + * Snapshot kernel memory and reset the system.
> > > > + * After resume, the hibernation snapshot is written out.
> > > > + */
> > > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = swsusp_save();
> > > > + if (ret == 0)
> > > > +         soft_restart(virt_to_phys(cpu_resume));
> > > 
> > > By the time the suspend finisher (ie this function) is run, the
> > > processor state has been saved and I think that's all you have to do,
> > > function can just return after calling swsusp_save(), unless I am missing
> > > something.
> > > 
> > > I do not understand why a soft_restart is required here. On a side note,
> > > finisher is called with irqs disabled so, since you added a function for
> > > soft restart noirq, it should be used, if needed, but I have to understand
> > > why in the first place.
> > 
> > It's required because you can't just return from the finisher.  A normal
> > return from the finisher will always be interpreted as an abort rather
> > than success (because the state has to be unwound.)
> > 
> > This is the only way to get a zero return from cpu_suspend().
> 
> Yes, that's the only reason why this code is jumping to cpu_resume, since
> all it is needed is to snapshot the CPU context and by the time the
> finisher is called that's done. Wanted to say that soft reboot is not
> useful (cache flushing and resume with MMU off), but what you are saying
> is correct. We might be saving swsusp_save return value in a global
> variable and just return from the finisher, but that's horrible and
> given the amount of time it takes to snapshot the image to disk the
> cost of this soft reboot will be dwarfed by that.
> 
> I wanted to ask and clarify why the code was written like this though, given
> its complexity.

We could also return a constant > 1.  __cpu_suspend code will replace
a 0 return with 1 for paths exiting suspend, but will not change return
values != 0.  

cpu_suspend_abort:
        ldmia   sp!, {r1 - r3}          @ pop phys pgd, virt SP, phys
	resume fn
	teq     r0, #0
	moveq   r0, #1                  @ force non-zero value
	mov     sp, r2
	ldmfd   sp!, {r4 - r11, pc}

We could take advantage of that if we wanted, but Lorenzo pointed out
also that the relative benefit is very low since the cost of
resuming is >> soft_restart. 

I'll go with leaving the soft_restart as is unless someone feels
strongly against.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 2/3] Fix hibernation restore hang in freeze_processes
  2014-02-19  1:52   ` Sebastian Capella
@ 2014-02-24  7:09     ` Ming Lei
  -1 siblings, 0 replies; 91+ messages in thread
From: Ming Lei @ 2014-02-24  7:09 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: Linux Kernel Mailing List, Linux PM List, linaro-kernel,
	linux-arm-kernel, Greg Kroah-Hartman, Russ Dill

On Wed, Feb 19, 2014 at 9:52 AM, Sebastian Capella
<sebastian.capella@linaro.org> wrote:
> During restore, pm_notifier chain are called with
> PM_RESTORE_PREPARE.  The firmware_class driver handler
> fw_pm_notify does not have a handler for this.  As a result,
> it keeps a reader on the kmod.c umhelper_sem.  During
> freeze_processes, the call to __usermodehelper_disable tries to
> take a write lock on this semaphore and hangs waiting.
>
> Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
> Cc: Russ Dill <Russ.Dill@ti.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Ming Lei <ming.lei@canonical.com>

Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 2/3] Fix hibernation restore hang in freeze_processes
@ 2014-02-24  7:09     ` Ming Lei
  0 siblings, 0 replies; 91+ messages in thread
From: Ming Lei @ 2014-02-24  7:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Feb 19, 2014 at 9:52 AM, Sebastian Capella
<sebastian.capella@linaro.org> wrote:
> During restore, pm_notifier chain are called with
> PM_RESTORE_PREPARE.  The firmware_class driver handler
> fw_pm_notify does not have a handler for this.  As a result,
> it keeps a reader on the kmod.c umhelper_sem.  During
> freeze_processes, the call to __usermodehelper_disable tries to
> take a write lock on this semaphore and hangs waiting.
>
> Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
> Cc: Russ Dill <Russ.Dill@ti.com>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Ming Lei <ming.lei@canonical.com>

Thanks,
--
Ming Lei

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-22 10:26     ` Russell King - ARM Linux
@ 2014-02-24 23:13       ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-24 23:13 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: linaro-kernel, linux-pm, Will Deacon, linux-kernel, Russ Dill,
	Robin Holt, Andrew Morton, Thomas Gleixner, linux-arm-kernel

Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> > From: Russ Dill <Russ.Dill@ti.com>
> > 
> > This adds the ability to run soft_restart with local_irq/fiq_disable
> > already called. This is helpful for the hibernation code paths.
> 
> I'd rather keep this simple.  There's no problem with calling soft_restart
> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> there should be harmless.

Hi Russell,

I'm observing a data abort loop when I replace this call:

In the local_irq_disable, it ends up calling trace_hardirqs_off
(CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
trace_hardirqs_off_caller which checks lockdep_recursion in the
current task, but we've switched to a temporary stack with the
call_with_stack, and get_current is returning NULL.  This
triggers a data abort, which calls trace_hardirqs_off
again and so on.

Do you have any suggestions here?

Thanks,

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-24 23:13       ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-24 23:13 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> > From: Russ Dill <Russ.Dill@ti.com>
> > 
> > This adds the ability to run soft_restart with local_irq/fiq_disable
> > already called. This is helpful for the hibernation code paths.
> 
> I'd rather keep this simple.  There's no problem with calling soft_restart
> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> there should be harmless.

Hi Russell,

I'm observing a data abort loop when I replace this call:

In the local_irq_disable, it ends up calling trace_hardirqs_off
(CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
trace_hardirqs_off_caller which checks lockdep_recursion in the
current task, but we've switched to a temporary stack with the
call_with_stack, and get_current is returning NULL.  This
triggers a data abort, which calls trace_hardirqs_off
again and so on.

Do you have any suggestions here?

Thanks,

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-24 23:13       ` Sebastian Capella
@ 2014-02-25  0:22         ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-25  0:22 UTC (permalink / raw)
  To: Sebastian Capella, Russell King - ARM Linux
  Cc: linaro-kernel, linux-pm, Will Deacon, linux-kernel, Russ Dill,
	Robin Holt, Andrew Morton, Thomas Gleixner, linux-arm-kernel

Quoting Sebastian Capella (2014-02-24 15:13:45)
> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> > On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> > > From: Russ Dill <Russ.Dill@ti.com>
> > > 
> > > This adds the ability to run soft_restart with local_irq/fiq_disable
> > > already called. This is helpful for the hibernation code paths.
> > 
> > I'd rather keep this simple.  There's no problem with calling soft_restart
> > with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> > there should be harmless.
> 
> Hi Russell,
> 
> I'm observing a data abort loop when I replace this call:
> 
> In the local_irq_disable, it ends up calling trace_hardirqs_off
> (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> trace_hardirqs_off_caller which checks lockdep_recursion in the
> current task, but we've switched to a temporary stack with the
> call_with_stack, and get_current is returning NULL.  This
> triggers a data abort, which calls trace_hardirqs_off
> again and so on.
> 
> Do you have any suggestions here?

I'll go ahead and send v2, leaving the soft_restart_noirq in place for
now since it's already past midnight in Surrey.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-25  0:22         ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-25  0:22 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Sebastian Capella (2014-02-24 15:13:45)
> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> > On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> > > From: Russ Dill <Russ.Dill@ti.com>
> > > 
> > > This adds the ability to run soft_restart with local_irq/fiq_disable
> > > already called. This is helpful for the hibernation code paths.
> > 
> > I'd rather keep this simple.  There's no problem with calling soft_restart
> > with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> > there should be harmless.
> 
> Hi Russell,
> 
> I'm observing a data abort loop when I replace this call:
> 
> In the local_irq_disable, it ends up calling trace_hardirqs_off
> (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> trace_hardirqs_off_caller which checks lockdep_recursion in the
> current task, but we've switched to a temporary stack with the
> call_with_stack, and get_current is returning NULL.  This
> triggers a data abort, which calls trace_hardirqs_off
> again and so on.
> 
> Do you have any suggestions here?

I'll go ahead and send v2, leaving the soft_restart_noirq in place for
now since it's already past midnight in Surrey.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-24 23:13       ` Sebastian Capella
  (?)
@ 2014-02-25  7:56         ` Russ Dill
  -1 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-25  7:56 UTC (permalink / raw)
  To: Sebastian Capella, Russell King - ARM Linux
  Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel,
	Andrew Morton, Thomas Gleixner, Will Deacon, Robin Holt

On 02/24/2014 03:13 PM, Sebastian Capella wrote:
> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
>>> From: Russ Dill <Russ.Dill@ti.com>
>>>
>>> This adds the ability to run soft_restart with local_irq/fiq_disable
>>> already called. This is helpful for the hibernation code paths.
>>
>> I'd rather keep this simple.  There's no problem with calling soft_restart
>> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
>> there should be harmless.
> 
> Hi Russell,
> 
> I'm observing a data abort loop when I replace this call:
> 
> In the local_irq_disable, it ends up calling trace_hardirqs_off
> (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> trace_hardirqs_off_caller which checks lockdep_recursion in the
> current task, but we've switched to a temporary stack with the
> call_with_stack, and get_current is returning NULL.  This
> triggers a data abort, which calls trace_hardirqs_off
> again and so on.
> 
> Do you have any suggestions here?
> 
> Thanks,
> 
> Sebastian
> 

So the alternative is to have a version of the call that calls a special
no trace version of local_irq_disable()/local_fiq_disable(). Which would
be preferable? Having a noirq version of soft_restart seems much simpler
to me.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-25  7:56         ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-25  7:56 UTC (permalink / raw)
  To: Sebastian Capella, Russell King - ARM Linux
  Cc: linaro-kernel, linux-pm, Will Deacon, linux-kernel, Robin Holt,
	Andrew Morton, Thomas Gleixner, linux-arm-kernel

On 02/24/2014 03:13 PM, Sebastian Capella wrote:
> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
>>> From: Russ Dill <Russ.Dill@ti.com>
>>>
>>> This adds the ability to run soft_restart with local_irq/fiq_disable
>>> already called. This is helpful for the hibernation code paths.
>>
>> I'd rather keep this simple.  There's no problem with calling soft_restart
>> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
>> there should be harmless.
> 
> Hi Russell,
> 
> I'm observing a data abort loop when I replace this call:
> 
> In the local_irq_disable, it ends up calling trace_hardirqs_off
> (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> trace_hardirqs_off_caller which checks lockdep_recursion in the
> current task, but we've switched to a temporary stack with the
> call_with_stack, and get_current is returning NULL.  This
> triggers a data abort, which calls trace_hardirqs_off
> again and so on.
> 
> Do you have any suggestions here?
> 
> Thanks,
> 
> Sebastian
> 

So the alternative is to have a version of the call that calls a special
no trace version of local_irq_disable()/local_fiq_disable(). Which would
be preferable? Having a noirq version of soft_restart seems much simpler
to me.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-25  7:56         ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-25  7:56 UTC (permalink / raw)
  To: linux-arm-kernel

On 02/24/2014 03:13 PM, Sebastian Capella wrote:
> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
>>> From: Russ Dill <Russ.Dill@ti.com>
>>>
>>> This adds the ability to run soft_restart with local_irq/fiq_disable
>>> already called. This is helpful for the hibernation code paths.
>>
>> I'd rather keep this simple.  There's no problem with calling soft_restart
>> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
>> there should be harmless.
> 
> Hi Russell,
> 
> I'm observing a data abort loop when I replace this call:
> 
> In the local_irq_disable, it ends up calling trace_hardirqs_off
> (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> trace_hardirqs_off_caller which checks lockdep_recursion in the
> current task, but we've switched to a temporary stack with the
> call_with_stack, and get_current is returning NULL.  This
> triggers a data abort, which calls trace_hardirqs_off
> again and so on.
> 
> Do you have any suggestions here?
> 
> Thanks,
> 
> Sebastian
> 

So the alternative is to have a version of the call that calls a special
no trace version of local_irq_disable()/local_fiq_disable(). Which would
be preferable? Having a noirq version of soft_restart seems much simpler
to me.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-25  7:56         ` Russ Dill
@ 2014-02-25 10:27           ` Thomas Gleixner
  -1 siblings, 0 replies; 91+ messages in thread
From: Thomas Gleixner @ 2014-02-25 10:27 UTC (permalink / raw)
  To: Russ Dill
  Cc: Sebastian Capella, Russell King - ARM Linux, linux-kernel,
	linux-pm, linaro-kernel, linux-arm-kernel, Andrew Morton,
	Will Deacon, Robin Holt

On Mon, 24 Feb 2014, Russ Dill wrote:
> On 02/24/2014 03:13 PM, Sebastian Capella wrote:
> > Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> >> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> >>> From: Russ Dill <Russ.Dill@ti.com>
> >>>
> >>> This adds the ability to run soft_restart with local_irq/fiq_disable
> >>> already called. This is helpful for the hibernation code paths.
> >>
> >> I'd rather keep this simple.  There's no problem with calling soft_restart
> >> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> >> there should be harmless.
> > 
> > Hi Russell,
> > 
> > I'm observing a data abort loop when I replace this call:
> > 
> > In the local_irq_disable, it ends up calling trace_hardirqs_off
> > (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> > trace_hardirqs_off_caller which checks lockdep_recursion in the
> > current task, but we've switched to a temporary stack with the
> > call_with_stack, and get_current is returning NULL.  This
> > triggers a data abort, which calls trace_hardirqs_off
> > again and so on.
> > 
> > Do you have any suggestions here?
> > 
> > Thanks,
> > 
> > Sebastian
> > 
> 
> So the alternative is to have a version of the call that calls a special
> no trace version of local_irq_disable()/local_fiq_disable(). Which would
> be preferable? Having a noirq version of soft_restart seems much simpler
> to me.

If you want escape the tracer and in that case you really want it
being on a different stack, use raw_local_irq_* which are not traced.

Thanks,

	tglx



^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-25 10:27           ` Thomas Gleixner
  0 siblings, 0 replies; 91+ messages in thread
From: Thomas Gleixner @ 2014-02-25 10:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 24 Feb 2014, Russ Dill wrote:
> On 02/24/2014 03:13 PM, Sebastian Capella wrote:
> > Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
> >> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella wrote:
> >>> From: Russ Dill <Russ.Dill@ti.com>
> >>>
> >>> This adds the ability to run soft_restart with local_irq/fiq_disable
> >>> already called. This is helpful for the hibernation code paths.
> >>
> >> I'd rather keep this simple.  There's no problem with calling soft_restart
> >> with interrupts already disabled.  local_irq_disable()/local_fiq_disable()
> >> there should be harmless.
> > 
> > Hi Russell,
> > 
> > I'm observing a data abort loop when I replace this call:
> > 
> > In the local_irq_disable, it ends up calling trace_hardirqs_off
> > (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled), which calls
> > trace_hardirqs_off_caller which checks lockdep_recursion in the
> > current task, but we've switched to a temporary stack with the
> > call_with_stack, and get_current is returning NULL.  This
> > triggers a data abort, which calls trace_hardirqs_off
> > again and so on.
> > 
> > Do you have any suggestions here?
> > 
> > Thanks,
> > 
> > Sebastian
> > 
> 
> So the alternative is to have a version of the call that calls a special
> no trace version of local_irq_disable()/local_fiq_disable(). Which would
> be preferable? Having a noirq version of soft_restart seems much simpler
> to me.

If you want escape the tracer and in that case you really want it
being on a different stack, use raw_local_irq_* which are not traced.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-23 20:02           ` Sebastian Capella
  (?)
@ 2014-02-25 11:32             ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-25 11:32 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: Russell King - ARM Linux, Len Brown, linaro-kernel,
	Catalin Marinas, Jonathan Austin, linux-pm, Will Deacon,
	Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig,
	Russ Dill, Pavel Machek, Cyril Chemparathy, Santosh Shilimkar,
	Stephen Boyd, linux-arm-kernel

On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> > On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > > +/*
> > > > > + * Snapshot kernel memory and reset the system.
> > > > > + * After resume, the hibernation snapshot is written out.
> > > > > + */
> > > > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + ret = swsusp_save();
> > > > > + if (ret == 0)
> > > > > +         soft_restart(virt_to_phys(cpu_resume));
> > > > 
> > > > By the time the suspend finisher (ie this function) is run, the
> > > > processor state has been saved and I think that's all you have to do,
> > > > function can just return after calling swsusp_save(), unless I am missing
> > > > something.
> > > > 
> > > > I do not understand why a soft_restart is required here. On a side note,
> > > > finisher is called with irqs disabled so, since you added a function for
> > > > soft restart noirq, it should be used, if needed, but I have to understand
> > > > why in the first place.
> > > 
> > > It's required because you can't just return from the finisher.  A normal
> > > return from the finisher will always be interpreted as an abort rather
> > > than success (because the state has to be unwound.)
> > > 
> > > This is the only way to get a zero return from cpu_suspend().
> > 
> > Yes, that's the only reason why this code is jumping to cpu_resume, since
> > all it is needed is to snapshot the CPU context and by the time the
> > finisher is called that's done. Wanted to say that soft reboot is not
> > useful (cache flushing and resume with MMU off), but what you are saying
> > is correct. We might be saving swsusp_save return value in a global
> > variable and just return from the finisher, but that's horrible and
> > given the amount of time it takes to snapshot the image to disk the
> > cost of this soft reboot will be dwarfed by that.
> > 
> > I wanted to ask and clarify why the code was written like this though, given
> > its complexity.
> 
> We could also return a constant > 1.  __cpu_suspend code will replace
> a 0 return with 1 for paths exiting suspend, but will not change return
> values != 0.  

Yes, we could but that's an API abuse and as I mentioned that soft_reboot is
not a massive deal, should not block your series. It is certainly
something to be benchmarked though since wiping the entire cache hierarchy
for nothing is not nifty.

> cpu_suspend_abort:
>         ldmia   sp!, {r1 - r3}          @ pop phys pgd, virt SP, phys
> 	resume fn
> 	teq     r0, #0
> 	moveq   r0, #1                  @ force non-zero value
> 	mov     sp, r2
> 	ldmfd   sp!, {r4 - r11, pc}
> 
> We could take advantage of that if we wanted, but Lorenzo pointed out
> also that the relative benefit is very low since the cost of
> resuming is >> soft_restart. 

The cost of writing to disk, to be precise. Again, this should be benchmarked.

> I'll go with leaving the soft_restart as is unless someone feels
> strongly against.

Leaving it as it is is fine for now, but should be commented, because that's
not clear why it is needed by just reading the code.

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-25 11:32             ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-25 11:32 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: Russell King - ARM Linux, Len Brown, linaro-kernel,
	Catalin Marinas, Jonathan Austin, linux-pm, Will Deacon,
	Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig,
	Russ Dill, Pavel Machek, Cyril Chemparathy, Santosh Shilimkar,
	Stephen Boyd, linux-arm-kernel

On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> > On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > > +/*
> > > > > + * Snapshot kernel memory and reset the system.
> > > > > + * After resume, the hibernation snapshot is written out.
> > > > > + */
> > > > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + ret = swsusp_save();
> > > > > + if (ret == 0)
> > > > > +         soft_restart(virt_to_phys(cpu_resume));
> > > > 
> > > > By the time the suspend finisher (ie this function) is run, the
> > > > processor state has been saved and I think that's all you have to do,
> > > > function can just return after calling swsusp_save(), unless I am missing
> > > > something.
> > > > 
> > > > I do not understand why a soft_restart is required here. On a side note,
> > > > finisher is called with irqs disabled so, since you added a function for
> > > > soft restart noirq, it should be used, if needed, but I have to understand
> > > > why in the first place.
> > > 
> > > It's required because you can't just return from the finisher.  A normal
> > > return from the finisher will always be interpreted as an abort rather
> > > than success (because the state has to be unwound.)
> > > 
> > > This is the only way to get a zero return from cpu_suspend().
> > 
> > Yes, that's the only reason why this code is jumping to cpu_resume, since
> > all it is needed is to snapshot the CPU context and by the time the
> > finisher is called that's done. Wanted to say that soft reboot is not
> > useful (cache flushing and resume with MMU off), but what you are saying
> > is correct. We might be saving swsusp_save return value in a global
> > variable and just return from the finisher, but that's horrible and
> > given the amount of time it takes to snapshot the image to disk the
> > cost of this soft reboot will be dwarfed by that.
> > 
> > I wanted to ask and clarify why the code was written like this though, given
> > its complexity.
> 
> We could also return a constant > 1.  __cpu_suspend code will replace
> a 0 return with 1 for paths exiting suspend, but will not change return
> values != 0.  

Yes, we could but that's an API abuse and as I mentioned that soft_reboot is
not a massive deal, should not block your series. It is certainly
something to be benchmarked though since wiping the entire cache hierarchy
for nothing is not nifty.

> cpu_suspend_abort:
>         ldmia   sp!, {r1 - r3}          @ pop phys pgd, virt SP, phys
> 	resume fn
> 	teq     r0, #0
> 	moveq   r0, #1                  @ force non-zero value
> 	mov     sp, r2
> 	ldmfd   sp!, {r4 - r11, pc}
> 
> We could take advantage of that if we wanted, but Lorenzo pointed out
> also that the relative benefit is very low since the cost of
> resuming is >> soft_restart. 

The cost of writing to disk, to be precise. Again, this should be benchmarked.

> I'll go with leaving the soft_restart as is unless someone feels
> strongly against.

Leaving it as it is is fine for now, but should be commented, because that's
not clear why it is needed by just reading the code.

Thanks,
Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-25 11:32             ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-25 11:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-22 04:09:10)
> > On Sat, Feb 22, 2014 at 10:38:40AM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Feb 19, 2014 at 04:12:54PM +0000, Lorenzo Pieralisi wrote:
> > > > On Wed, Feb 19, 2014 at 01:52:09AM +0000, Sebastian Capella wrote:
> > > > > +/*
> > > > > + * Snapshot kernel memory and reset the system.
> > > > > + * After resume, the hibernation snapshot is written out.
> > > > > + */
> > > > > +static int notrace __swsusp_arch_save_image(unsigned long unused)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + ret = swsusp_save();
> > > > > + if (ret == 0)
> > > > > +         soft_restart(virt_to_phys(cpu_resume));
> > > > 
> > > > By the time the suspend finisher (ie this function) is run, the
> > > > processor state has been saved and I think that's all you have to do,
> > > > function can just return after calling swsusp_save(), unless I am missing
> > > > something.
> > > > 
> > > > I do not understand why a soft_restart is required here. On a side note,
> > > > finisher is called with irqs disabled so, since you added a function for
> > > > soft restart noirq, it should be used, if needed, but I have to understand
> > > > why in the first place.
> > > 
> > > It's required because you can't just return from the finisher.  A normal
> > > return from the finisher will always be interpreted as an abort rather
> > > than success (because the state has to be unwound.)
> > > 
> > > This is the only way to get a zero return from cpu_suspend().
> > 
> > Yes, that's the only reason why this code is jumping to cpu_resume, since
> > all it is needed is to snapshot the CPU context and by the time the
> > finisher is called that's done. Wanted to say that soft reboot is not
> > useful (cache flushing and resume with MMU off), but what you are saying
> > is correct. We might be saving swsusp_save return value in a global
> > variable and just return from the finisher, but that's horrible and
> > given the amount of time it takes to snapshot the image to disk the
> > cost of this soft reboot will be dwarfed by that.
> > 
> > I wanted to ask and clarify why the code was written like this though, given
> > its complexity.
> 
> We could also return a constant > 1.  __cpu_suspend code will replace
> a 0 return with 1 for paths exiting suspend, but will not change return
> values != 0.  

Yes, we could but that's an API abuse and as I mentioned that soft_reboot is
not a massive deal, should not block your series. It is certainly
something to be benchmarked though since wiping the entire cache hierarchy
for nothing is not nifty.

> cpu_suspend_abort:
>         ldmia   sp!, {r1 - r3}          @ pop phys pgd, virt SP, phys
> 	resume fn
> 	teq     r0, #0
> 	moveq   r0, #1                  @ force non-zero value
> 	mov     sp, r2
> 	ldmfd   sp!, {r4 - r11, pc}
> 
> We could take advantage of that if we wanted, but Lorenzo pointed out
> also that the relative benefit is very low since the cost of
> resuming is >> soft_restart. 

The cost of writing to disk, to be precise. Again, this should be benchmarked.

> I'll go with leaving the soft_restart as is unless someone feels
> strongly against.

Leaving it as it is is fine for now, but should be commented, because that's
not clear why it is needed by just reading the code.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-25 10:27           ` Thomas Gleixner
  (?)
@ 2014-02-25 17:15             ` Russ Dill
  -1 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-25 17:15 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Sebastian Capella, Russell King - ARM Linux, linux-kernel,
	linux-pm, linaro-kernel, linux-arm-kernel, Andrew Morton,
	Will Deacon, Robin Holt

On 02/25/2014 02:27 AM, Thomas Gleixner wrote:
> On Mon, 24 Feb 2014, Russ Dill wrote:
>> On 02/24/2014 03:13 PM, Sebastian Capella wrote:
>>> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>>>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella
>>>> wrote:
>>>>> From: Russ Dill <Russ.Dill@ti.com>
>>>>> 
>>>>> This adds the ability to run soft_restart with
>>>>> local_irq/fiq_disable already called. This is helpful for
>>>>> the hibernation code paths.
>>>> 
>>>> I'd rather keep this simple.  There's no problem with calling
>>>> soft_restart with interrupts already disabled.
>>>> local_irq_disable()/local_fiq_disable() there should be
>>>> harmless.
>>> 
>>> Hi Russell,
>>> 
>>> I'm observing a data abort loop when I replace this call:
>>> 
>>> In the local_irq_disable, it ends up calling
>>> trace_hardirqs_off (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled),
>>> which calls trace_hardirqs_off_caller which checks
>>> lockdep_recursion in the current task, but we've switched to a
>>> temporary stack with the call_with_stack, and get_current is
>>> returning NULL.  This triggers a data abort, which calls
>>> trace_hardirqs_off again and so on.
>>> 
>>> Do you have any suggestions here?
>>> 
>>> Thanks,
>>> 
>>> Sebastian
>>> 
>> 
>> So the alternative is to have a version of the call that calls a
>> special no trace version of
>> local_irq_disable()/local_fiq_disable(). Which would be
>> preferable? Having a noirq version of soft_restart seems much
>> simpler to me.
> 
> If you want escape the tracer and in that case you really want it 
> being on a different stack, use raw_local_irq_* which are not
> traced.

So it might make sense to change soft_restart to use the
raw_local_irq_* calls.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-25 17:15             ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-25 17:15 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linaro-kernel, Russell King - ARM Linux, linux-pm, Will Deacon,
	linux-kernel, Sebastian Capella, Andrew Morton, linux-arm-kernel,
	Robin Holt

On 02/25/2014 02:27 AM, Thomas Gleixner wrote:
> On Mon, 24 Feb 2014, Russ Dill wrote:
>> On 02/24/2014 03:13 PM, Sebastian Capella wrote:
>>> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>>>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella
>>>> wrote:
>>>>> From: Russ Dill <Russ.Dill@ti.com>
>>>>> 
>>>>> This adds the ability to run soft_restart with
>>>>> local_irq/fiq_disable already called. This is helpful for
>>>>> the hibernation code paths.
>>>> 
>>>> I'd rather keep this simple.  There's no problem with calling
>>>> soft_restart with interrupts already disabled.
>>>> local_irq_disable()/local_fiq_disable() there should be
>>>> harmless.
>>> 
>>> Hi Russell,
>>> 
>>> I'm observing a data abort loop when I replace this call:
>>> 
>>> In the local_irq_disable, it ends up calling
>>> trace_hardirqs_off (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled),
>>> which calls trace_hardirqs_off_caller which checks
>>> lockdep_recursion in the current task, but we've switched to a
>>> temporary stack with the call_with_stack, and get_current is
>>> returning NULL.  This triggers a data abort, which calls
>>> trace_hardirqs_off again and so on.
>>> 
>>> Do you have any suggestions here?
>>> 
>>> Thanks,
>>> 
>>> Sebastian
>>> 
>> 
>> So the alternative is to have a version of the call that calls a
>> special no trace version of
>> local_irq_disable()/local_fiq_disable(). Which would be
>> preferable? Having a noirq version of soft_restart seems much
>> simpler to me.
> 
> If you want escape the tracer and in that case you really want it 
> being on a different stack, use raw_local_irq_* which are not
> traced.

So it might make sense to change soft_restart to use the
raw_local_irq_* calls.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-25 17:15             ` Russ Dill
  0 siblings, 0 replies; 91+ messages in thread
From: Russ Dill @ 2014-02-25 17:15 UTC (permalink / raw)
  To: linux-arm-kernel

On 02/25/2014 02:27 AM, Thomas Gleixner wrote:
> On Mon, 24 Feb 2014, Russ Dill wrote:
>> On 02/24/2014 03:13 PM, Sebastian Capella wrote:
>>> Quoting Russell King - ARM Linux (2014-02-22 02:26:17)
>>>> On Tue, Feb 18, 2014 at 05:52:07PM -0800, Sebastian Capella
>>>> wrote:
>>>>> From: Russ Dill <Russ.Dill@ti.com>
>>>>> 
>>>>> This adds the ability to run soft_restart with
>>>>> local_irq/fiq_disable already called. This is helpful for
>>>>> the hibernation code paths.
>>>> 
>>>> I'd rather keep this simple.  There's no problem with calling
>>>> soft_restart with interrupts already disabled.
>>>> local_irq_disable()/local_fiq_disable() there should be
>>>> harmless.
>>> 
>>> Hi Russell,
>>> 
>>> I'm observing a data abort loop when I replace this call:
>>> 
>>> In the local_irq_disable, it ends up calling
>>> trace_hardirqs_off (CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled),
>>> which calls trace_hardirqs_off_caller which checks
>>> lockdep_recursion in the current task, but we've switched to a
>>> temporary stack with the call_with_stack, and get_current is
>>> returning NULL.  This triggers a data abort, which calls
>>> trace_hardirqs_off again and so on.
>>> 
>>> Do you have any suggestions here?
>>> 
>>> Thanks,
>>> 
>>> Sebastian
>>> 
>> 
>> So the alternative is to have a version of the call that calls a
>> special no trace version of
>> local_irq_disable()/local_fiq_disable(). Which would be
>> preferable? Having a noirq version of soft_restart seems much
>> simpler to me.
> 
> If you want escape the tracer and in that case you really want it 
> being on a different stack, use raw_local_irq_* which are not
> traced.

So it might make sense to change soft_restart to use the
raw_local_irq_* calls.

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-25 11:32             ` Lorenzo Pieralisi
@ 2014-02-25 17:55               ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-25 17:55 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King - ARM Linux,
	Jonathan Austin, linux-pm, Catalin Marinas, Nicolas Pitre,
	Will Deacon, linux-kernel, Santosh Shilimkar, Rafael J. Wysocki,
	Pavel Machek, Uwe Kleine-K?nig, Russ Dill, Cyril Chemparathy,
	Stephen Boyd, linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-25 03:32:51)
> On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> > I'll go with leaving the soft_restart as is unless someone feels
> > strongly against.
> 
> Leaving it as it is is fine for now, but should be commented, because that's
> not clear why it is needed by just reading the code.

Hi Lorenzo,

How is something like this?

/*
 * Snapshot kernel memory and reset the system.
 * soft_restart is not technically needed, but is used
 * to get success returned from cpu_suspend.
 * After resume, the hibernation snapshot is written out.
 */
static int notrace __swsusp_arch_save_image(unsigned long unused)
{
        int ret;

        ret = swsusp_save();
        if (ret == 0)
                soft_restart(virt_to_phys(cpu_resume));
        return ret;
}

Thanks again for all of the feedback!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-25 17:55               ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-25 17:55 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-25 03:32:51)
> On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> > I'll go with leaving the soft_restart as is unless someone feels
> > strongly against.
> 
> Leaving it as it is is fine for now, but should be commented, because that's
> not clear why it is needed by just reading the code.

Hi Lorenzo,

How is something like this?

/*
 * Snapshot kernel memory and reset the system.
 * soft_restart is not technically needed, but is used
 * to get success returned from cpu_suspend.
 * After resume, the hibernation snapshot is written out.
 */
static int notrace __swsusp_arch_save_image(unsigned long unused)
{
        int ret;

        ret = swsusp_save();
        if (ret == 0)
                soft_restart(virt_to_phys(cpu_resume));
        return ret;
}

Thanks again for all of the feedback!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
  2014-02-25 17:15             ` Russ Dill
@ 2014-02-25 23:24               ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-25 23:24 UTC (permalink / raw)
  To: Russ Dill, Thomas Gleixner
  Cc: Russell King - ARM Linux, linux-kernel, linux-pm, linaro-kernel,
	linux-arm-kernel, Andrew Morton, Will Deacon, Robin Holt

Quoting Russ Dill (2014-02-25 09:15:33)
> On 02/25/2014 02:27 AM, Thomas Gleixner wrote:
> > If you want escape the tracer and in that case you really want it 
> > being on a different stack, use raw_local_irq_* which are not
> > traced.
> 
> So it might make sense to change soft_restart to use the
> raw_local_irq_* calls.

I've added this change.  So far it's working well.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart.
@ 2014-02-25 23:24               ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-25 23:24 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Russ Dill (2014-02-25 09:15:33)
> On 02/25/2014 02:27 AM, Thomas Gleixner wrote:
> > If you want escape the tracer and in that case you really want it 
> > being on a different stack, use raw_local_irq_* which are not
> > traced.
> 
> So it might make sense to change soft_restart to use the
> raw_local_irq_* calls.

I've added this change.  So far it's working well.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-25 17:55               ` Sebastian Capella
  (?)
@ 2014-02-26 10:24                 ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-26 10:24 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: Russell King - ARM Linux, Len Brown, linaro-kernel,
	Catalin Marinas, Jonathan Austin, linux-pm, Will Deacon,
	Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig,
	Russ Dill, Pavel Machek, Cyril Chemparathy, Santosh Shilimkar,
	Stephen Boyd, linux-arm-kernel

On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-25 03:32:51)
> > On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> > > I'll go with leaving the soft_restart as is unless someone feels
> > > strongly against.
> > 
> > Leaving it as it is is fine for now, but should be commented, because that's
> > not clear why it is needed by just reading the code.
> 
> Hi Lorenzo,
> 
> How is something like this?
> 
> /*
>  * Snapshot kernel memory and reset the system.

Please add:

"swsusp_save() is executed in the suspend finisher so that the CPU context
pointer and memory are part of the saved image, which is required by the
resume kernel image to restart execution from swsusp_arch_suspend()"

>  * soft_restart is not technically needed, but is used
>  * to get success returned from cpu_suspend.
>  * After resume, the hibernation snapshot is written out.

"When soft reboot completes, the hibernation snapshot is written out."

Resume is confusing since this code is resuming twice :D on image saving
and on kernel image restoration.

Lorenzo

>  */
> static int notrace __swsusp_arch_save_image(unsigned long unused)
> {
>         int ret;
> 
>         ret = swsusp_save();
>         if (ret == 0)
>                 soft_restart(virt_to_phys(cpu_resume));
>         return ret;
> }
> 
> Thanks again for all of the feedback!
> 
> Sebastian
> 


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-26 10:24                 ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-26 10:24 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: Russell King - ARM Linux, Len Brown, linaro-kernel,
	Catalin Marinas, Jonathan Austin, linux-pm, Will Deacon,
	Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig,
	Russ Dill, Pavel Machek, Cyril Chemparathy, Santosh Shilimkar,
	Stephen Boyd, linux-arm-kernel

On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-25 03:32:51)
> > On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> > > I'll go with leaving the soft_restart as is unless someone feels
> > > strongly against.
> > 
> > Leaving it as it is is fine for now, but should be commented, because that's
> > not clear why it is needed by just reading the code.
> 
> Hi Lorenzo,
> 
> How is something like this?
> 
> /*
>  * Snapshot kernel memory and reset the system.

Please add:

"swsusp_save() is executed in the suspend finisher so that the CPU context
pointer and memory are part of the saved image, which is required by the
resume kernel image to restart execution from swsusp_arch_suspend()"

>  * soft_restart is not technically needed, but is used
>  * to get success returned from cpu_suspend.
>  * After resume, the hibernation snapshot is written out.

"When soft reboot completes, the hibernation snapshot is written out."

Resume is confusing since this code is resuming twice :D on image saving
and on kernel image restoration.

Lorenzo

>  */
> static int notrace __swsusp_arch_save_image(unsigned long unused)
> {
>         int ret;
> 
>         ret = swsusp_save();
>         if (ret == 0)
>                 soft_restart(virt_to_phys(cpu_resume));
>         return ret;
> }
> 
> Thanks again for all of the feedback!
> 
> Sebastian
> 


^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-26 10:24                 ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-26 10:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-25 03:32:51)
> > On Sun, Feb 23, 2014 at 08:02:08PM +0000, Sebastian Capella wrote:
> > > I'll go with leaving the soft_restart as is unless someone feels
> > > strongly against.
> > 
> > Leaving it as it is is fine for now, but should be commented, because that's
> > not clear why it is needed by just reading the code.
> 
> Hi Lorenzo,
> 
> How is something like this?
> 
> /*
>  * Snapshot kernel memory and reset the system.

Please add:

"swsusp_save() is executed in the suspend finisher so that the CPU context
pointer and memory are part of the saved image, which is required by the
resume kernel image to restart execution from swsusp_arch_suspend()"

>  * soft_restart is not technically needed, but is used
>  * to get success returned from cpu_suspend.
>  * After resume, the hibernation snapshot is written out.

"When soft reboot completes, the hibernation snapshot is written out."

Resume is confusing since this code is resuming twice :D on image saving
and on kernel image restoration.

Lorenzo

>  */
> static int notrace __swsusp_arch_save_image(unsigned long unused)
> {
>         int ret;
> 
>         ret = swsusp_save();
>         if (ret == 0)
>                 soft_restart(virt_to_phys(cpu_resume));
>         return ret;
> }
> 
> Thanks again for all of the feedback!
> 
> Sebastian
> 

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-26 10:24                 ` Lorenzo Pieralisi
@ 2014-02-26 17:50                   ` Sebastian Capella
  -1 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-26 17:50 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Len Brown, linaro-kernel, Russell King - ARM Linux,
	Jonathan Austin, linux-pm, Catalin Marinas, Nicolas Pitre,
	Will Deacon, linux-kernel, Santosh Shilimkar, Rafael J. Wysocki,
	Pavel Machek, Uwe Kleine-K?nig, Russ Dill, Cyril Chemparathy,
	Stephen Boyd, linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-26 02:24:27)
> On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> 
> Please add:
> 
> "swsusp_save() is executed in the suspend finisher so that the CPU context
> pointer and memory are part of the saved image, which is required by the
> resume kernel image to restart execution from swsusp_arch_suspend()"
> 
> >  * soft_restart is not technically needed, but is used
> >  * to get success returned from cpu_suspend.
> >  * After resume, the hibernation snapshot is written out.
> 
> "When soft reboot completes, the hibernation snapshot is written out."
> 
> Resume is confusing since this code is resuming twice :D on image saving
> and on kernel image restoration.

Thanks Lorenzo!

Here's what I've got.

/*
 * Snapshot kernel memory and reset the system.
 *
 * swsusp_save() is executed in the suspend finisher so that the CPU
 * context pointer and memory are part of the saved image, which is
 * required by the resume kernel image to restart execution from
 * swsusp_arch_suspend().
 *
 * soft_restart is not technically needed, but is used to get success
 * returned from cpu_suspend.
 * 
 * When soft reboot completes, the hibernation snapshot is
 * written out.
 */

Does this look ok?  I'll prepare a v4 patchset.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-26 17:50                   ` Sebastian Capella
  0 siblings, 0 replies; 91+ messages in thread
From: Sebastian Capella @ 2014-02-26 17:50 UTC (permalink / raw)
  To: linux-arm-kernel

Quoting Lorenzo Pieralisi (2014-02-26 02:24:27)
> On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> 
> Please add:
> 
> "swsusp_save() is executed in the suspend finisher so that the CPU context
> pointer and memory are part of the saved image, which is required by the
> resume kernel image to restart execution from swsusp_arch_suspend()"
> 
> >  * soft_restart is not technically needed, but is used
> >  * to get success returned from cpu_suspend.
> >  * After resume, the hibernation snapshot is written out.
> 
> "When soft reboot completes, the hibernation snapshot is written out."
> 
> Resume is confusing since this code is resuming twice :D on image saving
> and on kernel image restoration.

Thanks Lorenzo!

Here's what I've got.

/*
 * Snapshot kernel memory and reset the system.
 *
 * swsusp_save() is executed in the suspend finisher so that the CPU
 * context pointer and memory are part of the saved image, which is
 * required by the resume kernel image to restart execution from
 * swsusp_arch_suspend().
 *
 * soft_restart is not technically needed, but is used to get success
 * returned from cpu_suspend.
 * 
 * When soft reboot completes, the hibernation snapshot is
 * written out.
 */

Does this look ok?  I'll prepare a v4 patchset.

Thanks!

Sebastian

^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
  2014-02-26 17:50                   ` Sebastian Capella
  (?)
@ 2014-02-26 19:03                     ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-26 19:03 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: Russell King - ARM Linux, Len Brown, linaro-kernel,
	Catalin Marinas, Jonathan Austin, linux-pm, Will Deacon,
	Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig,
	Russ Dill, Pavel Machek, Cyril Chemparathy, Santosh Shilimkar,
	Stephen Boyd, linux-arm-kernel

On Wed, Feb 26, 2014 at 05:50:55PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-26 02:24:27)
> > On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> > 
> > Please add:
> > 
> > "swsusp_save() is executed in the suspend finisher so that the CPU context
> > pointer and memory are part of the saved image, which is required by the
> > resume kernel image to restart execution from swsusp_arch_suspend()"
> > 
> > >  * soft_restart is not technically needed, but is used
> > >  * to get success returned from cpu_suspend.
> > >  * After resume, the hibernation snapshot is written out.
> > 
> > "When soft reboot completes, the hibernation snapshot is written out."
> > 
> > Resume is confusing since this code is resuming twice :D on image saving
> > and on kernel image restoration.
> 
> Thanks Lorenzo!
> 
> Here's what I've got.
> 
> /*
>  * Snapshot kernel memory and reset the system.
>  *
>  * swsusp_save() is executed in the suspend finisher so that the CPU
>  * context pointer and memory are part of the saved image, which is
>  * required by the resume kernel image to restart execution from
>  * swsusp_arch_suspend().
>  *
>  * soft_restart is not technically needed, but is used to get success
>  * returned from cpu_suspend.
>  * 
>  * When soft reboot completes, the hibernation snapshot is
>  * written out.
>  */
> 
> Does this look ok?  I'll prepare a v4 patchset.

Yes it does, I will wait and review v4 then.

Thank you,
Lorenzo


^ permalink raw reply	[flat|nested] 91+ messages in thread

* Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-26 19:03                     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-26 19:03 UTC (permalink / raw)
  To: Sebastian Capella
  Cc: Len Brown, linaro-kernel, Russell King - ARM Linux,
	Jonathan Austin, linux-pm, Catalin Marinas, Nicolas Pitre,
	Will Deacon, linux-kernel, Santosh Shilimkar, Rafael J. Wysocki,
	Pavel Machek, Uwe Kleine-K?nig, Russ Dill, Cyril Chemparathy,
	Stephen Boyd, linux-arm-kernel

On Wed, Feb 26, 2014 at 05:50:55PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-26 02:24:27)
> > On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> > 
> > Please add:
> > 
> > "swsusp_save() is executed in the suspend finisher so that the CPU context
> > pointer and memory are part of the saved image, which is required by the
> > resume kernel image to restart execution from swsusp_arch_suspend()"
> > 
> > >  * soft_restart is not technically needed, but is used
> > >  * to get success returned from cpu_suspend.
> > >  * After resume, the hibernation snapshot is written out.
> > 
> > "When soft reboot completes, the hibernation snapshot is written out."
> > 
> > Resume is confusing since this code is resuming twice :D on image saving
> > and on kernel image restoration.
> 
> Thanks Lorenzo!
> 
> Here's what I've got.
> 
> /*
>  * Snapshot kernel memory and reset the system.
>  *
>  * swsusp_save() is executed in the suspend finisher so that the CPU
>  * context pointer and memory are part of the saved image, which is
>  * required by the resume kernel image to restart execution from
>  * swsusp_arch_suspend().
>  *
>  * soft_restart is not technically needed, but is used to get success
>  * returned from cpu_suspend.
>  * 
>  * When soft reboot completes, the hibernation snapshot is
>  * written out.
>  */
> 
> Does this look ok?  I'll prepare a v4 patchset.

Yes it does, I will wait and review v4 then.

Thank you,
Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

* [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk
@ 2014-02-26 19:03                     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 91+ messages in thread
From: Lorenzo Pieralisi @ 2014-02-26 19:03 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Feb 26, 2014 at 05:50:55PM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-26 02:24:27)
> > On Tue, Feb 25, 2014 at 05:55:31PM +0000, Sebastian Capella wrote:
> > 
> > Please add:
> > 
> > "swsusp_save() is executed in the suspend finisher so that the CPU context
> > pointer and memory are part of the saved image, which is required by the
> > resume kernel image to restart execution from swsusp_arch_suspend()"
> > 
> > >  * soft_restart is not technically needed, but is used
> > >  * to get success returned from cpu_suspend.
> > >  * After resume, the hibernation snapshot is written out.
> > 
> > "When soft reboot completes, the hibernation snapshot is written out."
> > 
> > Resume is confusing since this code is resuming twice :D on image saving
> > and on kernel image restoration.
> 
> Thanks Lorenzo!
> 
> Here's what I've got.
> 
> /*
>  * Snapshot kernel memory and reset the system.
>  *
>  * swsusp_save() is executed in the suspend finisher so that the CPU
>  * context pointer and memory are part of the saved image, which is
>  * required by the resume kernel image to restart execution from
>  * swsusp_arch_suspend().
>  *
>  * soft_restart is not technically needed, but is used to get success
>  * returned from cpu_suspend.
>  * 
>  * When soft reboot completes, the hibernation snapshot is
>  * written out.
>  */
> 
> Does this look ok?  I'll prepare a v4 patchset.

Yes it does, I will wait and review v4 then.

Thank you,
Lorenzo

^ permalink raw reply	[flat|nested] 91+ messages in thread

end of thread, other threads:[~2014-02-26 19:03 UTC | newest]

Thread overview: 91+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-19  1:52 [PATCH RFC v1 0/3] hibernation support on ARM Sebastian Capella
2014-02-19  1:52 ` Sebastian Capella
2014-02-19  1:52 ` [PATCH RFC v1 1/3] ARM: Add irq disabled version of soft_restart Sebastian Capella
2014-02-19  1:52   ` Sebastian Capella
2014-02-22 10:26   ` Russell King - ARM Linux
2014-02-22 10:26     ` Russell King - ARM Linux
2014-02-24 23:13     ` Sebastian Capella
2014-02-24 23:13       ` Sebastian Capella
2014-02-25  0:22       ` Sebastian Capella
2014-02-25  0:22         ` Sebastian Capella
2014-02-25  7:56       ` Russ Dill
2014-02-25  7:56         ` Russ Dill
2014-02-25  7:56         ` Russ Dill
2014-02-25 10:27         ` Thomas Gleixner
2014-02-25 10:27           ` Thomas Gleixner
2014-02-25 17:15           ` Russ Dill
2014-02-25 17:15             ` Russ Dill
2014-02-25 17:15             ` Russ Dill
2014-02-25 23:24             ` Sebastian Capella
2014-02-25 23:24               ` Sebastian Capella
2014-02-19  1:52 ` [PATCH RFC v1 2/3] Fix hibernation restore hang in freeze_processes Sebastian Capella
2014-02-19  1:52   ` Sebastian Capella
2014-02-24  7:09   ` Ming Lei
2014-02-24  7:09     ` Ming Lei
2014-02-19  1:52 ` [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk Sebastian Capella
2014-02-19  1:52   ` Sebastian Capella
2014-02-19 16:12   ` Lorenzo Pieralisi
2014-02-19 16:12     ` Lorenzo Pieralisi
2014-02-19 16:12     ` Lorenzo Pieralisi
2014-02-19 19:10     ` Russ Dill
2014-02-19 19:10       ` Russ Dill
2014-02-19 19:10       ` Russ Dill
2014-02-20 10:37       ` Lorenzo Pieralisi
2014-02-20 10:37         ` Lorenzo Pieralisi
2014-02-20 10:37         ` Lorenzo Pieralisi
2014-02-19 19:33     ` Sebastian Capella
2014-02-19 19:33       ` Sebastian Capella
2014-02-20 16:27       ` Lorenzo Pieralisi
2014-02-20 16:27         ` Lorenzo Pieralisi
2014-02-20 16:27         ` Lorenzo Pieralisi
2014-02-21 18:39         ` Sebastian Capella
2014-02-21 18:39           ` Sebastian Capella
2014-02-21 23:59           ` Sebastian Capella
2014-02-21 23:59             ` Sebastian Capella
2014-02-22  4:37             ` Sebastian Capella
2014-02-22  4:37               ` Sebastian Capella
2014-02-22  6:46               ` Russ Dill
2014-02-22  6:46                 ` Russ Dill
2014-02-22  6:46                 ` Russ Dill
2014-02-22 10:22                 ` Russell King - ARM Linux
2014-02-22 10:22                   ` Russell King - ARM Linux
2014-02-22 10:22                   ` Russell King - ARM Linux
2014-02-22 10:16         ` Russell King - ARM Linux
2014-02-22 10:16           ` Russell King - ARM Linux
2014-02-22 10:16           ` Russell King - ARM Linux
2014-02-22 12:13           ` Lorenzo Pieralisi
2014-02-22 12:13             ` Lorenzo Pieralisi
2014-02-22 12:13             ` Lorenzo Pieralisi
2014-02-22 22:30           ` Pavel Machek
2014-02-22 22:30             ` Pavel Machek
2014-02-22 22:30             ` Pavel Machek
2014-02-21  1:01     ` Sebastian Capella
2014-02-21  1:01       ` Sebastian Capella
2014-02-22 10:38     ` Russell King - ARM Linux
2014-02-22 10:38       ` Russell King - ARM Linux
2014-02-22 10:38       ` Russell King - ARM Linux
2014-02-22 12:09       ` Lorenzo Pieralisi
2014-02-22 12:09         ` Lorenzo Pieralisi
2014-02-22 12:09         ` Lorenzo Pieralisi
2014-02-22 22:28         ` Pavel Machek
2014-02-22 22:28           ` Pavel Machek
2014-02-22 22:28           ` Pavel Machek
2014-02-23 19:52         ` Sebastian Capella
2014-02-23 19:52           ` Sebastian Capella
2014-02-23 19:52           ` Sebastian Capella
2014-02-23 20:02         ` Sebastian Capella
2014-02-23 20:02           ` Sebastian Capella
2014-02-23 20:02           ` Sebastian Capella
2014-02-25 11:32           ` Lorenzo Pieralisi
2014-02-25 11:32             ` Lorenzo Pieralisi
2014-02-25 11:32             ` Lorenzo Pieralisi
2014-02-25 17:55             ` Sebastian Capella
2014-02-25 17:55               ` Sebastian Capella
2014-02-26 10:24               ` Lorenzo Pieralisi
2014-02-26 10:24                 ` Lorenzo Pieralisi
2014-02-26 10:24                 ` Lorenzo Pieralisi
2014-02-26 17:50                 ` Sebastian Capella
2014-02-26 17:50                   ` Sebastian Capella
2014-02-26 19:03                   ` Lorenzo Pieralisi
2014-02-26 19:03                     ` Lorenzo Pieralisi
2014-02-26 19:03                     ` Lorenzo Pieralisi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.