-mm swsusp: fix highmem handling

-mm swsusp: fix highmem handling

[Pavel Machek]

Hi!

Swsusp was not restoring highmem properly. I did not find a nice place
where to restore it, through, so it went to swsusp_free.

I'm not sure why you are saving state before
save_processor_state. swsusp_arch_resume will overwrite this,
anyway. Is it to make something balanced?
								Pavel

--- clean-mm/kernel/power/swsusp.c	2004-07-28 23:39:49.000000000 +0200
+++ linux-mm/kernel/power/swsusp.c	2004-07-28 23:30:33.000000000 +0200
@@ -656,6 +652,10 @@
 			free_suspend_pagedir_zone(zone, p);
 	}
 	free_pages(p, pagedir_order);
+#ifdef CONFIG_HIGHMEM
+	printk( "Restoring highmem\n" );
+	restore_highmem();
+#endif
 }
 
 
@@ -890,7 +890,6 @@
 {
 	int error;
 	local_irq_disable();
-	save_processor_state();
 	error = swsusp_arch_resume();
 	restore_processor_state();
 	local_irq_enable();

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: -mm swsusp: fix highmem handling

[Patrick Mochel]

On Thu, 29 Jul 2004, Pavel Machek wrote:

> Swsusp was not restoring highmem properly. I did not find a nice place
> where to restore it, through, so it went to swsusp_free.

That's too late, and will be called if suspend failed, to clean up any
allocated memory. The proper place seems to be in swsusp_resume().

> I'm not sure why you are saving state before save_processor_state.
> swsusp_arch_resume will overwrite this, anyway. Is it to make something
> balanced?

Yes, so it matches the calls in swsusp_suspend() - Previously there was a
hack that did kernel_fpu_end() after calling save_processor_state(), to
pass in_atomic() checks. By restoring the state after we've snapshotted on
suspend prevents this from being a problem.

In general, if we assume that save_processor_state() does anything to the
CPU, besides just benign register saving, we have to make sure that it's
put into the same state on resume before we restore state..


	Pat

Re: -mm swsusp: fix highmem handling

[Pavel Machek]

Hi!

> > I'm not sure why you are saving state before save_processor_state.
> > swsusp_arch_resume will overwrite this, anyway. Is it to make something
> > balanced?
> 
> Yes, so it matches the calls in swsusp_suspend() - Previously there was a
> hack that did kernel_fpu_end() after calling save_processor_state(), to
> pass in_atomic() checks. By restoring the state after we've snapshotted on
> suspend prevents this from being a problem.

> In general, if we assume that save_processor_state() does anything to the
> CPU, besides just benign register saving, we have to make sure that it's
> put into the same state on resume before we restore state..

Perhaps comment is needed there? "state saved by this is ignored, but
save_processor_state changes preempt count"? 

									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: -mm swsusp: fix highmem handling

[Pavel Machek]

Hi!


> Swsusp was not restoring highmem properly. I did not find a nice place
> where to restore it, through, so it went to swsusp_free.
> 
> I'm not sure why you are saving state before
> save_processor_state. swsusp_arch_resume will overwrite this,
> anyway. Is it to make something balanced?

On your suggestion (sorry, I do not have it nearby) I tried moving
restore_highmem into swsusp_resume, and it crashed. I'll try moving it
around.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

-mm swsusp: fix highmem handling

[Pavel Machek]

Hi!

This fixes highmem handling, and adds some comments so that others do
not fall into the same trap I fallen in: code does not continue below
swsusp_arch_resume if things go okay.

Please apply,
								Pavel

--- clean-mm/kernel/power/swsusp.c	2004-07-28 23:39:49.000000000 +0200
+++ linux-mm/kernel/power/swsusp.c	2004-08-09 11:54:04.000000000 +0200
@@ -870,8 +866,12 @@
 	local_irq_disable();
 	save_processor_state();
 	error = swsusp_arch_suspend();
+	/* Restore control flow magically appears here */
 	restore_processor_state();
 	local_irq_enable();
+#ifdef CONFIG_HIGHMEM
+	restore_highmem();
+#endif
 	return error;
 }
 
@@ -890,8 +889,12 @@
 {
 	int error;
 	local_irq_disable();
 	save_processor_state();
 	error = swsusp_arch_resume();
+	/* Code below is only ever reached in case of failure. Otherwise
+	 * execution continues at place where swsusp_arch_suspend was called
+         */
+	BUG_ON(!error);
 	restore_processor_state();
 	local_irq_enable();
 	return error;



-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: -mm swsusp: fix highmem handling

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> This fixes highmem handling, and adds some comments so that others do
> not fall into the same trap I fallen in: code does not continue below
> swsusp_arch_resume if things go okay.
> 
> Please apply,
> 								Pavel
> 
> --- clean-mm/kernel/power/swsusp.c	2004-07-28 23:39:49.000000000 +0200
> +++ linux-mm/kernel/power/swsusp.c	2004-08-09 11:54:04.000000000 +0200
> @@ -870,8 +866,12 @@
>  	local_irq_disable();
>  	save_processor_state();
>  	error = swsusp_arch_suspend();
> +	/* Restore control flow magically appears here */
>  	restore_processor_state();
>  	local_irq_enable();
> +#ifdef CONFIG_HIGHMEM
> +	restore_highmem();
> +#endif
>  	return error;

restore_highmem() is a noop if !CONFIG_HIGHMEM, so I shall remove the
above ifdef/endif.

Re: -mm swsusp: fix highmem handling

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> This fixes highmem handling, and adds some comments so that others do
> not fall into the same trap I fallen in: code does not continue below
> swsusp_arch_resume if things go okay.

Actually these changes seem to be in Pat's tree already.  Please check next
-mm, raise a patch against that.