Re: Help port swsusp to ppc.

[Pavel Machek <pavel@ucw.cz>]

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?]