swsusp does not stop DMA properly during resume

swsusp does not stop DMA properly during resume

[Pavel Machek]

Hi!

As Ben pointed out, swsusp is not doing the right thing with devices
in 2.6.1. I had patch for a long time here, and it needs to go
in... It stops them before copying pages back, so there are no issues
with running DMAs etc.

								Pavel

Ben:

> 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
> 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.

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

Re: swsusp does not stop DMA properly during resume

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
> 
> As Ben pointed out, swsusp is not doing the right thing with devices
> in 2.6.1. I had patch for a long time here, and it needs to go
> in... It stops them before copying pages back, so there are no issues
> with running DMAs etc.

I _think_ what this patch is doing is suspending all devices from within
the boot kernel before starting into the resumed kernel.  Is this correct?

> +	update_screen(fg_console);	/* Hmm, is this the problem? */

Cryptic comment.  To what "problem" does this refer?

Re: swsusp does not stop DMA properly during resume

[Pavel Machek]

Hi!

> > As Ben pointed out, swsusp is not doing the right thing with devices
> > in 2.6.1. I had patch for a long time here, and it needs to go
> > in... It stops them before copying pages back, so there are no issues
> > with running DMAs etc.
> 
> I _think_ what this patch is doing is suspending all devices from within
> the boot kernel before starting into the resumed kernel.  Is this
> correct?

Yes.

> > +	update_screen(fg_console);	/* Hmm, is this the problem? */
> 
> Cryptic comment.  To what "problem" does this refer?

Ugh, I am afraid I forgot waht this one means. I was seing some data
corruption but that was solved...

								Pavel

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp does not stop DMA properly during resume

[Benjamin Herrenschmidt]

> I _think_ what this patch is doing is suspending all devices from within
> the boot kernel before starting into the resumed kernel.  Is this correct?
> 
> > +	update_screen(fg_console);	/* Hmm, is this the problem? */
> 
> Cryptic comment.  To what "problem" does this refer?

Note that you should make sure all calls to update_screen (among others)
are guarded by the console semaphore, with my VT patch, not doing so
will result in WARN_ON's

Ben.

Re: swsusp does not stop DMA properly during resume

[Pavel Machek]

Hi!

> > I _think_ what this patch is doing is suspending all devices from within
> > the boot kernel before starting into the resumed kernel.  Is this correct?
> > 
> > > +	update_screen(fg_console);	/* Hmm, is this the problem? */
> > 
> > Cryptic comment.  To what "problem" does this refer?
> 
> Note that you should make sure all calls to update_screen (among others)
> are guarded by the console semaphore, with my VT patch, not doing so
> will result in WARN_ON's

Hmmm... yes, I'll need to fix that one. [Is there console semaphore in
2.6.1, or do I need to wait for 2.6.2 before I can do something with
this?]

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp does not stop DMA properly during resume

[Andrew Morton]

Pavel Machek <pavel@suse.cz> wrote:
>
> Hi!
> 
> > > I _think_ what this patch is doing is suspending all devices from within
> > > the boot kernel before starting into the resumed kernel.  Is this correct?
> > > 
> > > > +	update_screen(fg_console);	/* Hmm, is this the problem? */
> > > 
> > > Cryptic comment.  To what "problem" does this refer?
> > 
> > Note that you should make sure all calls to update_screen (among others)
> > are guarded by the console semaphore, with my VT patch, not doing so
> > will result in WARN_ON's
> 
> Hmmm... yes, I'll need to fix that one. [Is there console semaphore in
> 2.6.1, or do I need to wait for 2.6.2 before I can do something with
> this?]

It's acquire_console_sem()/release_console_sem().

I'll change the patch.  Could you please test it all in next -mm, let me
know?

Re: swsusp does not stop DMA properly during resume

[Pavel Machek]

Hi!

> > > > I _think_ what this patch is doing is suspending all devices from within
> > > > the boot kernel before starting into the resumed kernel.  Is this correct?
> > > > 
> > > > > +	update_screen(fg_console);	/* Hmm, is this the problem? */
> > > > 
> > > > Cryptic comment.  To what "problem" does this refer?
> > > 
> > > Note that you should make sure all calls to update_screen (among others)
> > > are guarded by the console semaphore, with my VT patch, not doing so
> > > will result in WARN_ON's
> > 
> > Hmmm... yes, I'll need to fix that one. [Is there console semaphore in
> > 2.6.1, or do I need to wait for 2.6.2 before I can do something with
> > this?]
> 
> It's acquire_console_sem()/release_console_sem().
> 
> I'll change the patch.  Could you please test it all in next -mm, let me
> know?

Okay, no problem.
									Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp does not stop DMA properly during resume

[Benjamin Herrenschmidt]

> Hmmm... yes, I'll need to fix that one. [Is there console semaphore in
> 2.6.1, or do I need to wait for 2.6.2 before I can do something with
> this?]

{acquire,release}_console_sem() is in. I just added the warnings.

I also added proper take/release of it in the kernel/power/console.c
code, so you shouldn't have to touch that part

Ben.