[PATCH] swsusp: misc cleanups [0/4]

[PATCH] swsusp: misc cleanups [0/4]

[Rafael J. Wysocki]

Hi,

You said you wanted something to  to try git on, so here you go. ;-)

The following series of patches contains some cleanups for swsusp.c.  IMO,
they are not very important, but at least the first two of them need to go
at some time.

In the order of decreasing importance:
1/4 - move the recalculation of nr_copy_pages so that the right number is used
	in enough_free_mem() and enough_swap()
2/4 - drop the unnecessary function does_collide_order()
3/4 - clean up whitespace
4/4 - make some comments and printk()s up to date

The first three patches are against 2.6.12-rc3 and they are mutually independent.
The last one is on top of the first three.

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] swsusp: misc cleanups [1/4]

[Rafael J. Wysocki]

The following patch moves the recalculation of nr_copy_pages so that the
right number is used in the calculation of the size of memory and swap needed.

It prevents swsusp from attempting to suspend if there is not enough memory
and/or swap (which is unlikely anyway).

Please consider for applying.


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nurp a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c	2005-04-22 14:48:04.000000000 +0200
+++ b/kernel/power/swsusp.c	2005-04-23 21:29:00.000000000 +0200
@@ -781,18 +781,18 @@ static int swsusp_alloc(void)
 {
 	int error;
 
+	pagedir_nosave = NULL;
+	nr_copy_pages = calc_nr(nr_copy_pages);
+
 	pr_debug("suspend: (pages needed: %d + %d free: %d)\n",
 		 nr_copy_pages, PAGES_FOR_IO, nr_free_pages());
 
-	pagedir_nosave = NULL;
 	if (!enough_free_mem())
 		return -ENOMEM;
 
 	if (!enough_swap())
 		return -ENOSPC;
 
-	nr_copy_pages = calc_nr(nr_copy_pages);
-
 	if (!(pagedir_save = alloc_pagedir(nr_copy_pages))) {
 		printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
 		return -ENOMEM;

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] swsusp: misc cleanups [1/4]

[Pavel Machek]

Hi!

> The following patch moves the recalculation of nr_copy_pages so that the
> right number is used in the calculation of the size of memory and swap needed.
> 
> It prevents swsusp from attempting to suspend if there is not enough memory
> and/or swap (which is unlikely anyway).
> 
> Please consider for applying.

Applied to git.

									Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [2/4]

[Rafael J. Wysocki]

The following patch removes the unnecessary functions does_collide_order().

This function is no longer necessary, as currently there are only 0-order
allocations in swsusp, and the use of it is confusing.

Please consider for applying.


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nurp a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c	2005-04-22 14:48:04.000000000 +0200
+++ b/kernel/power/swsusp.c	2005-04-23 21:39:57.000000000 +0200
@@ -929,21 +929,6 @@ int swsusp_resume(void)
 	return error;
 }
 
-/* More restore stuff */
-
-/*
- * Returns true if given address/order collides with any orig_address 
- */
-static int does_collide_order(unsigned long addr, int order)
-{
-	int i;
-	
-	for (i=0; i < (1<<order); i++)
-		if (!PageNosaveFree(virt_to_page(addr + i * PAGE_SIZE)))
-			return 1;
-	return 0;
-}
-
 /**
  *	On resume, for storing the PBE list and the image,
  *	we can only use memory pages that do not conflict with the pages
@@ -973,7 +958,7 @@ static unsigned long get_usable_page(uns
 	unsigned long m;
 
 	m = get_zeroed_page(gfp_mask);
-	while (does_collide_order(m, 0)) {
+	while (!PageNosaveFree(virt_to_page(m))) {
 		eat_page((void *)m);
 		m = get_zeroed_page(gfp_mask);
 		if (!m)
@@ -1061,7 +1046,7 @@ static struct pbe * swsusp_pagedir_reloc
 	/* Relocate colliding pages */
 
 	for_each_pb_page (pbpage, pblist) {
-		if (does_collide_order((unsigned long)pbpage, 0)) {
+		if (!PageNosaveFree(virt_to_page((unsigned long)pbpage))) {
 			m = (void *)get_usable_page(GFP_ATOMIC | __GFP_COLD);
 			if (!m) {
 				error = -ENOMEM;

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] swsusp: misc cleanups [2/4]

[Pavel Machek]

On So 23-04-05 23:29:18, Rafael J. Wysocki wrote:
> The following patch removes the unnecessary functions does_collide_order().
> 
> This function is no longer necessary, as currently there are only 0-order
> allocations in swsusp, and the use of it is confusing.
> 
> Please consider for applying.

Applied (to git).
									Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [3/4]

[Rafael J. Wysocki]

The following patch cleans up whitespace in swsusp.c (a bit):
- removes any trailing whitespace
- adds spaces after if, for, for_each_pbe, for_each_zone etc., wherever
necessary.

Please consider for applying.


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nurp a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c	2005-04-22 14:48:04.000000000 +0200
+++ b/kernel/power/swsusp.c	2005-04-23 21:48:19.000000000 +0200
@@ -10,12 +10,12 @@
  * This file is released under the GPLv2.
  *
  * I'd like to thank the following people for their work:
- * 
+ *
  * Pavel Machek <pavel@ucw.cz>:
  * Modifications, defectiveness pointing, being with me at the very beginning,
  * suspend to swap space, stop all tasks. Port to 2.4.18-ac and 2.5.17.
  *
- * Steve Doddi <dirk@loth.demon.co.uk>: 
+ * Steve Doddi <dirk@loth.demon.co.uk>:
  * Support the possibility of hardware state restoring.
  *
  * Raph <grey.havens@earthling.net>:
@@ -84,11 +84,11 @@ extern char resume_file[];
 unsigned int nr_copy_pages __nosavedata = 0;
 
 /* Suspend pagedir is allocated before final copy, therefore it
-   must be freed after resume 
+   must be freed after resume
 
    Warning: this is evil. There are actually two pagedirs at time of
    resume. One is "pagedir_save", which is empty frame allocated at
-   time of suspend, that must be freed. Second is "pagedir_nosave", 
+   time of suspend, that must be freed. Second is "pagedir_nosave",
    allocated at time of resume, that travels through memory not to
    collide with anything.
 
@@ -132,7 +132,7 @@ static int mark_swapfiles(swp_entry_t pr
 {
 	int error;
 
-	rw_swap_page_sync(READ, 
+	rw_swap_page_sync(READ,
 			  swp_entry(root_swap, 0),
 			  virt_to_page((unsigned long)&swsusp_header));
 	if (!memcmp("SWAP-SPACE",swsusp_header.sig, 10) ||
@@ -140,7 +140,7 @@ static int mark_swapfiles(swp_entry_t pr
 		memcpy(swsusp_header.orig_sig,swsusp_header.sig, 10);
 		memcpy(swsusp_header.sig,SWSUSP_SIG, 10);
 		swsusp_header.swsusp_info = prev;
-		error = rw_swap_page_sync(WRITE, 
+		error = rw_swap_page_sync(WRITE,
 					  swp_entry(root_swap, 0),
 					  virt_to_page((unsigned long)
 						       &swsusp_header));
@@ -174,22 +174,22 @@ static int is_resume_device(const struct
 static int swsusp_swap_check(void) /* This is called before saving image */
 {
 	int i, len;
-	
+
 	len=strlen(resume_file);
 	root_swap = 0xFFFF;
-	
+
 	swap_list_lock();
-	for(i=0; i<MAX_SWAPFILES; i++) {
+	for (i=0; i<MAX_SWAPFILES; i++) {
 		if (swap_info[i].flags == 0) {
 			swapfile_used[i]=SWAPFILE_UNUSED;
 		} else {
-			if(!len) {
+			if (!len) {
 	    			printk(KERN_WARNING "resume= option should be used to set suspend device" );
-				if(root_swap == 0xFFFF) {
+				if (root_swap == 0xFFFF) {
 					swapfile_used[i] = SWAPFILE_SUSPEND;
 					root_swap = i;
 				} else
-					swapfile_used[i] = SWAPFILE_IGNORED;				  
+					swapfile_used[i] = SWAPFILE_IGNORED;
 			} else {
 	  			/* we ignore all swap devices that are not the resume_file */
 				if (is_resume_device(&swap_info[i])) {
@@ -209,15 +209,15 @@ static int swsusp_swap_check(void) /* Th
  * This is called after saving image so modification
  * will be lost after resume... and that's what we want.
  * we make the device unusable. A new call to
- * lock_swapdevices can unlock the devices. 
+ * lock_swapdevices can unlock the devices.
  */
 static void lock_swapdevices(void)
 {
 	int i;
 
 	swap_list_lock();
-	for(i = 0; i< MAX_SWAPFILES; i++)
-		if(swapfile_used[i] == SWAPFILE_IGNORED) {
+	for (i = 0; i< MAX_SWAPFILES; i++)
+		if (swapfile_used[i] == SWAPFILE_IGNORED) {
 			swap_info[i].flags ^= 0xFF;
 		}
 	swap_list_unlock();
@@ -229,7 +229,7 @@ static void lock_swapdevices(void)
  *	@loc:	Place to store the entry we used.
  *
  *	Allocate a new swap entry and 'sync' it. Note we discard -EIO
- *	errors. That is an artifact left over from swsusp. It did not 
+ *	errors. That is an artifact left over from swsusp. It did not
  *	check the return of rw_swap_page_sync() at all, since most pages
  *	written back to swap would return -EIO.
  *	This is a partial improvement, since we will at least return other
@@ -241,7 +241,7 @@ static int write_page(unsigned long addr
 	int error = 0;
 
 	entry = get_swap_page();
-	if (swp_offset(entry) && 
+	if (swp_offset(entry) &&
 	    swapfile_used[swp_type(entry)] == SWAPFILE_SUSPEND) {
 		error = rw_swap_page_sync(WRITE, entry,
 					  virt_to_page(addr));
@@ -257,7 +257,7 @@ static int write_page(unsigned long addr
 /**
  *	data_free - Free the swap entries used by the saved image.
  *
- *	Walk the list of used swap entries and free each one. 
+ *	Walk the list of used swap entries and free each one.
  *	This is only used for cleanup when suspend fails.
  */
 static void data_free(void)
@@ -290,7 +290,7 @@ static int data_write(void)
 		mod = 1;
 
 	printk( "Writing data to swap (%d pages)...     ", nr_copy_pages );
-	for_each_pbe(p, pagedir_nosave) {
+	for_each_pbe (p, pagedir_nosave) {
 		if (!(i%mod))
 			printk( "\b\b\b\b%3d%%", i / mod );
 		if ((error = write_page(p->address, &(p->swap_address))))
@@ -335,7 +335,7 @@ static int close_swap(void)
 
 	dump_info();
 	error = write_page((unsigned long)&swsusp_info, &entry);
-	if (!error) { 
+	if (!error) {
 		printk( "S" );
 		error = mark_swapfiles(entry);
 		printk( "|\n" );
@@ -370,7 +370,7 @@ static int write_pagedir(void)
 	struct pbe * pbe;
 
 	printk( "Writing pagedir...");
-	for_each_pb_page(pbe, pagedir_nosave) {
+	for_each_pb_page (pbe, pagedir_nosave) {
 		if ((error = write_page((unsigned long)pbe, &swsusp_info.pagedir[n++])))
 			return error;
 	}
@@ -472,7 +472,7 @@ static int save_highmem(void)
 	int res = 0;
 
 	pr_debug("swsusp: Saving Highmem\n");
-	for_each_zone(zone) {
+	for_each_zone (zone) {
 		if (is_highmem(zone))
 			res = save_highmem_zone(zone);
 		if (res)
@@ -547,7 +547,7 @@ static void count_data_pages(void)
 
 	nr_copy_pages = 0;
 
-	for_each_zone(zone) {
+	for_each_zone (zone) {
 		if (is_highmem(zone))
 			continue;
 		mark_free_pages(zone);
@@ -562,9 +562,9 @@ static void copy_data_pages(void)
 	struct zone *zone;
 	unsigned long zone_pfn;
 	struct pbe * pbe = pagedir_nosave;
-	
+
 	pr_debug("copy_data_pages(): pages to copy: %d\n", nr_copy_pages);
-	for_each_zone(zone) {
+	for_each_zone (zone) {
 		if (is_highmem(zone))
 			continue;
 		mark_free_pages(zone);
@@ -702,7 +702,7 @@ static void free_image_pages(void)
 {
 	struct pbe * p;
 
-	for_each_pbe(p, pagedir_save) {
+	for_each_pbe (p, pagedir_save) {
 		if (p->address) {
 			ClearPageNosave(virt_to_page(p->address));
 			free_page(p->address);
@@ -719,7 +719,7 @@ static int alloc_image_pages(void)
 {
 	struct pbe * p;
 
-	for_each_pbe(p, pagedir_save) {
+	for_each_pbe (p, pagedir_save) {
 		p->address = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
 		if (!p->address)
 			return -ENOMEM;
@@ -740,7 +740,7 @@ void swsusp_free(void)
 /**
  *	enough_free_mem - Make sure we enough free memory to snapshot.
  *
- *	Returns TRUE or FALSE after checking the number of available 
+ *	Returns TRUE or FALSE after checking the number of available
  *	free pages.
  */
 
@@ -758,11 +758,11 @@ static int enough_free_mem(void)
 /**
  *	enough_swap - Make sure we have enough swap to save the image.
  *
- *	Returns TRUE or FALSE after checking the total amount of swap 
+ *	Returns TRUE or FALSE after checking the total amount of swap
  *	space avaiable.
  *
  *	FIXME: si_swapinfo(&i) returns all swap devices information.
- *	We should only consider resume_device. 
+ *	We should only consider resume_device.
  */
 
 static int enough_swap(void)
@@ -827,8 +827,8 @@ static int suspend_prepare_image(void)
 	error = swsusp_alloc();
 	if (error)
 		return error;
-	
-	/* During allocating of suspend pagedir, new cold pages may appear. 
+
+	/* During allocating of suspend pagedir, new cold pages may appear.
 	 * Kill them.
 	 */
 	drain_local_pages();
@@ -1045,7 +1045,7 @@ static struct pbe * swsusp_pagedir_reloc
 
 	/* Set page flags */
 
-	for_each_zone(zone) {
+	for_each_zone (zone) {
         	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
                 	SetPageNosaveFree(pfn_to_page(zone_pfn +
 					zone->zone_start_pfn));
@@ -1181,9 +1181,9 @@ static int bio_write_page(pgoff_t page_o
 static const char * sanity_check(void)
 {
 	dump_info();
-	if(swsusp_info.version_code != LINUX_VERSION_CODE)
+	if (swsusp_info.version_code != LINUX_VERSION_CODE)
 		return "kernel version";
-	if(swsusp_info.num_physpages != num_physpages)
+	if (swsusp_info.num_physpages != num_physpages)
 		return "memory size";
 	if (strcmp(swsusp_info.uts.sysname,system_utsname.sysname))
 		return "system type";
@@ -1193,7 +1193,7 @@ static const char * sanity_check(void)
 		return "version";
 	if (strcmp(swsusp_info.uts.machine,system_utsname.machine))
 		return "machine";
-	if(swsusp_info.cpus != num_online_cpus())
+	if (swsusp_info.cpus != num_online_cpus())
 		return "number of cpus";
 	return NULL;
 }
@@ -1230,7 +1230,7 @@ static int check_sig(void)
 		 * Reset swap signature now.
 		 */
 		error = bio_write_page(0, &swsusp_header);
-	} else { 
+	} else {
 		printk(KERN_ERR "swsusp: Suspend partition has wrong signature?\n");
 		return -EINVAL;
 	}

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] swsusp: misc cleanups [3/4]

[Pavel Machek]

Hi!

> The following patch cleans up whitespace in swsusp.c (a bit):
> - removes any trailing whitespace
> - adds spaces after if, for, for_each_pbe, for_each_zone etc., wherever
> necessary.
> 
> Please consider for applying.

Few hunks rejected, so I just applied those that work. This is the
result....

								Pavel

Index: kernel/power/swsusp.c
===================================================================
--- 32777ae73f5bb73dc108d3848153adb77b8a424a/kernel/power/swsusp.c  (mode:100644 sha1:cb4a01ed956eba4369ac1c21fa52901f13ac2cd5)
+++ uncommitted/kernel/power/swsusp.c  (mode:100644)
@@ -10,12 +10,12 @@
  * This file is released under the GPLv2.
  *
  * I'd like to thank the following people for their work:
- * 
+ *
  * Pavel Machek <pavel@ucw.cz>:
  * Modifications, defectiveness pointing, being with me at the very beginning,
  * suspend to swap space, stop all tasks. Port to 2.4.18-ac and 2.5.17.
  *
- * Steve Doddi <dirk@loth.demon.co.uk>: 
+ * Steve Doddi <dirk@loth.demon.co.uk>:
  * Support the possibility of hardware state restoring.
  *
  * Raph <grey.havens@earthling.net>:
@@ -97,11 +97,11 @@
 unsigned int nr_copy_pages __nosavedata = 0;
 
 /* Suspend pagedir is allocated before final copy, therefore it
-   must be freed after resume 
+   must be freed after resume
 
    Warning: this is evil. There are actually two pagedirs at time of
    resume. One is "pagedir_save", which is empty frame allocated at
-   time of suspend, that must be freed. Second is "pagedir_nosave", 
+   time of suspend, that must be freed. Second is "pagedir_nosave",
    allocated at time of resume, that travels through memory not to
    collide with anything.
 
@@ -206,7 +206,7 @@
 {
 	int error;
 
-	rw_swap_page_sync(READ, 
+	rw_swap_page_sync(READ,
 			  swp_entry(root_swap, 0),
 			  virt_to_page((unsigned long)&swsusp_header));
 	if (!memcmp("SWAP-SPACE",swsusp_header.sig, 10) ||
@@ -218,7 +218,7 @@
 		memcpy(swsusp_header.iv, iv, MAXIV);
 #endif
 		swsusp_header.swsusp_info = prev;
-		error = rw_swap_page_sync(WRITE, 
+		error = rw_swap_page_sync(WRITE,
 					  swp_entry(root_swap, 0),
 					  virt_to_page((unsigned long)
 						       &swsusp_header));
@@ -252,22 +252,22 @@
 static int swsusp_swap_check(void) /* This is called before saving image */
 {
 	int i, len;
-	
+
 	len=strlen(resume_file);
 	root_swap = 0xFFFF;
-	
+
 	swap_list_lock();
-	for(i=0; i<MAX_SWAPFILES; i++) {
+	for (i=0; i<MAX_SWAPFILES; i++) {
 		if (swap_info[i].flags == 0) {
 			swapfile_used[i]=SWAPFILE_UNUSED;
 		} else {
-			if(!len) {
+			if (!len) {
 	    			printk(KERN_WARNING "resume= option should be used to set suspend device" );
-				if(root_swap == 0xFFFF) {
+				if (root_swap == 0xFFFF) {
 					swapfile_used[i] = SWAPFILE_SUSPEND;
 					root_swap = i;
 				} else
-					swapfile_used[i] = SWAPFILE_IGNORED;				  
+					swapfile_used[i] = SWAPFILE_IGNORED;
 			} else {
 	  			/* we ignore all swap devices that are not the resume_file */
 				if (is_resume_device(&swap_info[i])) {
@@ -287,15 +287,15 @@
  * This is called after saving image so modification
  * will be lost after resume... and that's what we want.
  * we make the device unusable. A new call to
- * lock_swapdevices can unlock the devices. 
+ * lock_swapdevices can unlock the devices.
  */
 static void lock_swapdevices(void)
 {
 	int i;
 
 	swap_list_lock();
-	for(i = 0; i< MAX_SWAPFILES; i++)
-		if(swapfile_used[i] == SWAPFILE_IGNORED) {
+	for (i = 0; i< MAX_SWAPFILES; i++)
+		if (swapfile_used[i] == SWAPFILE_IGNORED) {
 			swap_info[i].flags ^= 0xFF;
 		}
 	swap_list_unlock();
@@ -307,7 +307,7 @@
  *	@loc:	Place to store the entry we used.
  *
  *	Allocate a new swap entry and 'sync' it. Note we discard -EIO
- *	errors. That is an artifact left over from swsusp. It did not 
+ *	errors. That is an artifact left over from swsusp. It did not
  *	check the return of rw_swap_page_sync() at all, since most pages
  *	written back to swap would return -EIO.
  *	This is a partial improvement, since we will at least return other
@@ -319,7 +319,7 @@
 	int error = 0;
 
 	entry = get_swap_page();
-	if (swp_offset(entry) && 
+	if (swp_offset(entry) &&
 	    swapfile_used[swp_type(entry)] == SWAPFILE_SUSPEND) {
 		error = rw_swap_page_sync(WRITE, entry,
 					  virt_to_page(addr));
@@ -335,7 +335,7 @@
 /**
  *	data_free - Free the swap entries used by the saved image.
  *
- *	Walk the list of used swap entries and free each one. 
+ *	Walk the list of used swap entries and free each one.
  *	This is only used for cleanup when suspend fails.
  */
 static void data_free(void)
@@ -381,7 +381,7 @@
 		mod = 1;
 
 	printk( "Writing data to swap (%d pages)...     ", nr_copy_pages );
-	for_each_pbe(p, pagedir_nosave) {
+	for_each_pbe (p, pagedir_nosave) {
 		if (!(i%mod))
 			printk( "\b\b\b\b%3d%%", i / mod );
 #ifdef CONFIG_SWSUSP_ENCRYPT
@@ -437,7 +437,7 @@
 
 	dump_info();
 	error = write_page((unsigned long)&swsusp_info, &entry);
-	if (!error) { 
+	if (!error) {
 		printk( "S" );
 		error = mark_swapfiles(entry);
 		printk( "|\n" );
@@ -472,7 +472,7 @@
 	struct pbe * pbe;
 
 	printk( "Writing pagedir...");
-	for_each_pb_page(pbe, pagedir_nosave) {
+	for_each_pb_page (pbe, pagedir_nosave) {
 		if ((error = write_page((unsigned long)pbe, &swsusp_info.pagedir[n++])))
 			return error;
 	}
@@ -578,7 +578,7 @@
 	int res = 0;
 
 	pr_debug("swsusp: Saving Highmem\n");
-	for_each_zone(zone) {
+	for_each_zone (zone) {
 		if (is_highmem(zone))
 			res = save_highmem_zone(zone);
 		if (res)
@@ -653,7 +653,7 @@
 
 	nr_copy_pages = 0;
 
-	for_each_zone(zone) {
+	for_each_zone (zone) {
 		if (is_highmem(zone))
 			continue;
 		mark_free_pages(zone);
@@ -668,9 +668,9 @@
 	struct zone *zone;
 	unsigned long zone_pfn;
 	struct pbe * pbe = pagedir_nosave;
-	
+
 	pr_debug("copy_data_pages(): pages to copy: %d\n", nr_copy_pages);
-	for_each_zone(zone) {
+	for_each_zone (zone) {
 		if (is_highmem(zone))
 			continue;
 		mark_free_pages(zone);
@@ -808,7 +808,7 @@
 {
 	struct pbe * p;
 
-	for_each_pbe(p, pagedir_save) {
+	for_each_pbe (p, pagedir_save) {
 		if (p->address) {
 			ClearPageNosave(virt_to_page(p->address));
 			free_page(p->address);
@@ -825,7 +825,7 @@
 {
 	struct pbe * p;
 
-	for_each_pbe(p, pagedir_save) {
+	for_each_pbe (p, pagedir_save) {
 		p->address = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
 		if (!p->address)
 			return -ENOMEM;
@@ -846,7 +846,7 @@
 /**
  *	enough_free_mem - Make sure we enough free memory to snapshot.
  *
- *	Returns TRUE or FALSE after checking the number of available 
+ *	Returns TRUE or FALSE after checking the number of available
  *	free pages.
  */
 
@@ -864,11 +864,11 @@
 /**
  *	enough_swap - Make sure we have enough swap to save the image.
  *
- *	Returns TRUE or FALSE after checking the total amount of swap 
+ *	Returns TRUE or FALSE after checking the total amount of swap
  *	space avaiable.
  *
  *	FIXME: si_swapinfo(&i) returns all swap devices information.
- *	We should only consider resume_device. 
+ *	We should only consider resume_device.
  */
 
 static int enough_swap(void)
@@ -933,8 +933,8 @@
 	error = swsusp_alloc();
 	if (error)
 		return error;
-	
-	/* During allocating of suspend pagedir, new cold pages may appear. 
+
+	/* During allocating of suspend pagedir, new cold pages may appear.
 	 * Kill them.
 	 */
 	drain_local_pages();
@@ -1150,7 +1150,7 @@
 
 	/* Set page flags */
 
-	for_each_zone(zone) {
+	for_each_zone (zone) {
         	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
                 	SetPageNosaveFree(pfn_to_page(zone_pfn +
 					zone->zone_start_pfn));



-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [3/4]

[Rafael J. Wysocki]

On Sunday, 24 of April 2005 00:18, Pavel Machek wrote:
> Hi!
> 
> > The following patch cleans up whitespace in swsusp.c (a bit):
> > - removes any trailing whitespace
> > - adds spaces after if, for, for_each_pbe, for_each_zone etc., wherever
> > necessary.
> > 
> > Please consider for applying.
> 
> Few hunks rejected, so I just applied those that work.

Sorry.  I did my best to be careful about this one, but without much success,
apparently.

> This is the result....

Fine,

Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] swsusp: misc cleanups [3/4]

[Pavel Machek]

On Ne 24-04-05 12:41:55, Rafael J. Wysocki wrote:
> On Sunday, 24 of April 2005 00:18, Pavel Machek wrote:
> > Hi!
> > 
> > > The following patch cleans up whitespace in swsusp.c (a bit):
> > > - removes any trailing whitespace
> > > - adds spaces after if, for, for_each_pbe, for_each_zone etc., wherever
> > > necessary.
> > > 
> > > Please consider for applying.
> > 
> > Few hunks rejected, so I just applied those that work.
> 
> Sorry.  I did my best to be careful about this one, but without much success,
> apparently.

Well, you had no chance, and it is not your fault. My tree is not
published anywhere, and I already have some "crypto swsusp" and
similar stuff in....
							Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [4/4]

[Rafael J. Wysocki]

The following patch makes some comments and printk()s in swsusp.c up to date.
In particular it adds the string "swsusp" before every message that is printed by
the code in swsusp.c.

Please consider for applying.


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nurp b/kernel/power/swsusp.c c/kernel/power/swsusp.c
--- b/kernel/power/swsusp.c	2005-04-23 22:41:57.000000000 +0200
+++ c/kernel/power/swsusp.c	2005-04-23 22:42:38.000000000 +0200
@@ -289,7 +289,7 @@ static int data_write(void)
 	if (!mod)
 		mod = 1;
 
-	printk( "Writing data to swap (%d pages)...     ", nr_copy_pages );
+	printk( "swsusp: Writing data to swap (%d pages)...     ", nr_copy_pages );
 	for_each_pbe (p, pagedir_nosave) {
 		if (!(i%mod))
 			printk( "\b\b\b\b%3d%%", i / mod );
@@ -369,7 +369,7 @@ static int write_pagedir(void)
 	unsigned n = 0;
 	struct pbe * pbe;
 
-	printk( "Writing pagedir...");
+	printk( "swsusp: Writing pagedir ... ");
 	for_each_pb_page (pbe, pagedir_nosave) {
 		if ((error = write_page((unsigned long)pbe, &swsusp_info.pagedir[n++])))
 			return error;
@@ -563,7 +563,7 @@ static void copy_data_pages(void)
 	unsigned long zone_pfn;
 	struct pbe * pbe = pagedir_nosave;
 
-	pr_debug("copy_data_pages(): pages to copy: %d\n", nr_copy_pages);
+	pr_debug("swsusp: pages to copy: %d\n", nr_copy_pages);
 	for_each_zone (zone) {
 		if (is_highmem(zone))
 			continue;
@@ -656,14 +656,11 @@ static void create_pbe_list(struct pbe *
 			p->next = p + 1;
 		p->next = NULL;
 	}
-	pr_debug("create_pbe_list(): initialized %d PBEs\n", num);
+	pr_debug("swsusp: initialized %d PBEs\n", num);
 }
 
 /**
- *	alloc_pagedir - Allocate the page directory.
- *
- *	First, determine exactly how many pages we need and
- *	allocate them.
+ *	alloc_pagedir - Allocate the list of page backup entries.
  *
  *	We arrange the pages in a chain: each page is an array of PBES_PER_PAGE
  *	struct pbe elements (pbes) and the last element in the page points
@@ -680,7 +677,6 @@ static struct pbe * alloc_pagedir(unsign
 	if (!nr_pages)
 		return NULL;
 
-	pr_debug("alloc_pagedir(): nr_pages = %d\n", nr_pages);
 	pblist = (struct pbe *)get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
 	for (pbe = pblist, num = PBES_PER_PAGE; pbe && num < nr_pages;
         		pbe = pbe->next, num += PBES_PER_PAGE) {
@@ -784,7 +780,7 @@ static int swsusp_alloc(void)
 	pagedir_nosave = NULL;
 	nr_copy_pages = calc_nr(nr_copy_pages);
 
-	pr_debug("suspend: (pages needed: %d + %d free: %d)\n",
+	pr_debug("swsusp: (pages needed: %d + %d free: %d)\n",
 		 nr_copy_pages, PAGES_FOR_IO, nr_free_pages());
 
 	if (!enough_free_mem())
@@ -794,13 +790,13 @@ static int swsusp_alloc(void)
 		return -ENOSPC;
 
 	if (!(pagedir_save = alloc_pagedir(nr_copy_pages))) {
-		printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
+		printk(KERN_ERR "swsusp: Allocating pagedir failed.\n");
 		return -ENOMEM;
 	}
 	create_pbe_list(pagedir_save, nr_copy_pages);
 	pagedir_nosave = pagedir_save;
 	if ((error = alloc_image_pages())) {
-		printk(KERN_ERR "suspend: Allocating image pages failed.\n");
+		printk(KERN_ERR "swsusp: Allocating image pages failed.\n");
 		swsusp_free();
 		return error;
 	}
@@ -815,7 +811,7 @@ static int suspend_prepare_image(void)
 
 	pr_debug("swsusp: critical section: \n");
 	if (save_highmem()) {
-		printk(KERN_CRIT "Suspend machine: Not enough free pages for highmem\n");
+		printk(KERN_CRIT "swsusp: Not enough free pages for highmem\n");
 		restore_highmem();
 		return -ENOMEM;
 	}
@@ -892,7 +888,7 @@ int swsusp_suspend(void)
 	 * at resume time, and evil weirdness ensues.
 	 */
 	if ((error = device_power_down(PMSG_FREEZE))) {
-		printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
+		printk(KERN_ERR "swsusp: Some devices failed to power down, aborting suspend\n");
 		local_irq_enable();
 		swsusp_free();
 		return error;
@@ -914,7 +910,7 @@ int swsusp_resume(void)
 	int error;
 	local_irq_disable();
 	if (device_power_down(PMSG_FREEZE))
-		printk(KERN_ERR "Some devices failed to power down, very bad\n");
+		printk(KERN_ERR "swsusp: Some devices failed to power down, very bad\n");
 	/* We'll ignore saved state, but this gets preempt count (etc) right */
 	save_processor_state();
 	error = swsusp_arch_resume();
@@ -1226,9 +1222,6 @@ static int check_sig(void)
 
 /**
  *	data_read - Read image pages from swap.
- *
- *	You do not need to check for overlaps, check_pagedir()
- *	already did that.
  */
 
 static int data_read(struct pbe *pblist)

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] swsusp: misc cleanups [4/4]

[Pavel Machek]

Hi!

> The following patch makes some comments and printk()s in swsusp.c up to date.
> In particular it adds the string "swsusp" before every message that is printed by
> the code in swsusp.c.

I don't like this one. Adding swsusp everywhere just clutters the
screen, most of it should be clear from context.

> @@ -1226,9 +1222,6 @@ static int check_sig(void)
>  
>  /**
>   *	data_read - Read image pages from swap.
> - *
> - *	You do not need to check for overlaps, check_pagedir()
> - *	already did that.
>   */
>  
>  static int data_read(struct pbe *pblist)

Why is this comment no longer valid?
								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [4/4]

[Rafael J. Wysocki]

Hi,

On Sunday, 24 of April 2005 00:07, Pavel Machek wrote:
> Hi!
> 
> > The following patch makes some comments and printk()s in swsusp.c up to date.
> > In particular it adds the string "swsusp" before every message that is printed by
> > the code in swsusp.c.
> 
> I don't like this one. Adding swsusp everywhere just clutters the
> screen, most of it should be clear from context.

Right.  Still, I'd like to drop function names from debug messages
and replace "suspend" with "swsusp" in some messages.  I'll send another
patch for it after 2.6.12 (please let me know if you don't think it's a good
idea ;-)).

For now, please drop the patch altogether.

> > @@ -1226,9 +1222,6 @@ static int check_sig(void)
> >  
> >  /**
> >   *	data_read - Read image pages from swap.
> > - *
> > - *	You do not need to check for overlaps, check_pagedir()
> > - *	already did that.
> >   */
> >  
> >  static int data_read(struct pbe *pblist)
> 
> Why is this comment no longer valid?

It's just confusing.  Initially, I didn't intend to change it, but then I read it
and thought "What overlaps?".  In data_read() there's nothing that could
overlap with anything else ...

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH] swsusp: misc cleanups [4/4]

[Pavel Machek]

Hi!

> > > The following patch makes some comments and printk()s in swsusp.c up to date.
> > > In particular it adds the string "swsusp" before every message that is printed by
> > > the code in swsusp.c.
> > 
> > I don't like this one. Adding swsusp everywhere just clutters the
> > screen, most of it should be clear from context.
> 
> Right.  Still, I'd like to drop function names from debug messages
> and replace "suspend" with "swsusp" in some messages.  I'll send another
> patch for it after 2.6.12 (please let me know if you don't think it's a good
> idea ;-)).

Well, I'll not say "no" just now ;-). I thought 'suspend' is nicer for
non-technical users, not being acronym.

> For now, please drop the patch altogether.

Done.

> > > @@ -1226,9 +1222,6 @@ static int check_sig(void)
> > >  
> > >  /**
> > >   *	data_read - Read image pages from swap.
> > > - *
> > > - *	You do not need to check for overlaps, check_pagedir()
> > > - *	already did that.
> > >   */
> > >  
> > >  static int data_read(struct pbe *pblist)
> > 
> > Why is this comment no longer valid?
> 
> It's just confusing.  Initially, I didn't intend to change it, but then I read it
> and thought "What overlaps?".  In data_read() there's nothing that could
> overlap with anything else ...

Well, if there were no checking in check_pagedir, we could end up in
situation where some page's address == some other page's original
address. That would be really bad. This comment says it can not happen
because care was taken in check_pagedir(). Perhaps it needs to be
explained better?

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [4/4]

[Stefan Seyfried]

Pavel Machek wrote:
> Hi!
> 
>> The following patch makes some comments and printk()s in swsusp.c up to date.
>> In particular it adds the string "swsusp" before every message that is printed by
>> the code in swsusp.c.
> 
> I don't like this one. Adding swsusp everywhere just clutters the
> screen, most of it should be clear from context.

I like it. The messages are short enough so we should not get line wraps
and it makes stuff more clear. You know, the context is not familiar to
everyone, just imagine the "why do we {suspend,resume} devices during
{resume,suspend} questions.

Also, i can ask for "send me output of dmesg|grep ^swsusp" to avoid
newbies flooding me with totally irrelevant info ;-)
-- 
Stefan Seyfried, QA / R&D Team Mobile Devices, SUSE LINUX Nürnberg.

"Any ideas, John?"
"Well, surrounding them's out."

Re: [PATCH] swsusp: misc cleanups [4/4]

[Pavel Machek]

Hi!

> >> The following patch makes some comments and printk()s in swsusp.c up to date.
> >> In particular it adds the string "swsusp" before every message that is printed by
> >> the code in swsusp.c.
> > 
> > I don't like this one. Adding swsusp everywhere just clutters the
> > screen, most of it should be clear from context.
> 
> I like it. The messages are short enough so we should not get line wraps
> and it makes stuff more clear. You know, the context is not familiar to
> everyone, just imagine the "why do we {suspend,resume} devices during
> {resume,suspend} questions.
> 
> Also, i can ask for "send me output of dmesg|grep ^swsusp" to avoid
> newbies flooding me with totally irrelevant info ;-)

That would not work, anyway. You want the messages from drivers,
too... and drivers are not going to prepend "swsusp".

Ultimately, cleaning up "suspend screen" so that it is not too scary
for non-technical users might be nice... It means killing some debug
messages from drivers, too.

					 			Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [4/4]

[Stefan Seyfried]

Pavel Machek wrote:

> That would not work, anyway. You want the messages from drivers,
> too... and drivers are not going to prepend "swsusp".

Yes it would. I do not need driver messages if the reason is "no swap
partition" or something like that ;-))

> Ultimately, cleaning up "suspend screen" so that it is not too scary
> for non-technical users might be nice... It means killing some debug
> messages from drivers, too.

I'd just sweep it under the bootsplash carpet. Then we have both:
possibility to debug and nice progressbar as long as everything works
fine :-)
-- 
Stefan Seyfried, QA / R&D Team Mobile Devices, SUSE LINUX Nürnberg.

"Any ideas, John?"
"Well, surrounding them's out."

Re: [PATCH] swsusp: misc cleanups [4/4]

[Pavel Machek]

Hi!

> > That would not work, anyway. You want the messages from drivers,
> > too... and drivers are not going to prepend "swsusp".
> 
> Yes it would. I do not need driver messages if the reason is "no swap
> partition" or something like that ;-))

Umm, okay. grep -i5 swsusp ... should get most driver messages, too...

> > Ultimately, cleaning up "suspend screen" so that it is not too scary
> > for non-technical users might be nice... It means killing some debug
> > messages from drivers, too.
> 
> I'd just sweep it under the bootsplash carpet. Then we have both:
> possibility to debug and nice progressbar as long as everything works
> fine :-)

I don't like bootsplash... plus I'd like to clean it properly. It
prints too much junk even for me...
								Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [0/4]

[Pavel Machek]

Hi!

> You said you wanted something to  to try git on, so here you go. ;-)

Oops... I did not want to test it *that* badly.

Okay, here's a deal: if I say "applied" in this context, it means
"applied to my git tree". If git breaks (or if I break it), they are
gone and I'll ask you to resend (or you'll notice and resend or
whatever...)

								Pavel
> The following series of patches contains some cleanups for swsusp.c.  IMO,
> they are not very important, but at least the first two of them need to go
> at some time.
> 
> In the order of decreasing importance:
> 1/4 - move the recalculation of nr_copy_pages so that the right number is used
> 	in enough_free_mem() and enough_swap()
> 2/4 - drop the unnecessary function does_collide_order()
> 3/4 - clean up whitespace
> 4/4 - make some comments and printk()s up to date
> 
> The first three patches are against 2.6.12-rc3 and they are mutually independent.
> The last one is on top of the first three.

-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH] swsusp: misc cleanups [0/4]

[Rafael J. Wysocki]

Hi,

On Saturday, 23 of April 2005 23:58, Pavel Machek wrote:
> Hi!
> 
> > You said you wanted something to  to try git on, so here you go. ;-)
> 
> Oops... I did not want to test it *that* badly.

Sorry.

> Okay, here's a deal: if I say "applied" in this context, it means
> "applied to my git tree". If git breaks (or if I break it), they are
> gone and I'll ask you to resend (or you'll notice and resend or
> whatever...)

OK

Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"