[pm] fix oops after saving image

[pm] fix oops after saving image

[Pavel Machek]

Hi!

I was seeing strange oopsen after saving image. They are almost
harmless: anything that happens after writing final signature does not
matter (as long as  it does not write to disk). But that oops makes
testing hard as you have to manually powerdown the machine.

free_page() at this point is unneccessary as machine is going down,
anyway. Please apply,
								Pavel
PS: For a test, I'm running while true; do echo 4 > /proc/acpi/sleep;
sleep 30; done while making kernel. It seems to work so far.

--- tmp/linux/kernel/power/swsusp.c	2003-10-02 00:04:35.000000000 +0200
+++ linux/kernel/power/swsusp.c	2003-10-01 23:56:49.000000000 +0200
@@ -345,7 +348,7 @@
 	printk( "|\n" );
 
 	MDELAY(1000);
-	free_page((unsigned long) buffer);
+	/* Trying to free_page((unsigned long) buffer) here is bad idea, not sure why */
 	return 0;
 }
 
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [pm] fix oops after saving image

[Patrick Mochel]

> --- tmp/linux/kernel/power/swsusp.c	2003-10-02 00:04:35.000000000 +0200
> +++ linux/kernel/power/swsusp.c	2003-10-01 23:56:49.000000000 +0200
> @@ -345,7 +348,7 @@
>  	printk( "|\n" );
>  
>  	MDELAY(1000);
> -	free_page((unsigned long) buffer);
> +	/* Trying to free_page((unsigned long) buffer) here is bad idea, not sure why */
>  	return 0;
>  }

Patches like this really do a disservice to anyone trying to read the code 
and figure out what is going on. I've spent a considerable amount of time 
deciphering and santizing the swsusp code, which is why pmdisk exists. 

The patch is simply a band-aid, and completely meaningless without the 
context of the email. If I applied this, one would be able to ascertain 
the reason for the patch, if they manipulated the BK tools correctly. 
However, seeing that line solely in the context on the rest of the source 
makes one cock their head, squint their eyes and pray that they never have 
to look at that file again. 

If you're seeing an Oops, please search more for the cause and submit a 
real fix for it. 

Or, simply change the semantics of the code enough to eliminate the
possibility of a problem. In the pmdisk code, I've statically declared the 
header, so we don't need that alloc/free. Please see that file for an 
example. 


	Pat

Re: [pm] fix oops after saving image

[Pavel Machek]

Hi!

> > --- tmp/linux/kernel/power/swsusp.c	2003-10-02 00:04:35.000000000 +0200
> > +++ linux/kernel/power/swsusp.c	2003-10-01 23:56:49.000000000 +0200
> > @@ -345,7 +348,7 @@
> >  	printk( "|\n" );
> >  
> >  	MDELAY(1000);
> > -	free_page((unsigned long) buffer);
> > +	/* Trying to free_page((unsigned long) buffer) here is bad idea, not sure why */
> >  	return 0;
> >  }
> 
> Patches like this really do a disservice to anyone trying to read the code 
> and figure out what is going on. I've spent a considerable amount of time 
> deciphering and santizing the swsusp code, which is why pmdisk exists. 
> 
> The patch is simply a band-aid, and completely meaningless without the 
> context of the email. If I applied this, one would be able to ascertain 
> the reason for the patch, if they manipulated the BK tools correctly. 
> However, seeing that line solely in the context on the rest of the source 
> makes one cock their head, squint their eyes and pray that they never have 
> to look at that file again. 
> 
> If you're seeing an Oops, please search more for the cause and submit a 
> real fix for it. 
> 
> Or, simply change the semantics of the code enough to eliminate the
> possibility of a problem. In the pmdisk code, I've statically declared the 
> header, so we don't need that alloc/free. Please see that file for an 
> example. 

I do not want to waste 4K, does this look better?
									Pavel

--- tmp/linux/kernel/power/swsusp.c	2003-10-02 22:29:06.000000000 +0200
+++ linux/kernel/power/swsusp.c	2003-10-02 22:27:07.000000000 +0200
@@ -283,6 +283,9 @@
 	unsigned long address;
 	struct page *page;
 
+	if (!buffer)
+		return -ENOMEM;
+
 	printk( "Writing data to swap (%d pages): ", nr_copy_pages );
 	for (i=0; i<nr_copy_pages; i++) {
 		if (!(i%100))
@@ -345,7 +348,7 @@
 	printk( "|\n" );
 
 	MDELAY(1000);
-	free_page((unsigned long) buffer);
+	/* No need to free anything, system is going down, anyway. */
 	return 0;
 }
 


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

Re: [pm] fix oops after saving image

[Patrick Mochel]

> I do not want to waste 4K, does this look better?
> 									Pavel
> 
> --- tmp/linux/kernel/power/swsusp.c	2003-10-02 22:29:06.000000000 +0200
> +++ linux/kernel/power/swsusp.c	2003-10-02 22:27:07.000000000 +0200
> @@ -283,6 +283,9 @@
>  	unsigned long address;
>  	struct page *page;
>  
> +	if (!buffer)
> +		return -ENOMEM;
> +
>  	printk( "Writing data to swap (%d pages): ", nr_copy_pages );
>  	for (i=0; i<nr_copy_pages; i++) {
>  		if (!(i%100))

Argh! This bit was in the previous patch I applied. Please get it 
straight, or just keep adding to one patch and resending it (with an 
itemized list of changes). 

> @@ -345,7 +348,7 @@
>  	printk( "|\n" );
>  
>  	MDELAY(1000);
> -	free_page((unsigned long) buffer);
> +	/* No need to free anything, system is going down, anyway. */
>  	return 0;
>  }

It's technically still incorrect, since you'd still be leaking memory if
suspend failed. And, the comment is still in an unfortunate place.  
Something like this, before the function helps to provide understanding of 
the entire operation:


/**
 *	write_suspend_image - Write entire image to disk. 
 *
 * 	...
 *
 *	Note: The buffer we allocate to use to write the suspend header is 
 *	not freed because it otherwise causes a random Oops that I can't 
 *	find. This is a band-aid that probably only masks the problem, and 
 *	technically bad, since it means we leak memory unconditionally if 
 *	suspend fails. 
 */

static int write_suspend_image(void)
...



	Pat

Re: [pm] fix oops after saving image

[Pavel Machek]

Hi!

> > I do not want to waste 4K, does this look better?
> > 									Pavel
> > 
> > --- tmp/linux/kernel/power/swsusp.c	2003-10-02 22:29:06.000000000 +0200
> > +++ linux/kernel/power/swsusp.c	2003-10-02 22:27:07.000000000 +0200
> > @@ -283,6 +283,9 @@
> >  	unsigned long address;
> >  	struct page *page;
> >  
> > +	if (!buffer)
> > +		return -ENOMEM;
> > +
> >  	printk( "Writing data to swap (%d pages): ", nr_copy_pages );
> >  	for (i=0; i<nr_copy_pages; i++) {
> >  		if (!(i%100))
> 
> Argh! This bit was in the previous patch I applied. Please get it 
> straight, or just keep adding to one patch and resending it (with an 
> itemized list of changes). 

I do not want to do that (other people would have problems
reviewing). I'll try to be more carefull.

One thing would help me: could you reply with "Applied" when you apply
some patch? Just now it seems to be silence==applied and reply==some
problem, but that makes it "interesting" to guess what your tree looks
like.

> > @@ -345,7 +348,7 @@
> >  	printk( "|\n" );
> >  
> >  	MDELAY(1000);
> > -	free_page((unsigned long) buffer);
> > +	/* No need to free anything, system is going down, anyway. */
> >  	return 0;
> >  }
> 
> It's technically still incorrect, since you'd still be leaking memory if
> suspend failed. And, the comment is still in an unfortunate place.  
> Something like this, before the function helps to provide understanding of 
> the entire operation:

At this point, suspend may no longer fail: we have ready-to-run image
in swap, and rollback would happen on next reboot -- corrupting
data. Yep better docs is needed.

How about this one?
								Pavel

--- tmp/linux/kernel/power/swsusp.c	2003-10-04 00:21:55.000000000 +0200
+++ linux/kernel/power/swsusp.c	2003-10-04 00:17:19.000000000 +0200
@@ -274,6 +274,17 @@
 	swap_list_unlock();
 }
 
+/**
+ *    write_suspend_image - Write entire image to disk.
+ *
+ *    After writing suspend signature to the disk, suspend may no
+ *    longer fail: we have ready-to-run image in swap, and rollback
+ *    would happen on next reboot -- corrupting data.
+ *
+ *    Note: The buffer we allocate to use to write the suspend header is
+ *    not freed; its not needed since system is going down anyway
+ *    (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
+ */
 static int write_suspend_image(void)
 {
 	int i;
@@ -345,7 +359,6 @@
 	printk( "|\n" );
 
 	MDELAY(1000);
-	free_page((unsigned long) buffer);
 	return 0;
 }
 

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

Re: [pm] fix oops after saving image

[Stan Bubrouski]

Pavel Machek wrote:
> + *    Note: The buffer we allocate to use to write the suspend header is
> + *    not freed; its not needed since system is going down anyway
> + *    (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
> + */

Too lazy to properly fix your comment as well.

-sb

Re: [pm] fix oops after saving image

[Pavel Machek]

Hi!

> >+ *    Note: The buffer we allocate to use to write the suspend header is
> >+ *    not freed; its not needed since system is going down anyway
> >+ *    (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
> >+ */
> 
> Too lazy to properly fix your comment as well.

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

Re: [pm] fix oops after saving image

[Nigel Cunningham]

I guess he means...

On Sat, 2003-10-04 at 20:02, Pavel Machek wrote:
> > >+ *    not freed; its not needed since system is going down anyway

should be

> >+ *    not freed; it's not needed since the system is going down anyway

(it's = it is, its = belongs to it)

and

> > >+ *    (plus it causes oops and I'm lazy^H^H^H^Htoo busy).

should be

> >+ *    (plus it causes an oops and I'm too lazy^H^H^H^Hbusy).

Regards,

Nigel
-- 
Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

You see, at just the right time, when we were still powerless,
Christ died for the ungodly.
	-- Romans 5:6, NIV.

Re: [pm] fix oops after saving image

[Stan Bubrouski]

Yeah that too :)

Nigel Cunningham wrote:

> I guess he means...
> 
> On Sat, 2003-10-04 at 20:02, Pavel Machek wrote:
> 
>>>>+ *    not freed; its not needed since system is going down anyway
> 
> 
> should be
> 
> 
>>>+ *    not freed; it's not needed since the system is going down anyway
> 
> 
> (it's = it is, its = belongs to it)
> 
> and
> 
> 
>>>>+ *    (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
> 
> 
> should be
> 
> 
>>>+ *    (plus it causes an oops and I'm too lazy^H^H^H^Hbusy).
> 
> 
> Regards,
> 
> Nigel

Re: [pm] fix oops after saving image

[Stan Bubrouski]

Pavel Machek wrote:
> Hi!
> 
> 
>>>+ *    Note: The buffer we allocate to use to write the suspend header is
>>>+ *    not freed; its not needed since system is going down anyway
>>>+ *    (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
>>>+ */
>>
>>Too lazy to properly fix your comment as well.
> 
> 
> Can you elaborate?
> 								Pavel

Pavel what mail client are you using?  The last comment
reads:
+ *    (plus it causes oops and I'm lazy<CTRL-H><CTRL-H><CTRL-H><CTRL-H>too busy).

I spelled out the ^H so you can see them, that is how
the comment looks to me.  The word 'lazy' is still in
there along with too busy, you never backspaced over
the lazy in the comment.  That's all :P

-sb

Re: [pm] fix oops after saving image

[Pavel Machek]

Hi!

> >>>+ *    Note: The buffer we allocate to use to write the suspend header is
> >>>+ *    not freed; its not needed since system is going down anyway
> >>>+ *    (plus it causes oops and I'm lazy^H^H^H^Htoo busy).
> >>>+ */
> >>
> >>Too lazy to properly fix your comment as well.
> >
> >
> >Can you elaborate?
> >								Pavel
> 
> Pavel what mail client are you using?  The last comment
> reads:
> + *    (plus it causes oops and I'm lazy<CTRL-H><CTRL-H><CTRL-H><CTRL-H>too 
> busy).
> 
> I spelled out the ^H so you can see them, that is how
> the comment looks to me.  The word 'lazy' is still in
> there along with too busy, you never backspaced over
> the lazy in the comment.  That's all :P

:-))))))))))))))))))))))

No, my mail client is okay. I actually wrote ^ and H, and that should
be a joke.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [pm] fix oops after saving image

[Stan Bubrouski]

Pavel Machek wrote:

> 
> :-))))))))))))))))))))))
> 
> No, my mail client is okay. I actually wrote ^ and H, and that should
> be a joke.
> 								Pavel

lol my b you da man :)

-sb

Re: [pm] fix oops after saving image

[Patrick Mochel]

> One thing would help me: could you reply with "Applied" when you apply
> some patch? Just now it seems to be silence==applied and reply==some
> problem, but that makes it "interesting" to guess what your tree looks
> like.

Yes, I'll try to be better about this. 

> At this point, suspend may no longer fail: we have ready-to-run image
> in swap, and rollback would happen on next reboot -- corrupting
> data. Yep better docs is needed.
> 
> How about this one?

Applied.


	Pat