Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S265802AbUATWo1 (ORCPT ); Tue, 20 Jan 2004 17:44:27 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S265814AbUATWo0 (ORCPT ); Tue, 20 Jan 2004 17:44:26 -0500 Received: from gprs214-112.eurotel.cz ([160.218.214.112]:6272 "EHLO amd.ucw.cz") by vger.kernel.org with ESMTP id S265802AbUATWoH (ORCPT ); Tue, 20 Jan 2004 17:44:07 -0500 Date: Tue, 20 Jan 2004 23:42:37 +0100 From: Pavel Machek To: Benjamin Herrenschmidt Cc: ncunningham@users.sourceforge.net, Hugang , ncunningham@clear.net.nz, Linux Kernel Mailing List , debian-powerpc@lists.debian.org Subject: Re: Help port swsusp to ppc. Message-ID: <20040120224237.GB1192@elf.ucw.cz> References: <20040119105237.62a43f65@localhost> <1074483354.10595.5.camel@gaston> <1074489645.2111.8.camel@laptop-linux> <1074490463.10595.16.camel@gaston> <1074534964.2505.6.camel@laptop-linux> <1074549790.10595.55.camel@gaston> <20040120204407.GA9749@elf.ucw.cz> <1074635683.795.43.camel@gaston> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1074635683.795.43.camel@gaston> X-Warning: Reading this can be dangerous to your mental health. User-Agent: Mutt/1.5.4i Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6173 Lines: 174 Hi! > > FYI, this is that "ugly C-generated assembly" we are talking about. I > > do not think it is so bad. > > The x86 version has been cleaned up and isn't _that_ bad, though it > could definitely use some comments and I don't like the "Lxxx" labels, > I'd rather either use number with the "nb" or "nf" GAS constructs or > use real words labels. Looking at it though, I fail to see the need > to get it generated by gcc in the first place :) Well, I had C version that was working (and it was slightly easier to code it in C), and really did not want to introduce errors at that point. > The PPC version that was proposed is horrible. Sorry, I missed that one. > > FYI, there are exactly 6 variables in "nosave" section. Two loop > > variables you can see in above code, one spinlock, number of pages to > > save, pointer to directory of pages to be copied, and its length. > > > > I could probably move spinlock and length of pgdir out of there... > > That's not too much, you could probably afford having a one page header > to the suspend image with those informations and the page copy loop > (provided by the suspended kernel so you don't have _any_ compatibility > issue and can even do it from the bootloader one day...) Unfortunately the copy loop has quite complicated interface. It expects tables already loaded into memory, at non-conflicting addresses etc. > > > device_suspend is, imho, hairy too. We have some semantics that need > > > cleanup here, I'll have to talk to Patrick about them. Putting devices > > > to an idle state is what you need and what kexec need, and doesn't mean > > > putting them to _sleep_. Or maybe we could pass a specific state to > > > > Okay, I can agree that putting them into sleep is not ideal [it > > puzzles users, at least]. But it is quite simple and should work > > okay. I do not want another round of device_suspend changes in > > 2.6.X... [Well, perhaps if it was done in right&compatible way (tm), > > it would be acceptable...] > > We already have the shutdown() callback or we can simply use device_suspend() > with a D1 parameter instead of D3 or D4. That's what I'd do... We need to do something with those 1,3s and 4s. IIRC there's no such thing as D4 (or is it expected to be == D3cold?). This needs to be documented somewhere. > Looking at swsusp code in current 2.6, when do you do that pass ? On the > shutdown pass, you call devices_suspend(4); which is fine. But I don't see > where you call devices_suspend(X) on the resume path. IMHO, that > should be My fault, the patch is in my tree only. (Its here for quite a long time. I synced it to Patrick and then he disappeared.). > done from the boot kernel after loading the suspend image and before > starting the resume process. Your mdelay(1000) for waiting for DMA to settle > down is PLAIN WRONG imho, even dangerous. (And typically, an USB controller > will still be hapilly be DMA'ing all over your memory). That's what I call > idling devices at this point, and that's where I'd call devices_suspend(1), > and we should do that for kexec too. Yes, that's right solution. Here's the patch. Index: linux/kernel/power/swsusp.c =================================================================== --- linux.orig/kernel/power/swsusp.c 2004-01-13 22:52:40.000000000 +0100 +++ linux/kernel/power/swsusp.c 2004-01-09 20:33:05.000000000 +0100 @@ -488,33 +492,6 @@ printk("|\n"); } -/* Make disk drivers accept operations, again */ -static void drivers_unsuspend(void) -{ - device_resume(); -} - -/* Called from process context */ -static int drivers_suspend(void) -{ - return device_suspend(4); -} - -#define RESUME_PHASE1 1 /* Called from interrupts disabled */ -#define RESUME_PHASE2 2 /* Called with interrupts enabled */ -#define RESUME_ALL_PHASES (RESUME_PHASE1 | RESUME_PHASE2) -static void drivers_resume(int flags) -{ - if (flags & RESUME_PHASE1) { - device_resume(); - } - if (flags & RESUME_PHASE2) { -#ifdef SUSPEND_CONSOLE - update_screen(fg_console); /* Hmm, is this the problem? */ -#endif - } -} - static int suspend_prepare_image(void) { struct sysinfo i; @@ -569,7 +546,7 @@ static void suspend_save_image(void) { - drivers_unsuspend(); + device_resume(); lock_swapdevices(); write_suspend_image(); @@ -615,6 +592,7 @@ mb(); spin_lock_irq(&suspend_pagedir_lock); /* Done to disable interrupts */ + device_power_down(4); PRINTK( "Waiting for DMAs to settle down...\n"); mdelay(1000); /* We do not want some readahead with DMA to corrupt our memory, right? Do it with disabled interrupts for best effect. That way, if some @@ -630,8 +608,10 @@ PRINTK( "Freeing prev allocated pagedir\n" ); free_suspend_pagedir((unsigned long) pagedir_save); + device_power_up(); spin_unlock_irq(&suspend_pagedir_lock); - drivers_resume(RESUME_ALL_PHASES); + device_resume(); + update_screen(fg_console); /* Hmm, is this the problem? */ PRINTK( "Fixing swap signatures... " ); mark_swapfiles(((swp_entry_t) {0}), MARK_SWAP_RESUME); @@ -672,7 +652,9 @@ { int is_problem; read_swapfiles(); + device_power_down(4); is_problem = suspend_prepare_image(); + device_power_up(); spin_unlock_irq(&suspend_pagedir_lock); if (!is_problem) { kernel_fpu_end(); /* save_processor_state() does kernel_fpu_begin, and we need to revert it in order to pass in_atomic() checks */ @@ -716,7 +709,7 @@ blk_run_queues(); /* Save state of all device drivers, and stop them. */ - if(drivers_suspend()==0) + if ((res = device_suspend(4))==0) /* If stopping device drivers worked, we proceed basically into * suspend_save_image. * @@ -1091,6 +1072,7 @@ printk( "resuming from %s\n", resume_file); if (read_suspend_image(resume_file, 0)) goto read_failure; + device_suspend(4); do_magic(1); panic("This never returns"); -- When do you have a heart between your knees? [Johanka's followup: and *two* hearts?] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/