swsusp does not stop DMA properly during resume
Subsystems (2)
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:
Quoted 9 lines
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 calldevices_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 calldevices_suspend(1), and we should do that for kexec too.
kernel/power/swsusp.c +9 -30
@@ -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" );
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 7 lines · 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?
kernel/power/swsusp.c line 614
Quoted 1 line
kernel/power/swsusp.c +1
-0+update_screen (fg_console);/* Hmm, is this the problem? */
Cryptic comment. To what "problem" does this refer?
Hi!
Quoted 8 lines
Quoted 4 lines
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.
Quoted 3 lines
kernel/power/swsusp.c line 614
Quoted 1 line
kernel/power/swsusp.c +1
-0+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
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 7 lines
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?
kernel/power/swsusp.c line 614
Quoted 1 line
kernel/power/swsusp.c +1
-0+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.
Hi!
Quoted 10 lines
Quoted 6 lines
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?
kernel/power/swsusp.c line 614
Quoted 1 line
kernel/power/swsusp.c +1
-0+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
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 10 lines · Pavel Machek
Hi!
Quoted 7 lines
Quoted 6 lines
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?
kernel/power/swsusp.c line 614
Quoted 1 line
kernel/power/swsusp.c +1
-0+update_screen (fg_console);/* Hmm, is this the problem? */ Cryptic comment. To what "problem" does this refer?
Quoted 7 lines
Quoted 3 lines
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'sHmmm... 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?
Hi!
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
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?
kernel/power/swsusp.c line 614
Quoted 1 line
kernel/power/swsusp.c +1
-0+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
Quoted 8 lines
Quoted 3 lines
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
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 4 lines
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.