From patchwork Tue Apr 12 16:30:27 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Frank Hofmann X-Patchwork-Id: 701271 Received: from smtp1.linux-foundation.org (smtp1.linux-foundation.org [140.211.169.13]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p3CGYHvv029601 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Tue, 12 Apr 2011 16:34:38 GMT Received: from daredevil.linux-foundation.org (localhost [127.0.0.1]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p3CGVghT001201; Tue, 12 Apr 2011 09:32:12 -0700 Received: from mxvs2.esa.t-systems.com (mxvs2.esa.t-systems.com [81.7.202.143]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p3CGUXqM001109 for ; Tue, 12 Apr 2011 09:30:35 -0700 Received: from unknown (HELO nl-exc-01.intra.local) ([82.210.235.24]) by mx.esa.t-systems.com with ESMTP; 12 Apr 2011 16:30:29 +0000 Received: from magrathea ([10.14.8.112]) by nl-exc-01.intra.local with Microsoft SMTPSVC(6.0.3790.3959); Tue, 12 Apr 2011 18:30:28 +0200 Date: Tue, 12 Apr 2011 17:30:27 +0100 (BST) From: Frank Hofmann To: Pavel Machek In-Reply-To: <20110412055958.GA3507@ucw.cz> Message-ID: References: <20110412055958.GA3507@ucw.cz> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 X-OriginalArrivalTime: 12 Apr 2011 16:30:28.0674 (UTC) FILETIME=[EF2D8220:01CBF92E] Received-SPF: pass (localhost is always allowed.) X-Spam-Status: No, hits=-3.627 required=5 tests=AWL, BAYES_00, OSDL_HEADER_SUBJECT_BRACKETED X-Spam-Checker-Version: SpamAssassin 3.2.4-osdl_revision__1.47__ X-MIMEDefang-Filter: lf$Revision: 1.188 $ X-Scanned-By: MIMEDefang 2.63 on 140.211.169.21 Cc: Frank Hofmann , linux-pm@lists.linux-foundation.org, tuxonice-devel@tuxonice.net, linux-arm-kernel@lists.infradead.org Subject: Re: [linux-pm] [RFC PATCH] Current status, suspend-to-disk support on ARM X-BeenThere: linux-pm@lists.linux-foundation.org X-Mailman-Version: 2.1.9 Precedence: list Reply-To: frank.hofmann@tomtom.com List-Id: Linux power management List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 12 Apr 2011 16:34:38 +0000 (UTC) On Tue, 12 Apr 2011, Pavel Machek wrote: > Hi! > >> There's one ugly thing in this set - I've changed a generic kernel >> header, to #define save/restore_processor_state() >> on ARM so that it only does preempt_disable/enable(). It's >> surprising that this isn't the default behaviour; all platforms need >> swsusp_arch_suspend/resume anyway so why force the existance of >> _two_ arch-specific hooks ? > > Can you submit separate patch cleaning it up? How about the attached ? It's for discussion, and hence not tested (admittedly, I need an x86 test system ...). The diff is against RMK's devel-stable tree, as of commit 5f183860d5007ec76ea36bfa6c36d66e37f0dbcf There's a few hurdles here: a) who knows all assembly calling conventions for all architectures supported by linux ? This applies to SH and S390; save/restore_processor_state on those are primitive and should be inlined within swsusp_arch_suspend/resume, just like the attached patch does for the powerpc variants. I've done a bounce call within S390 but don't know enough about SH3 for arch/sh/kernel/cpu/sh3/swsusp.S - i.e. add a call in assembly to init_cpu(current), and then ditch save_processor_state. b) some architectures do things _beyond_ register/state saving in those two functions, and hence rely on them being called paired. c) CONFIG_KEXEC_JUMP (ab-)uses them. I don't know that subsystem at all, but this sort of re-use means the symbols can't just be ditched. They become arch-dependent though, no non-arch/ code calls them anymore. What I've attached might break SH3 swsusp, as save_processor_state is no longer called there (it stores FPU state). I've also been wondering: The comments in kernel/power/hibernate.c mention the need to get preempt counts "right" as major motivator to call save/restore_processor_state. effect" of save/restore_processor_state). But then on all architectures in kernels as far back as 2.6.32 it doesn't look like save/restore_processor state are actually adjusting the preempt count; nowhere do they call preempt_enable/disable. What is and what is not necessary, really ? Finally, on things like e.g. the floating point context saves: Wouldn't this happen as part of freezing processes (in the early stages of suspend), and/or as part of device quiesce ? I wonder why e.g. powerpc does it explicitly; not doing so on ARM doesn't seem to cause problems. > >> @@ -195,6 +195,14 @@ config VECTORS_BASE >> help >> The base address of exception vectors. >> >> +config ARCH_HIBERNATION_POSSIBLE >> + bool >> + help >> + If the machine architecture supports suspend-to-disk >> + it should select this automatically for you. >> + Otherwise, say 'Y' at your own peril. >> + > > Good for debugging, but not good for mainline. How would this be done better ? FrankH. > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > arch/mips/power/hibernate.S | 8 ++++++- arch/powerpc/kernel/swsusp.c | 39 ------------------------------------ arch/powerpc/kernel/swsusp_32.S | 18 ++++++++++++++++ arch/powerpc/kernel/swsusp_asm64.S | 21 +++++++++++++++++++ arch/powerpc/kernel/swsusp_booke.S | 19 +++++++++++++++++ arch/s390/kernel/swsusp_asm64.S | 6 +++++ arch/sh/kernel/swsusp.c | 5 ---- arch/unicore32/kernel/hibernate.c | 9 -------- arch/x86/power/hibernate_asm_32.S | 4 ++- arch/x86/power/hibernate_asm_64.S | 2 + include/linux/suspend.h | 2 + kernel/power/hibernate.c | 10 ++++---- 12 files changed, 83 insertions(+), 60 deletions(-) diff --git a/arch/mips/power/hibernate.S b/arch/mips/power/hibernate.S index dbb5c7b..1b12ae1 100644 --- a/arch/mips/power/hibernate.S +++ b/arch/mips/power/hibernate.S @@ -27,6 +27,9 @@ LEAF(swsusp_arch_suspend) PTR_S s5, PT_R21(t0) PTR_S s6, PT_R22(t0) PTR_S s7, PT_R23(t0) + jal save_processor_state + PTR_LA t0, saved_regs + PTR_L ra, PT_R31(t0) j swsusp_save END(swsusp_arch_suspend) @@ -45,7 +48,6 @@ LEAF(swsusp_arch_resume) PTR_L t0, PBE_NEXT(t0) bnez t0, 0b PTR_LA t0, saved_regs - PTR_L ra, PT_R31(t0) PTR_L sp, PT_R29(t0) PTR_L fp, PT_R30(t0) PTR_L gp, PT_R28(t0) @@ -57,6 +59,10 @@ LEAF(swsusp_arch_resume) PTR_L s5, PT_R21(t0) PTR_L s6, PT_R22(t0) PTR_L s7, PT_R23(t0) + jal restore_processor_state + PTR_LA t0, saved_regs + PTR_L ra, PT_R31(t0) + PTR_LI v0, 0x0 jr ra END(swsusp_arch_resume) diff --git a/arch/powerpc/kernel/swsusp.c b/arch/powerpc/kernel/swsusp.c deleted file mode 100644 index 560c961..0000000 --- a/arch/powerpc/kernel/swsusp.c +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Common powerpc suspend code for 32 and 64 bits - * - * Copyright 2007 Johannes Berg - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * as published by the Free Software Foundation; either version - * 2 of the License, or (at your option) any later version. - */ - -#include -#include -#include -#include -#include - -void save_processor_state(void) -{ - /* - * flush out all the special registers so we don't need - * to save them in the snapshot - */ - flush_fp_to_thread(current); - flush_altivec_to_thread(current); - flush_spe_to_thread(current); - -#ifdef CONFIG_PPC64 - hard_irq_disable(); -#endif - -} - -void restore_processor_state(void) -{ -#ifdef CONFIG_PPC32 - switch_mmu_context(NULL, current->active_mm); -#endif -} diff --git a/arch/powerpc/kernel/swsusp_32.S b/arch/powerpc/kernel/swsusp_32.S index b0754e2..08137c5 100644 --- a/arch/powerpc/kernel/swsusp_32.S +++ b/arch/powerpc/kernel/swsusp_32.S @@ -116,6 +116,16 @@ _GLOBAL(swsusp_arch_suspend) /* Backup various CPU config stuffs */ bl __save_cpu_setup #endif + /* flush out all the special registers so we don't need + * to save them in the snapshot + */ + tophys(r4,r2); + bl flush_fp_to_thread + tophys(r4,r2); + bl flush_altivec_to_thread + tophys(r4,r2); + bl flush_spe_to_thread + /* Call the low level suspend stuff (we should probably have made * a stackframe... */ @@ -323,6 +333,14 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_USE_HIGH_BATS) li r0,1 mtdec r0 + /* restore MMU context - switch_mmu_context(NULL, current->mm); */ + li r3,0 + /* 'current->mm' needs to be in r4 */ + tophys(r4, r2) + lwz r4, MM(r4) + tophys(r4, r4) + bl switch_mmu_context + /* Restore the callee-saved registers and return */ lwz r0,SL_CR(r11) mtcr r0 diff --git a/arch/powerpc/kernel/swsusp_asm64.S b/arch/powerpc/kernel/swsusp_asm64.S index 86ac1d9..e46786d 100644 --- a/arch/powerpc/kernel/swsusp_asm64.S +++ b/arch/powerpc/kernel/swsusp_asm64.S @@ -76,8 +76,29 @@ restore_pblist_ptr: .section .text .align 5 _GLOBAL(swsusp_arch_suspend) + /* Save LR first so that we can make function calls */ ld r11,swsusp_save_area_ptr@toc(r2) SAVE_SPECIAL(LR) + + /* flush out all the special registers so we don't need + * to save them in the snapshot + */ + ld r3,PACACURRENT(r13) + bl flush_fp_to_thread + ld r3,PACACURRENT(r13) + bl flush_altivec_to_thread + ld r3,PACACURRENT(r13) + bl flush_spe_to_thread + /* Disable interrupts now */ + mfmsr r4 /* Get current interrupt state */ + rldicu r3,r4,48,1 /* clear MSR_EE */ + rotldi r3,r3,16 + mtmsrd r3,1 /* Update machine state */ + li r3,0 + stb r3,PACASOFTIRQEN(r13) + stb r3,PACAHARDIRQEN(r13) + + ld r11,swsusp_save_area_ptr@toc(r2) SAVE_REGISTER(r1) SAVE_SPECIAL(CR) SAVE_SPECIAL(TB) diff --git a/arch/powerpc/kernel/swsusp_booke.S b/arch/powerpc/kernel/swsusp_booke.S index 11a3930..529e95b 100644 --- a/arch/powerpc/kernel/swsusp_booke.S +++ b/arch/powerpc/kernel/swsusp_booke.S @@ -50,8 +50,27 @@ _GLOBAL(swsusp_arch_suspend) lis r11,swsusp_save_area@h ori r11,r11,swsusp_save_area@l + /* save LR first so we can make function calls */ mflr r0 stw r0,SL_LR(r11) + + /* flush out all the special registers so we don't need + * to save them in the snapshot + */ + ld r3,PACACURRENT(r13) + bl flush_fp_to_thread + ld r3,PACACURRENT(r13) + bl flush_altivec_to_thread + ld r3,PACACURRENT(r13) + bl flush_spe_to_thread + /* Disable interrupts now */ + wrteei 0 + li r0,0 + stb r0,PACASOFTIRQEN(r13) + stb r0,PACAHARDIRQEN(r13) + + lis r11,swsusp_save_area@h + ori r11,r11,swsusp_save_area@l mfcr r0 stw r0,SL_CR(r11) stw r1,SL_SP(r11) diff --git a/arch/s390/kernel/swsusp_asm64.S b/arch/s390/kernel/swsusp_asm64.S index 1f066e4..00cceff 100644 --- a/arch/s390/kernel/swsusp_asm64.S +++ b/arch/s390/kernel/swsusp_asm64.S @@ -25,6 +25,8 @@ .align 4 .globl swsusp_arch_suspend swsusp_arch_suspend: + brasl %r14,save_processor_state /* disables lowcore protection */ + stmg %r6,%r15,__SF_GPRS(%r15) lgr %r1,%r15 aghi %r15,-STACK_FRAME_OVERHEAD @@ -100,6 +102,8 @@ swsusp_arch_suspend: /* Save image */ brasl %r14,swsusp_save + brasl %r14,restore_processor_state + /* Restore prefix register and return */ lghi %r1,0x1000 spx 0x318(%r1) @@ -259,6 +263,8 @@ restore_registers: /* Reinitialize the channel subsystem */ brasl %r14,channel_subsystem_reinit + /* reenable lowcore protection */ + brasl %r14,restore_processor_state /* Return 0 */ lmg %r6,%r15,STACK_FRAME_OVERHEAD + __SF_GPRS(%r15) lghi %r2,0 diff --git a/arch/sh/kernel/swsusp.c b/arch/sh/kernel/swsusp.c index 12b64a0..65a964e 100644 --- a/arch/sh/kernel/swsusp.c +++ b/arch/sh/kernel/swsusp.c @@ -31,8 +31,3 @@ void save_processor_state(void) { init_fpu(current); } - -void restore_processor_state(void) -{ - local_flush_tlb_all(); -} diff --git a/arch/unicore32/kernel/hibernate.c b/arch/unicore32/kernel/hibernate.c index 7d0f0b7..aba4ad4 100644 --- a/arch/unicore32/kernel/hibernate.c +++ b/arch/unicore32/kernel/hibernate.c @@ -149,12 +149,3 @@ int pfn_is_nosave(unsigned long pfn) return (pfn >= begin_pfn) && (pfn < end_pfn); } - -void save_processor_state(void) -{ -} - -void restore_processor_state(void) -{ - local_flush_tlb_all(); -} diff --git a/arch/x86/power/hibernate_asm_32.S b/arch/x86/power/hibernate_asm_32.S index ad47dae..6827e1b 100644 --- a/arch/x86/power/hibernate_asm_32.S +++ b/arch/x86/power/hibernate_asm_32.S @@ -15,6 +15,7 @@ .text ENTRY(swsusp_arch_suspend) + call save_processor_state movl %esp, saved_context_esp movl %ebx, saved_context_ebx movl %ebp, saved_context_ebp @@ -22,7 +23,6 @@ ENTRY(swsusp_arch_suspend) movl %edi, saved_context_edi pushfl popl saved_context_eflags - call swsusp_save ret @@ -75,6 +75,8 @@ done: pushl saved_context_eflags popfl + call restore_processor_state + xorl %eax, %eax ret diff --git a/arch/x86/power/hibernate_asm_64.S b/arch/x86/power/hibernate_asm_64.S index 9356547..1318398 100644 --- a/arch/x86/power/hibernate_asm_64.S +++ b/arch/x86/power/hibernate_asm_64.S @@ -23,6 +23,7 @@ #include ENTRY(swsusp_arch_suspend) + call save_processor_state movq $saved_context, %rax movq %rsp, pt_regs_sp(%rax) movq %rbp, pt_regs_bp(%rax) @@ -139,6 +140,7 @@ ENTRY(restore_registers) pushq pt_regs_flags(%rax) popfq + call restore_processor_state xorq %rax, %rax /* tell the hibernation core that we've just restored the memory */ diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 5a89e36..145d9a4 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -259,8 +259,10 @@ static inline bool system_entering_hibernation(void) { return false; } #endif /* CONFIG_HIBERNATION */ #ifdef CONFIG_PM_SLEEP +#ifdef CONFIG_KEXEC_JUMP void save_processor_state(void); void restore_processor_state(void); +#endif /* kernel/power/main.c */ extern int register_pm_notifier(struct notifier_block *nb); diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index aeabd26..554b741 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -285,13 +285,14 @@ static int create_image(int platform_mode) goto Power_up; in_suspend = 1; - save_processor_state(); + preempt_disable(); error = swsusp_arch_suspend(); if (error) printk(KERN_ERR "PM: Error %d creating hibernation image\n", error); /* Restore control flow magically appears here */ - restore_processor_state(); + flush_tlb_all(); + preempt_enable(); if (!in_suspend) { events_check_enabled = false; platform_leave(platform_mode); @@ -412,8 +413,7 @@ static int resume_target_kernel(bool platform_mode) if (error) goto Enable_irqs; - /* We'll ignore saved state, but this gets preempt count (etc) right */ - save_processor_state(); + preempt_disable(); error = restore_highmem(); if (!error) { error = swsusp_arch_resume(); @@ -432,7 +432,7 @@ static int resume_target_kernel(bool platform_mode) * subsequent failures */ swsusp_free(); - restore_processor_state(); + preempt_enable(); touch_softlockup_watchdog(); syscore_resume();