[PATCH v7 0/2] hibernation support on ARM

[PATCH v7 0/2] hibernation support on ARM

[Sebastian Capella]

Patches adding support for hibernation on ARM
 - ARM hibernation / suspend-to-disk
 - Change soft_restart to use non-tracing raw_local_irq_disable

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

[PATCH v7 1/2] ARM: avoid tracers in soft_restart
 arch/arm/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 Use raw_local_irq_disable in place of local_irq_disable to avoid
 infinite abort recursion while tracing. (unchanged since v3)

[PATCH v7 2/2] ARM hibernation / suspend-to-disk
 arch/arm/include/asm/memory.h |    1 +
 arch/arm/kernel/Makefile      |    1 +
 arch/arm/kernel/hibernate.c   |  108 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/Kconfig           |    5 ++
 include/linux/suspend.h       |    2 +
 5 files changed, 117 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
    #120: FILE: arch/arm/kernel/hibernate.c:24:
    +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
    #200: FILE: arch/arm/kernel/hibernate.c:104:
    +	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


Changes in v7:
--------------
* remove use of RELOC_HIDE macro
* remove unused #includes
* fixup comment for arch_restore_image
* ensure alignment of resume stack on 8 byte boundary

Changes in v6:
--------------
* Simplify static variable names

Changes in v5:
--------------
* Fixed checkpatch warning on trailing whitespace

Changes in v4:
--------------
* updated comment for soft_restart with review feedback
* dropped freeze_processes patch which was queued separately 
  to 3.14 by Rafael Wysocki:
  https://lkml.org/lkml/2014/2/25/683

Changes in v3:
--------------
* added comment to use of soft_restart
* drop irq disable soft_restart patch
* add patch to avoid tracers in soft_restart by using raw_local_irq_*

Changes in v2:
--------------
* Removed unneeded flush_thread, use of __naked and cpu_init.
* dropped Cyril Chemparathy <cyril@ti.com> from Cc: list as 
  emails are bouncing.

Thanks,

Sebastian Capella

[PATCH v7 1/2] ARM: avoid tracers in soft_restart

[Sebastian Capella]

Use of tracers in local_irq_disable is causes recursive aborts when
called with irqs disabled and using a temporary stack (hibernation).
Replace local_irq_disable with raw_local_irq_disable instead to
avoid tracers.

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 <robin.m.holt@gmail.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
Cc: Steven Capper <steve.capper@linaro.org>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Tejun Heo <tj@kernel.org>
---
 arch/arm/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 92f7b15..f58b723 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -100,7 +100,7 @@ void soft_restart(unsigned long addr)
 	u64 *stack = soft_restart_stack + ARRAY_SIZE(soft_restart_stack);
 
 	/* Disable interrupts first */
-	local_irq_disable();
+	raw_local_irq_disable();
 	local_fiq_disable();
 
 	/* Disable the L2 if we're the last man standing. */
-- 
1.7.9.5

[PATCH v7 1/2] ARM: avoid tracers in soft_restart

[Sebastian Capella]

Quoting Sebastian Capella (2014-03-05 02:50:04)
> Use of tracers in local_irq_disable is causes recursive aborts when
> called with irqs disabled and using a temporary stack (hibernation).
> Replace local_irq_disable with raw_local_irq_disable instead to
> avoid tracers.
> 
> 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 <robin.m.holt@gmail.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Konstantin Khlebnikov <k.khlebnikov@samsung.com>
> Cc: Steven Capper <steve.capper@linaro.org>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Tejun Heo <tj@kernel.org>
> ---
>  arch/arm/kernel/process.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> index 92f7b15..f58b723 100644
> --- a/arch/arm/kernel/process.c
> +++ b/arch/arm/kernel/process.c
> @@ -100,7 +100,7 @@ void soft_restart(unsigned long addr)
>         u64 *stack = soft_restart_stack + ARRAY_SIZE(soft_restart_stack);
>  
>         /* Disable interrupts first */
> -       local_irq_disable();
> +       raw_local_irq_disable();
>         local_fiq_disable();
>  
>         /* Disable the L2 if we're the last man standing. */
> -- 
> 1.7.9.5
Hi,

I have not heard any feedback on this change, since V3 of the
patchset.

Is everyone ok with this as it is?

Thomas, thanks for proposing this, is this looking ok to you?

Thanks!

Sebastian

[PATCH v7 1/2] ARM: avoid tracers in soft_restart

[Sebastian Capella]

On 6 March 2014 15:45, Sebastian Capella <sebastian.capella@linaro.org> wrote:
> Quoting Sebastian Capella (2014-03-05 02:50:04)
>> Use of tracers in local_irq_disable is causes recursive aborts when
>> called with irqs disabled and using a temporary stack (hibernation).
>> Replace local_irq_disable with raw_local_irq_disable instead to
>> avoid tracers.

> I have not heard any feedback on this change, since V3 of the
> patchset.
> Is everyone ok with this as it is?
> Thomas, thanks for proposing this, is this looking ok to you?

ping

I haven't received any further communication / acks on this particular
patch, since Russell's ~approval below:

On 26 February 2014 15:37, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> Great, I'm happy with this now.  I assume that the plan is to submit this
>  to my patch system when everyone's given their fill of acks?

Is this ok as it is?

Thanks,

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

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.

Acked-by: Russ Dill <Russ.Dill@ti.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Len Brown <len.brown@intel.com>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Will Deacon <will.deacon@arm.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>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Jiang Liu <liuj97@gmail.com>
Cc: Sricharan R <r.sricharan@ti.com>
Cc: Victor Kamensky <victor.kamensky@linaro.org>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/arm/include/asm/memory.h |    1 +
 arch/arm/kernel/Makefile      |    1 +
 arch/arm/kernel/hibernate.c   |  108 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/Kconfig           |    5 ++
 include/linux/suspend.h       |    2 +
 5 files changed, 117 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..d32adbb 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((unsigned long)(x))
 #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..656718a
--- /dev/null
+++ b/arch/arm/kernel/hibernate.c
@@ -0,0 +1,108 @@
+/*
+ * 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/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);
+	local_fiq_disable();
+}
+
+void notrace restore_processor_state(void)
+{
+	local_fiq_enable();
+}
+
+/*
+ * 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.
+ */
+static int notrace 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, arch_save_image);
+}
+
+/*
+ * Restore page contents for physical pages that were in use during loading
+ * hibernation image.  Switch to idmap_pgd so the physical page tables
+ * are overwritten with the same contents.
+ */
+static void notrace 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(virt_to_phys(cpu_resume));
+}
+
+static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __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 swsusp_arch_resume(void)
+{
+	extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
+	call_with_stack(arch_restore_image, 0,
+		resume_stack + ARRAY_SIZE(resume_stack));
+	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

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Sorry to those receiving repeated emails, I'm having an issue with some
of the special characters names.

Quoting Sebastian Capella (2014-03-05 02:50:05)
> From: Russ Dill <Russ.Dill@ti.com>
> Enable hibernation for ARM architectures and provide ARM
> architecture specific calls used during hibernation.
...
Quoting Stephen Boyd (2014-02-27 18:19:49)
> On 02/27/14 17:47, Russ Dill wrote:
> > On 02/27/2014 04:09 PM, Stephen Boyd wrote:
> >> On 02/27/14 15:57, Sebastian Capella wrote:
> >>> 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))
> >> Just curious, is there a reason for the RELOC_HIDE() here?
> >> Or
> >> __pa_symbol() for that matter? It looks like only x86 uses
> >> this on
> >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta?
> > From my understanding this needs to stick around so long as
> > gcc 3.x is
> > supported (did it get dropped yet?) on ARM Linux since it
> > doesn't
> > support -fno-strict-overflow.
>
> I don't think it's been dropped yet but I wonder if anyone has
> tried
> recent kernels with such a compiler?

Hi Stephen,

I was wondering if you had any objections or additional comments on
the current patch.

I've basically dropped the RELOC_HIDE.  It hasn't proven to be an issue
with the current compiler, and I was unable to revert back to 3.x
compiler on the cortex-v7 platforms I'm using.

Since this is being used on Hibernation, we would not be breaking any
existing functionality in any event, and it sounds like this is unlikely
to be an issue.

I spoke informally to a friend who works on toolchains at Linaro and
he suggested this may be related to the the strict-aliasing feature
which appears to be disabled by default in the kernel
(-fno-strict-aliasing) and generally not well liked.

Unless someone objects, I'll plan to add this to Russell's patch system.

Thanks,

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Stephen Boyd]

On 03/05, Sebastian Capella wrote:
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 8756e4b..d32adbb 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((unsigned long)(x))

Thanks for removing RELOC_HIDE, as Russell already stated it's
never been necessary on ARM.

Looking at this definition now it doesn't look right. Isn't
&__nosave_begin a virtual address? Casting it to an unsigned long
isn't going to give you a physical address. Why can't we use
__pa()?

> +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);
> +}

Perhaps this code could be:

	unsigned long nosave_begin_pfn = virt_to_pfn(&__nosave_begin);
	unsigned long nosave_end_pfn = virt_to_pfn(&__nosave_end);

	return (pfn >= nosave_begin_pfn) && (pfn <= nosave_end_pfn);

or if virt_to_pfn() doesn't exist on ARM and we can't add it for
some reason:

	unsigned long nosave_begin_pfn = __phys_to_pfn(__pa(&__nosave_begin));
	unsigned long nosave_end_pfn = __phys_to_pfn(__pa(&__nosave_end));

	return (pfn >= nosave_begin_pfn) && (pfn <= nosave_end_pfn);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

On 6 March 2014 20:42, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 03/05, Sebastian Capella wrote:
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>> index 8756e4b..d32adbb 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((unsigned long)(x))
> Looking at this definition now it doesn't look right. Isn't
> &__nosave_begin a virtual address? Casting it to an unsigned long
> isn't going to give you a physical address. Why can't we use
> __pa()?

You're correct, sorry, I missed removing the cast, it's not needed.
I've removed the __pa_symbol define as I'm switching to the __pa
implementation you suggested below.

>> +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);
>> +}
>
> Perhaps this code could be:
>
>         unsigned long nosave_begin_pfn = virt_to_pfn(&__nosave_begin);
>         unsigned long nosave_end_pfn = virt_to_pfn(&__nosave_end);
>
>         return (pfn >= nosave_begin_pfn) && (pfn <= nosave_end_pfn);
>
> or if virt_to_pfn() doesn't exist on ARM and we can't add it for
> some reason:
>
>         unsigned long nosave_begin_pfn = __phys_to_pfn(__pa(&__nosave_begin));
>         unsigned long nosave_end_pfn = __phys_to_pfn(__pa(&__nosave_end));
>
>         return (pfn >= nosave_begin_pfn) && (pfn <= nosave_end_pfn);
I've moved to this second implementation.  I don't think I understand
the ramifications enough to add the virt_to_pfn call for arm.

I've changed one thing.  __nosave_end is pointing@the first byte
not included in the nosave region.  I subtracted one from it so that
we make sure we're referring to the last pfn, and left the pfn
comparison as you'd suggested.

int pfn_is_nosave(unsigned long pfn)
{
        unsigned long nosave_begin_pfn = __phys_to_pfn(__pa(&__nosave_begin));
        unsigned long nosave_end_pfn = __phys_to_pfn(__pa(&__nosave_end - 1));

        return (pfn >= nosave_begin_pfn) && (pfn <= nosave_end_pfn);
}

I'll test this and post it if everyone's ok with it.

Thanks for your review and comments Stephen!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Russell King - ARM Linux]

On Mon, Mar 10, 2014 at 11:32:17AM -0700, Sebastian Capella wrote:
> I've moved to this second implementation.  I don't think I understand
> the ramifications enough to add the virt_to_pfn call for arm.

Let's look at the implementations...

#define __pa(x)                 __virt_to_phys((unsigned long)(x))
static inline phys_addr_t __virt_to_phys(unsigned long x)
{
        return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
}

So, __pa() returns a phys_addr_t (which may be either 32-bit or 64-bit).

#define virt_to_pfn(kaddr)  (__pa(kaddr) >> PAGE_SHIFT)

And that will do an appropriate shift to convert the phys_addr_t to a PFN,
which will be fine to assign to an unsigned long variable.

#define __phys_to_pfn(paddr)    ((unsigned long)((paddr) >> PAGE_SHIFT))

This does the same thing, but has an explicit cast.

So, the only difference between the two suggestions is that additional
cast, which has no effect here.

> I've changed one thing.  __nosave_end is pointing at the first byte
> not included in the nosave region.  I subtracted one from it so that
> we make sure we're referring to the last pfn, and left the pfn
> comparison as you'd suggested.

Yes, that should be safer.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Thanks Russell!

On 16 March 2014 02:46, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Mon, Mar 10, 2014 at 11:32:17AM -0700, Sebastian Capella wrote:
...
> Let's look at the implementations...
>
> #define __pa(x)                 __virt_to_phys((unsigned long)(x))
> static inline phys_addr_t __virt_to_phys(unsigned long x)
> {
>         return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
> }
>
> So, __pa() returns a phys_addr_t (which may be either 32-bit or 64-bit).
>
> #define virt_to_pfn(kaddr)  (__pa(kaddr) >> PAGE_SHIFT)
>
> And that will do an appropriate shift to convert the phys_addr_t to a PFN,
> which will be fine to assign to an unsigned long variable.
>
> #define __phys_to_pfn(paddr)    ((unsigned long)((paddr) >> PAGE_SHIFT))
>
> This does the same thing, but has an explicit cast.
>
> So, the only difference between the two suggestions is that additional
> cast, which has no effect here.

Sounds good, thanks!

I was a little unsure about the page sizes that could affect all of
the implementations.

I'll make this change and repost, basically using virt_to_pfn in place
of the previous __pa_symbol, and remove unused declarations.

Thanks again!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Ezequiel Garcia]

On Mar 05, Sebastian Capella wrote:
[..]
> 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"
>  

Is there any reason why CPU_FEROCEON is not listed here? FWIW, I've just built
(but not really tested) a Kirkwood kernel with CONFIG_HIBERNATION=y.

And is there any reason to put this config in arch/arm/mm/Kconfig, instead of
in arch/arm/Kconfig, below ARCH_SUSPEND_POSSIBLE? 

I'm also puzzled about having two separate options for suspend and hibernate,
maybe someone can explain me why a given CPU would support the former but not
the latter?

Thanks!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

On 16 March 2014 00:09, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Mar 05, Sebastian Capella wrote:
> [..]
>> 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
...
>> +     default y if CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || CPU_XSCALE || CPU_XSC3 || CPU_V6 || CPU_V6K || CPU_V7
...
> Is there any reason why CPU_FEROCEON is not listed here? FWIW, I've just built
> (but not really tested) a Kirkwood kernel with CONFIG_HIBERNATION=y.
No reason; I did not change this from the original patch I'd received.
 I didn't try to get a comprehensive list of supported hardware.  To
my understanding, the goal is to get the infrastructure in so that
people can start working on their platforms and add support for them.

> And is there any reason to put this config in arch/arm/mm/Kconfig, instead of
> in arch/arm/Kconfig, below ARCH_SUSPEND_POSSIBLE?
I don't have a reason.  Anyone else have a comment on this?
Otherwise, I'll move it.. thanks!

> I'm also puzzled about having two separate options for suspend and hibernate,
> maybe someone can explain me why a given CPU would support the former but not
> the latter?
It's part of having the generic hibernation implemented and available
but with architecture specific dependencies.  Where an architecture
may not have support for hibernation, it will prevent compilation of
the generic hibernation support.  For example, at the moment, ARM does
not support hibernation.  As a result, hibernation is not presented as
an option in menuconfig.  If you were to artificially enable it (by
hacking ARCH_HIBERNATION_POSSIBLE=y, and then turning
CONFIG_HIBERNATION=y) then you'd get linker errors due to the missing
architecture hibernation functions.  (basically what is being added by
this patch for ARM).  It's like this for other architectures as well
(mips, sh, etc..)

Thanks,

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Ezequiel Garcia]

On Mar 17, Sebastian Capella wrote:
> On 16 March 2014 00:09, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
> > On Mar 05, Sebastian Capella wrote:
> > [..]
> >> 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
> ...
> >> +     default y if CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || CPU_XSCALE || CPU_XSC3 || CPU_V6 || CPU_V6K || CPU_V7
> ...
> > Is there any reason why CPU_FEROCEON is not listed here? FWIW, I've just built
> > (but not really tested) a Kirkwood kernel with CONFIG_HIBERNATION=y.
> No reason; I did not change this from the original patch I'd received.
>  I didn't try to get a comprehensive list of supported hardware.  To
> my understanding, the goal is to get the infrastructure in so that
> people can start working on their platforms and add support for them.
> 

Sure, no problem. If you consider that build-test is enough, feel free to put
CPU_FEROCEON on that list. We added suspend/resume to feroceon not long ago.

> > And is there any reason to put this config in arch/arm/mm/Kconfig, instead of
> > in arch/arm/Kconfig, below ARCH_SUSPEND_POSSIBLE?
> I don't have a reason.  Anyone else have a comment on this?
> Otherwise, I'll move it.. thanks!
>

It looked reasonable to me.

> > I'm also puzzled about having two separate options for suspend and hibernate,
> > maybe someone can explain me why a given CPU would support the former but not
> > the latter?
> It's part of having the generic hibernation implemented and available
> but with architecture specific dependencies.  Where an architecture
> may not have support for hibernation, it will prevent compilation of
> the generic hibernation support.  For example, at the moment, ARM does
> not support hibernation.
[..]

I guess my question wasn't clear. I mean to ask: Are there any other
requirements on an ARM platform to support hibernation, other than
suspend/resume support?

If this is the *only* requirement, it seems to me we could make our
ARCH_SUSPEND_POSSIBLE also select ARCH_HIBERNATION_POSSIBLE.

Does this make sense?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

On 17 March 2014 13:44, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Mar 17, Sebastian Capella wrote:
>> On 16 March 2014 00:09, Ezequiel Garcia
>> <ezequiel.garcia@free-electrons.com> wrote:
>> > On Mar 05, Sebastian Capella wrote:
>> > [..]
>> >> 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
...
>> > Is there any reason why CPU_FEROCEON is not listed here? FWIW, I've just built
>> > (but not really tested) a Kirkwood kernel with CONFIG_HIBERNATION=y.
...
> Sure, no problem. If you consider that build-test is enough, feel free to put
> CPU_FEROCEON on that list. We added suspend/resume to feroceon not long ago.
>> > And is there any reason to put this config in arch/arm/mm/Kconfig, instead of
>> > in arch/arm/Kconfig, below ARCH_SUSPEND_POSSIBLE?
...
> I guess my question wasn't clear. I mean to ask: Are there any other
> requirements on an ARM platform to support hibernation, other than
> suspend/resume support?
>
> If this is the *only* requirement, it seems to me we could make our
> ARCH_SUSPEND_POSSIBLE also select ARCH_HIBERNATION_POSSIBLE.

Thanks, I've added it like this in arch/arm/Kconfig.  I'm sure you
know, but this way also takes care of the CPU_FEROCEON in the default
list since SUSPEND_POSSIBLE already contains it.

config ARCH_HIBERNATION_POSSIBLE
        bool
        depends on MMU
        default y if ARCH_SUSPEND_POSSIBLE

Does this look ok?

Thanks!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Ezequiel Garcia]

On Mar 17, Sebastian Capella wrote:
[..]
> 
> Thanks, I've added it like this in arch/arm/Kconfig.  I'm sure you
> know, but this way also takes care of the CPU_FEROCEON in the default
> list since SUSPEND_POSSIBLE already contains it.
> 
> config ARCH_HIBERNATION_POSSIBLE
>         bool
>         depends on MMU
>         default y if ARCH_SUSPEND_POSSIBLE
> 
> Does this look ok?
> 

I applied this change on top of your patches and tested it on a Kirkwood
Openblocks A6 board, using a resume=/dev/sda2 kernel parameter (iow, without
any U-Boot assistance to resume). Seems to work fine (as you can see here
http://sprunge.us/BJRV). I guess you can add a:

Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

On the other side, this board has no pm_power_off() support, which means
kernel_halt() is called after kernel_power_off().

I'm not sure if a NULL pm_power_off() is supported, but this makes my kernel
crash in a reboot notifier that's called twice (first in kernel_power_off
and then in kernel_halt):

Unable to handle kernel paging request at virtual address 00100104
pgd = df634000
[00100104] *pgd=1f5c3831, *pte=00000000, *ppte=00000000
Internal error: Oops: 817 [#1] PREEMPT ARM
CPU: 0 PID: 565 Comm: sh Not tainted 3.14.0-rc6-00002-g06da70a-dirty #24
task: df4d5440 ti: df666000 task.ti: df666000
PC is at led_trigger_unregister+0x3c/0xcc
LR is at led_trigger_unregister+0x20/0xcc
pc : [<c0273090>]    lr : [<c0273074>]    psr: 60000093
sp : df667e50  ip : 00100100  fp : 000ab294
r10: 00000002  r9 : 00000000  r8 : c0459a6c
r7 : c046c87c  r6 : c046c964  r5 : c03ee198  r4 : c046c964
r3 : 00200200  r2 : 00100100  r1 : 00200200  r0 : c046c8e4
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0005397f  Table: 1f634000  DAC: 00000015
Process sh (pid: 565, stack limit = 0xdf6661c0)
Stack: (0xdf667e50 to 0xdf668000)
7e40:                                     00000000 c046c964 c03ee198 00000000
7e60: 00000000 c0273ae0 c0273ab4 c046c94c ffffffff c0036698 c04546c8 ffffffff
7e80: 00000000 00000002 c0446800 c04550e8 df5270c8 c0036b24 00000000 c034cce0
7ea0: c03c4b18 c04550e8 df666018 00000000 c0476f60 c0036b54 00000000 c04550e8
7ec0: df5270c8 c00378ec 00000001 c034cc64 c0455288 c0044cac 0000006b df4270d8
7ee0: df53f480 00000005 00000004 df53f480 00000005 c0042dc4 00000005 df4270d8
7f00: df53f480 00000005 df667f80 df53f480 df5270c0 c01a2bf4 00000005 c0108dd0
7f20: c0108d8c 00000000 00000000 c010c090 00000000 00000000 df4dd280 000acb10
7f40: df667f80 00000005 00000000 00000005 00000000 c00af2b8 fffffff6 c0019d9c
7f60: 00000003 00000000 00000000 df4dd280 000acb10 00000000 00000005 c00af448
7f80: 00000000 00000000 00200200 000aa8b0 00000001 000acb10 00000004 c0009424
7fa0: df666000 c00092c0 000aa8b0 00000001 00000001 000acb10 00000005 00000000
7fc0: 000aa8b0 00000001 000acb10 00000004 00000020 000ab2a8 000ab274 000ab294
7fe0: 00000005 befbd738 0000e1f0 b6edeb4c 60000010 00000001 1fffd831 1fffdc31
[<c0273090>] (led_trigger_unregister) from [<c0273ae0>] (heartbeat_reboot_notifier+0x2c/0x40)
[<c0273ae0>] (heartbeat_reboot_notifier) from [<c0036698>] (notifier_call_chain+0x48/0x9c)
[<c0036698>] (notifier_call_chain) from [<c0036b24>] (__blocking_notifier_call_chain+0x48/0x60)
[<c0036b24>] (__blocking_notifier_call_chain) from [<c0036b54>] (blocking_notifier_call_chain+0x18/0x20)
[<c0036b54>] (blocking_notifier_call_chain) from [<c00378ec>] (kernel_halt+0x14/0x58)
[<c00378ec>] (kernel_halt) from [<c034cc64>] (power_down+0x8c/0xac)
[<c034cc64>] (power_down) from [<c0044cac>] (hibernate+0x1a8/0x1ec)
[<c0044cac>] (hibernate) from [<c0042dc4>] (state_store+0xac/0xb8)
[<c0042dc4>] (state_store) from [<c01a2bf4>] (kobj_attr_store+0x14/0x20)
[<c01a2bf4>] (kobj_attr_store) from [<c0108dd0>] (sysfs_kf_write+0x44/0x48)
[<c0108dd0>] (sysfs_kf_write) from [<c010c090>] (kernfs_fop_write+0xb4/0x14c)
[<c010c090>] (kernfs_fop_write) from [<c00af2b8>] (vfs_write+0xac/0x188)
[<c00af2b8>] (vfs_write) from [<c00af448>] (SyS_write+0x3c/0x78)
[<c00af448>] (SyS_write) from [<c00092c0>] (ret_fast_syscall+0x0/0x2c)
Code: e3a03602 e2822c01 e2833c02 e59f7084 (e58c1004) 
---[ end trace 72dd5ccae5489f38 ]---

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

On 19 March 2014 08:44, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Thanks!
> On the other side, this board has no pm_power_off() support, which means
> kernel_halt() is called after kernel_power_off().
>
> I'm not sure if a NULL pm_power_off() is supported, but this makes my kernel
> crash in a reboot notifier that's called twice (first in kernel_power_off
> and then in kernel_halt):

The omap board I'm using has a null pm_power_off.  I added code to
bypass kernel_power_off in the event pm_power_off is null like this:


diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index a5f702a..d96b910 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -594,7 +594,8 @@ static void power_down(void)
        case HIBERNATION_PLATFORM:
                hibernation_platform_enter();
        case HIBERNATION_SHUTDOWN:
-               kernel_power_off();
+               if (pm_power_off)
+                       kernel_power_off();
                break;
 #ifdef CONFIG_SUSPEND
        case HIBERNATION_SUSPEND:


This follows the behavior in the reboot syscall which does it this way
also.  I'm testing this now, and it seems work fine.  If this looks
good, I can add it as an additional patch.

Thanks,

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

On 19 March 2014 13:47, Sebastian Capella <sebastian.capella@linaro.org> wrote:
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index a5f702a..d96b910 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -594,7 +594,8 @@ static void power_down(void)
>         case HIBERNATION_PLATFORM:
>                 hibernation_platform_enter();
>         case HIBERNATION_SHUTDOWN:
> -               kernel_power_off();
> +               if (pm_power_off)
> +                       kernel_power_off();
>                 break;
>  #ifdef CONFIG_SUSPEND
>         case HIBERNATION_SUSPEND:
>
>
> This follows the behavior in the reboot syscall which does it this way
> also.  I'm testing this now, and it seems work fine.  If this looks
> good, I can add it as an additional patch.

BTW, one thing I would point out is that kernel_power_off and
kernel_halt call the same notifier but with different parameters
(SYS_POWER_OFF and SYS_HALT).

If pm_power_down is null, I dont see why we'd want to notify
SYS_POWER_OFF before SYS_HALT.  With the previous change I'm assuming
there's no benefit, so please chime in if you know a reason.

Thanks,

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Hi,

I have no further changes here..  I will submit a separate patch to
cover Ezequiel's concern.  Any final comments?  Otherwise, I'll submit
to Russell's system.

Thanks for the review!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Alexander Holler]

Am 19.03.2014 22:06, schrieb Sebastian Capella:
> On 19 March 2014 13:47, Sebastian Capella <sebastian.capella@linaro.org> wrote:
>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index a5f702a..d96b910 100644
>> --- a/kernel/power/hibernate.c
>> +++ b/kernel/power/hibernate.c
>> @@ -594,7 +594,8 @@ static void power_down(void)
>>          case HIBERNATION_PLATFORM:
>>                  hibernation_platform_enter();
>>          case HIBERNATION_SHUTDOWN:
>> -               kernel_power_off();
>> +               if (pm_power_off)
>> +                       kernel_power_off();
>>                  break;
>>   #ifdef CONFIG_SUSPEND
>>          case HIBERNATION_SUSPEND:
>>
>>
>> This follows the behavior in the reboot syscall which does it this way
>> also.  I'm testing this now, and it seems work fine.  If this looks
>> good, I can add it as an additional patch.
>
> BTW, one thing I would point out is that kernel_power_off and
> kernel_halt call the same notifier but with different parameters
> (SYS_POWER_OFF and SYS_HALT).
>
> If pm_power_down is null, I dont see why we'd want to notify
> SYS_POWER_OFF before SYS_HALT.  With the previous change I'm assuming
> there's no benefit, so please chime in if you know a reason.

Both states, power off and sys halt, do sound pretty final and I would 
assume something is broken, if power off is called before sys halt or 
vice versa. At least I would never expect that the 
reboot/poweroff/syshalt notifier may be called twice (and thats why the 
heartbeat-trigger may crash).

But just in case, changing that behaviour in ledtrig-heartbeat.c would 
be pretty easy, just remove the heartbeat_reboot_notifier (which plays 
nice and deregisters the trigger on reboot) and use the panic_notifier 
(which doesn't unregister the trigger but just turns off the led) for 
reboot too. Another solution would be to unregister the reboot_notifier 
in the reboot_nofifier itself. I've just seen one watchdog driver 
(drivers/rtc/rtc-m41t80.c) which does that. But I still think such 
shouldn't be necessary (and I haven't had a look at other 
reboot_notifier users).


Regards,

Alexander Holler

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Hi Alexander,

Thanks for your comments.

I've posted a different solution for arm only here, that just causes a
while loop in machine_power_off in the event it's called but does not
halt.

https://lkml.org/lkml/2014/3/24/299

On 25 March 2014 11:38, Alexander Holler <holler@ahsoftware.de> wrote:
> Both states, power off and sys halt, do sound pretty final and I would
> assume something is broken, if power off is called before sys halt or vice
> versa. At least I would never expect that the reboot/poweroff/syshalt
> notifier may be called twice (and thats why the heartbeat-trigger may
> crash).
>
> But just in case, changing that behaviour in ledtrig-heartbeat.c would be
> pretty easy, just remove the heartbeat_reboot_notifier (which plays nice and
> deregisters the trigger on reboot) and use the panic_notifier (which doesn't
> unregister the trigger but just turns off the led) for reboot too. Another
> solution would be to unregister the reboot_notifier in the reboot_nofifier
> itself. I've just seen one watchdog driver (drivers/rtc/rtc-m41t80.c) which
> does that. But I still think such shouldn't be necessary (and I haven't had
> a look at other reboot_notifier users).

I hope this will all be handled by the loop in machine_power_off, so
maybe this will not be needed.

Thanks!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Alexander Holler]

Am 25.03.2014 19:38, schrieb Alexander Holler:

> reboot too. Another solution would be to unregister the reboot_notifier
> in the reboot_nofifier itself. I've just seen one watchdog driver
> (drivers/rtc/rtc-m41t80.c) which does that. But I still think such

That, btw. is broken. ;)

Right after having send the mail, I've became that intuition, had a look 
and ... notifier.h does state the following:

  * atomic_notifier_chain_unregister(), 
blocking_notifier_chain_unregister(),
  * and srcu_notifier_chain_unregister() _must not_ be called from within
  * the call chain.

(The reboot-notifier chain is of type blocking_notifier_chain)

So I've picked up one user of reboot_notifier by random and had the luck 
to choose a broken one. ;)

I will make a patch and will have a look if the same failure can be 
found elsewhere. There aren't that much users of the reboot-notifier, so 
it shouldn't cost me that much time.

Regards,

Alexander Holler

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Alexander Holler]

Am 26.03.2014 00:36, schrieb Alexander Holler:
> Am 25.03.2014 19:38, schrieb Alexander Holler:
>
>> reboot too. Another solution would be to unregister the reboot_notifier
>> in the reboot_nofifier itself. I've just seen one watchdog driver
>> (drivers/rtc/rtc-m41t80.c) which does that. But I still think such
>
> That, btw. is broken. ;)
>
> Right after having send the mail, I've became that intuition, had a look
> and ... notifier.h does state the following:
>
>   * atomic_notifier_chain_unregister(),
> blocking_notifier_chain_unregister(),
>   * and srcu_notifier_chain_unregister() _must not_ be called from within
>   * the call chain.
>
> (The reboot-notifier chain is of type blocking_notifier_chain)
>
> So I've picked up one user of reboot_notifier by random and had the luck
> to choose a broken one. ;)
>
> I will make a patch and will have a look if the same failure can be
> found elsewhere. There aren't that much users of the reboot-notifier, so
> it shouldn't cost me that much time.

Hmm, and either I was confused, or have looked at some other user of the 
reboot_notifier, but rtc-m41t80.c doesn't call unregister from it's 
notifier. And unfortunately there are a bit more users of the 
reboot_notifier than I first thought. :/

I will check if I can find out at least at which driver I had a look at 
which did call unregister_notifier from the notifier itself.

Regards,

Alexander Holler

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[TonyHo]

Hi ,
I'm trying add the hibernation to Freescale imx6Q(Cortex-A9*4), and I 
use the patches from:
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036055.html 

And the TuxOnIce patches:
http://tuxonice.nigelcunningham.com.au/downloads/all/tuxonice-for-linux-3.0-2012-10-23.patch.bz2 

The kernel version is 3.0.35. And the SoC have 4 Cores.
When I try to make the hibernation snapshot image, the kernel hint "NULL 
pointer dereference", when enter the "swsusp_arch_suspend" function. 
This function comes from the first patch link.
The function is called in file tuxonice_builtin.c , the code snappit is 
showed below:
int toi_lowlevel_builtin(void){
...
     save_processor_state();
     printk(KERN_ERR "Will Swsusp_arch_suspend\n");
     error = swsusp_arch_suspend();
     printk(KERN_ERR "Done Swsusp_arch_suspend\n");
     if (error)
         printk(KERN_ERR "Error %d hibernating\n", error);
...
}

1. And I have tried to remove the "error = swsusp_arch_suspend()" , and 
this would fix the "NULL pointer dereference", So I can confirm this 
function is the problem. But I can't identify which sentence cause the 
NULL pointer dereference.  Detail log is following.

2. Also the "[ 23.058386] ...20%...40% " in below logs confuse me, it 
seems the cache writing is not complete, it miss the 60% and 80%.

3. When the 'replace the swsusp with tuxonice' is not selected, I use 
the 'echo disk>/sys/power/state' to make a hibernation, will cause the 
same problem.
So this question might caused by the patch from the first patch link. 
The freescale official BSP kernel version is 3.0.35, so the lastest 
suspend to disk patches is not suitable. Any one have tried the SMP 
Cortex-A9 hibernation ?

Looking forward to someone would give me a hand.

Detail Logs:
/ # echo >/sys/power/tuxonice/do_hibernate
[ 22.191954] TuxOnIce 3.2, with support for usm, compression, block i/o, 
swap storage, file storage, userui.
[ 22.202953] Initiating a hibernation cycle.
  [ 22.207481] Failed to launch userspace program 
'/usr/local/sbin/tuxoniceui_text': Error -2
[ 22.215774] Launch userspace program failed.
[ 22.220090] Starting other threads.
[ 22.223537] Freezing processes & syncing filesystems.
[ 22.228793] Stopping fuse filesystems.
[ 22.232549] Freezing user space processes ... (elapsed 0.01 seconds) done.
[ 22.782219] Stopping normal filesystems.
[ 22.800634] Freezing remaining freezable tasks ... (elapsed 0.01 
seconds) done.
[ 22.867306] Preparing Image. Try 1.
[ 22.925873] Restarting normal filesystems.
[ 22.957534] Stopping fuse filesystems.
[ 22.961311] Freezing user space processes ... (elapsed 0.00 seconds) done.
[ 22.968803] Stopping normal filesystems.
[ 22.975835] Freezing remaining freezable tasks ... (elapsed 0.01 
seconds) done.
[ 23.049812] Starting to save the image..
[ 23.053750] Writing caches...
[ 23.058386] ...20%...40%
[ 23.061313] ====>> iofinish+iobase=55P,iobarmax=8758P
[ 23.066467] ====>> iofinish+iobase=220KB,iobarmax=35032KB
[ 23.071982] ====>> 
toi_compress_bytes_in=225280B,toi_compress_bytes_out=106086B
[ 23.106327] Waited for i/o due to synchronous I/O 4 times.
[ 23.111848] Doing atomic copy/restore.
[ 23.115602] Enter the Hibernation [imx6_hibernation_begin]
[ 23.122407] udc suspend begins
[ 23.126080] add wake up source irq 51
[ 23.129910] PM: freeze of devices complete after 8.805 msecs
[ 23.136256] PM: late freeze of devices complete after 0.677 msecs
[ 23.142372] Enter the Hibernation [imx6_hibernation_pre_snapshot]
[ 23.148471] Disabling non-boot CPUs ...
[ 23.153364] CPU1: shutdown
[ 23.157274] CPU2: shutdown
[ 23.160823] CPU3: shutdown
[ 23.163908] Will Swsusp_arch_suspend
[ 23.421375] Unable to handle kernel NULL pointer dereference at virtual 
address 00000000
[ 23.429477] pgd = a8b3c000
[ 23.432186] [00000000] *pgd=3816c831, *pte=00000000, *ppte=00000000
[ 23.438500] Internal error: Oops: 80000007 [#1] PREEMPT SMP
[ 23.444076] Modules linked in:
[ 23.447150] CPU: 0 Not tainted (3.0.35 #56)
[ 23.451687] PC is at 0x0
[ 23.454223] LR is at 0x0
[ 23.456759] pc : [<00000000>] lr : [<00000000>] psr: 60000093
[ 23.456764] sp : a8b27e90 ip : 00000002 fp : 00000000
[ 23.468251] r10: 804ee340 r9 : 00000000 r8 : 00000000
[ 23.473479] r7 : 804aa4b0 r6 : 00000000 r5 : 804edd28 r4 : 804ed674
[ 23.480010] r3 : 804e4478 r2 : 804cf814 r1 : 00008000 r0 : 00000000
[ 23.486543] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment 
kernel
[ 23.493942] Control: 10c53c7d Table: 38b3c04a DAC: 00000015
[ 23.499690]
[ 23.499692] SP: 0xa8b27e10:
[ 23.503972] 7e10 33322020 3336312e 5d383039 00000020 00000000 00000001 
00000000 00025b3e
[ 23.512266] 7e30 00000000 0000040f 00000007 00000000 804aa4b0 80033bb4 
00000000 00008000
[ 23.520558] 7e50 804cf814 804e4478 804ed674 804edd28 00000000 804aa4b0 
00000000 00000000
[ 23.528850] 7e70 804ee340 00000000 00000002 a8b27e90 00000000 00000000 
60000093 ffffffff
[ 23.537141] 7e90 00000001 804ed674 804edd28 00000000 804aa4b0 00000000 
804ee340 800a700c
[ 23.545435] 7eb0 60000093 804ed684 804edd28 8009fd94 000021ff 00000037 
00000004 00000003
[ 23.553726] 7ed0 a8b26000 00000000 804aa000 804ed684 00000000 804ee340 
00000000 800a028c
[ 23.562020] 7ef0 8009e848 804cf9c8 a8bee000 00000001 00000001 00000000 
a825f3f8 804cfa2c
On 03/19/2014 11:44 PM, Ezequiel Garcia wrote:
> On Mar 17, Sebastian Capella wrote:
> [..]
>> Thanks, I've added it like this in arch/arm/Kconfig.  I'm sure you
>> know, but this way also takes care of the CPU_FEROCEON in the default
>> list since SUSPEND_POSSIBLE already contains it.
>>
>> config ARCH_HIBERNATION_POSSIBLE
>>          bool
>>          depends on MMU
>>          default y if ARCH_SUSPEND_POSSIBLE
>>
>> Does this look ok?
>>
> I applied this change on top of your patches and tested it on a Kirkwood
> Openblocks A6 board, using a resume=/dev/sda2 kernel parameter (iow, without
> any U-Boot assistance to resume). Seems to work fine (as you can see here
> http://sprunge.us/BJRV). I guess you can add a:
>
> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>
> On the other side, this board has no pm_power_off() support, which means
> kernel_halt() is called after kernel_power_off().
>
> I'm not sure if a NULL pm_power_off() is supported, but this makes my kernel
> crash in a reboot notifier that's called twice (first in kernel_power_off
> and then in kernel_halt):
>
> Unable to handle kernel paging request at virtual address 00100104
> pgd = df634000
> [00100104] *pgd=1f5c3831, *pte=00000000, *ppte=00000000
> Internal error: Oops: 817 [#1] PREEMPT ARM
> CPU: 0 PID: 565 Comm: sh Not tainted 3.14.0-rc6-00002-g06da70a-dirty #24
> task: df4d5440 ti: df666000 task.ti: df666000
> PC is at led_trigger_unregister+0x3c/0xcc
> LR is at led_trigger_unregister+0x20/0xcc
> pc : [<c0273090>]    lr : [<c0273074>]    psr: 60000093
> sp : df667e50  ip : 00100100  fp : 000ab294
> r10: 00000002  r9 : 00000000  r8 : c0459a6c
> r7 : c046c87c  r6 : c046c964  r5 : c03ee198  r4 : c046c964
> r3 : 00200200  r2 : 00100100  r1 : 00200200  r0 : c046c8e4
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 0005397f  Table: 1f634000  DAC: 00000015
> Process sh (pid: 565, stack limit = 0xdf6661c0)
> Stack: (0xdf667e50 to 0xdf668000)
> 7e40:                                     00000000 c046c964 c03ee198 00000000
> 7e60: 00000000 c0273ae0 c0273ab4 c046c94c ffffffff c0036698 c04546c8 ffffffff
> 7e80: 00000000 00000002 c0446800 c04550e8 df5270c8 c0036b24 00000000 c034cce0
> 7ea0: c03c4b18 c04550e8 df666018 00000000 c0476f60 c0036b54 00000000 c04550e8
> 7ec0: df5270c8 c00378ec 00000001 c034cc64 c0455288 c0044cac 0000006b df4270d8
> 7ee0: df53f480 00000005 00000004 df53f480 00000005 c0042dc4 00000005 df4270d8
> 7f00: df53f480 00000005 df667f80 df53f480 df5270c0 c01a2bf4 00000005 c0108dd0
> 7f20: c0108d8c 00000000 00000000 c010c090 00000000 00000000 df4dd280 000acb10
> 7f40: df667f80 00000005 00000000 00000005 00000000 c00af2b8 fffffff6 c0019d9c
> 7f60: 00000003 00000000 00000000 df4dd280 000acb10 00000000 00000005 c00af448
> 7f80: 00000000 00000000 00200200 000aa8b0 00000001 000acb10 00000004 c0009424
> 7fa0: df666000 c00092c0 000aa8b0 00000001 00000001 000acb10 00000005 00000000
> 7fc0: 000aa8b0 00000001 000acb10 00000004 00000020 000ab2a8 000ab274 000ab294
> 7fe0: 00000005 befbd738 0000e1f0 b6edeb4c 60000010 00000001 1fffd831 1fffdc31
> [<c0273090>] (led_trigger_unregister) from [<c0273ae0>] (heartbeat_reboot_notifier+0x2c/0x40)
> [<c0273ae0>] (heartbeat_reboot_notifier) from [<c0036698>] (notifier_call_chain+0x48/0x9c)
> [<c0036698>] (notifier_call_chain) from [<c0036b24>] (__blocking_notifier_call_chain+0x48/0x60)
> [<c0036b24>] (__blocking_notifier_call_chain) from [<c0036b54>] (blocking_notifier_call_chain+0x18/0x20)
> [<c0036b54>] (blocking_notifier_call_chain) from [<c00378ec>] (kernel_halt+0x14/0x58)
> [<c00378ec>] (kernel_halt) from [<c034cc64>] (power_down+0x8c/0xac)
> [<c034cc64>] (power_down) from [<c0044cac>] (hibernate+0x1a8/0x1ec)
> [<c0044cac>] (hibernate) from [<c0042dc4>] (state_store+0xac/0xb8)
> [<c0042dc4>] (state_store) from [<c01a2bf4>] (kobj_attr_store+0x14/0x20)
> [<c01a2bf4>] (kobj_attr_store) from [<c0108dd0>] (sysfs_kf_write+0x44/0x48)
> [<c0108dd0>] (sysfs_kf_write) from [<c010c090>] (kernfs_fop_write+0xb4/0x14c)
> [<c010c090>] (kernfs_fop_write) from [<c00af2b8>] (vfs_write+0xac/0x188)
> [<c00af2b8>] (vfs_write) from [<c00af448>] (SyS_write+0x3c/0x78)
> [<c00af448>] (SyS_write) from [<c00092c0>] (ret_fast_syscall+0x0/0x2c)
> Code: e3a03602 e2822c01 e2833c02 e59f7084 (e58c1004)
> ---[ end trace 72dd5ccae5489f38 ]---
>

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Hi Tony,

I have not used the tux-on-ice solution.  The patchset I've sent is to
add initial support for swsusp based hibernation for ARM.

Can you please begin a new, separate thread for this?  Ie. separate
from the review for the swsusp hibernation patches I've submitted?
I'm not sure where the right list/forum is for this, perhaps someone
can make a recommendation, but I am not sure it should go on LKML.
Please copy me on your new thread.

Thanks!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Russell King - ARM Linux]

On Wed, Mar 05, 2014 at 02:50:05AM -0800, Sebastian Capella wrote:
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 8756e4b..d32adbb 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((unsigned long)(x))
>  #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)

I don't see the appropriate version on the mailing list, so I'll comment
here instead.  In 8011/1, you added here:

+#define virt_to_pfn(kaddr)      (__pa(kaddr) >> PAGE_SHIFT)

which conflicts with my solution (which fixes some rather horrid assembly
code).  You can find my change as e26a9e00afc4 (ARM: Better virt_to_page()
handling).  I can drop this from your patch, but it would be a good idea
if you could re-validate against v3.15-rc1.

Thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

On 16 April 2014 03:12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Wed, Mar 05, 2014 at 02:50:05AM -0800, Sebastian Capella wrote:
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>> index 8756e4b..d32adbb 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((unsigned long)(x))
>>  #define pfn_to_kaddr(pfn)    __va((pfn) << PAGE_SHIFT)
>
> I don't see the appropriate version on the mailing list, so I'll comment
> here instead.  In 8011/1, you added here:
>
> +#define virt_to_pfn(kaddr)      (__pa(kaddr) >> PAGE_SHIFT)
>
> which conflicts with my solution (which fixes some rather horrid assembly
> code).  You can find my change as e26a9e00afc4 (ARM: Better virt_to_page()
> handling).  I can drop this from your patch, but it would be a good idea
> if you could re-validate against v3.15-rc1.

Hi Russell,

Thanks!  I'll validate it with e26a9e00afc4 added to my 3.13
hibernation branch, which includes the OMAP patches, and post the
results.  I can also verify against 3.15-rc1 + hibernation patches,
but there's not much to be done, other than compile test, as the
platform support is not there yet.  I can try rebasing the OMAP stuff
to v3.15-rc1, but I'm not sure what to expect out of that.

In any event, I'll let you know.

Thanks!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Hi Russell,

It seems to work fine with your virt_to_phys on the 3.13 + OMAP
patches kernel, and I checked on the 3.15-rc1 kernel + hibernation and
it compiled and ran fine.  I tried a couple of hibernations on this
version as well and they worked (aside from the crash in kernel_halt
we're discussing separately).  I believe this just works by chance
though because the omap platform has no real hibernation support
there.

I'll try now to merge up the omap patches to 3.15-rc1.

Thanks,

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Hi Russell,

Sorry for the delay.

I've merged the omap changes onto 3.15-rc2 and tested and it looks
like everything is working (so far 50 loops into/out of hibernation).

Thanks!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Hi Russell,

Ran this overnight and no failures on >1000 hibernations.

Looks good.
Thanks!

Sebastian

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Pavel Machek]

On Wed 2014-04-23 09:39:48, Sebastian Capella wrote:
> Hi Russell,
> 
> Ran this overnight and no failures on >1000 hibernations.

Congratulations. Looking forward to hibernate n900 on battery low :-).

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

[PATCH v7 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella]

Thanks Pavel!

I believe it's missed 3.15, so will need to verify again for 3.16.

Sebastian