More cleanups for swsusp

PATCH Pavel Machek 5 participants 9 messages Jan 20, 2004 – Jan 22, 2004

Subsystems (2)

Suspend to RAM Hibernation

lore

Share & tools

lore

Pavel Machek

author

2004-01-20 22:53 UTC #0 lore raw

Hi!

This fixes codingstyle a bit, converts "can not happen" panic into BUG_ON (fill_suspend_header() allocates no memory so panic is meaningless) and adds check for sizeof (struct link) [if that is not PAGE_SIZE, we have *bad* problem, better check early]. Please apply,

Pavel

kernel/power/swsusp.c +9 -9

Index: linux/kernel/power/swsusp.c
===================================================================
@@ -340,14 +344,14 @@
 	printk("H");
 	BUG_ON (sizeof(struct suspend_header) > PAGE_SIZE-sizeof(swp_entry_t));
 	BUG_ON (sizeof(union diskpage) != PAGE_SIZE);
+	BUG_ON (sizeof(struct link) != PAGE_SIZE);
 	if (!(entry = get_swap_page()).val)
 		panic( "\nNot enough swapspace when writing header" );
 	if (swapfile_used[swp_type(entry)] != SWAPFILE_SUSPEND)
 		panic("\nNot enough swapspace for header on suspend device" );
 
 	cur = (void *) buffer;
-	if (fill_suspend_header(&cur->sh))
-		panic("\nOut of memory while writing header");
+	BUG_ON (fill_suspend_header(&cur->sh));
 		
 	cur->link.next = prev;

@@ -856,23 +837,23 @@
 
 static int sanity_check_failed(char *reason)
 {
-	printk(KERN_ERR "%s%s\n",name_resume,reason);
+	printk(KERN_ERR "%s%s\n", name_resume, reason);
 	return -EPERM;
 }
 
 static int sanity_check(struct suspend_header *sh)
 {
-	if(sh->version_code != LINUX_VERSION_CODE)
+	if (sh->version_code != LINUX_VERSION_CODE)
 		return sanity_check_failed("Incorrect kernel version");
-	if(sh->num_physpages != num_physpages)
+	if (sh->num_physpages != num_physpages)
 		return sanity_check_failed("Incorrect memory size");
-	if(strncmp(sh->machine, system_utsname.machine, 8))
+	if (strncmp(sh->machine, system_utsname.machine, 8))
 		return sanity_check_failed("Incorrect machine type");
-	if(strncmp(sh->version, system_utsname.version, 20))
+	if (strncmp(sh->version, system_utsname.version, 20))
 		return sanity_check_failed("Incorrect version");
-	if(sh->num_cpus != num_online_cpus())
+	if (sh->num_cpus != num_online_cpus())
 		return sanity_check_failed("Incorrect number of cpus");
-	if(sh->page_size != PAGE_SIZE)
+	if (sh->page_size != PAGE_SIZE)
 		return sanity_check_failed("Incorrect PAGE_SIZE");
 	return 0;
 }

Signature · 5 lines

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

Andrew Morton

2004-01-20 23:12 UTC #1 lore raw

kernel/power/swsusp.c line 347

Quoted 2 lines · Pavel Machek

kernel/power/swsusp.c +1 -0
+	BUG_ON (sizeof(struct link) != PAGE_SIZE);

Looking at the code, this hardly seems worth checking. But the compiler should just rub this code out anwyay, so whatever.

hmm, one could do:

#define compile_time_assert(expr)					\
	do {								\
		if (!(expr))						\
			compile_time_assert_failed();	/* undefined */	\
	} while (0)

Pavel Machek

2004-01-20 23:27 UTC #2 lore raw

Hi!

Quoted 5 lines

kernel/power/swsusp.c line 347
Quoted 1 line
kernel/power/swsusp.c +1 -0
+	BUG_ON (sizeof(struct link) != PAGE_SIZE);
Looking at the code, this hardly seems worth checking. But the compiler should just rub this code out anwyay, so whatever.

Quoted 7 lines

hmm, one could do:

#define compile_time_assert(expr)					\
	do {								\
		if (!(expr))						\
			compile_time_assert_failed();	/* undefined */	\
	} while (0)

Well, if you provide such macro, I'll be happy to use it ;-). It should be something like

compile_time_assert(expr, "message")

because with more of these are in the code, it would be nightmare to find out which one is wrong.

Perhaps better name, too?

pavel@elonex:/tmp$ cat delme.c

#define COMPILE_ERR_ON(expr, message) \
        do { if (!(expr)) compile_time_assert_failed_##message(); } while (0)

void main(void)

{
        COMPILE_ERR_ON(0==8, cpu_broke_zeros_neck);
        COMPILE_ERR_ON(0==0, compiler_went_crazy);
}
pavel@elonex:/tmp$ gcc delme.c -o delme
delme.c: In function `main':
delme.c:6: warning: return type of `main' is not `int'
/tmp/ccAx2alr.o(.text+0x11): In function `main':
: undefined reference to `compile_time_assert_failed_cpu_broke_zeros_neck'
collect2: ld returned 1 exit status
pavel@elonex:/tmp$

Pavel

Signature · 2 lines

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

Rusty Russell

2004-01-21 05:18 UTC #3 lore raw

In reply to Andrew Morton

In message <[email protected]> you write:

Quoted 10 lines

kernel/power/swsusp.c line 347
Quoted 2 lines · Pavel Machek
kernel/power/swsusp.c +1 -0
+	BUG_ON (sizeof(struct link) != PAGE_SIZE);
Looking at the code, this hardly seems worth checking. But the compiler should just rub this code out anwyay, so whatever.
hmm, one could do:
#define compile_time_assert(expr) \

Already there: BUILD_BUG & BUILD_BUG_ON.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Pavel Machek

author

2004-01-22 00:17 UTC #4 lore raw

Hi!

Quoted 10 lines

Quoted 8 lines
kernel/power/swsusp.c line 347
Quoted 1 line
kernel/power/swsusp.c +1 -0
+	BUG_ON (sizeof(struct link) != PAGE_SIZE);
Looking at the code, this hardly seems worth checking. But the compiler should just rub this code out anwyay, so whatever.
hmm, one could do:
#define compile_time_assert(expr) \
Already there: BUILD_BUG & BUILD_BUG_ON.

Unfortunately that one gives no message to the user (AFAICS), so if/when people use it extensively, it will get nasty.
Pavel

Signature · 2 lines

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

Rusty Russell

2004-01-21 05:18 UTC #5 lore raw

In reply to Pavel Machek

In message <[email protected]> you write:

kernel/power/swsusp.c line 354

Quoted 3 lines

kernel/power/swsusp.c +1 -2

-	if (fill_suspend_header(&cur->sh))
-		panic("\nOut of memory while writing header");
+	BUG_ON (fill_suspend_header(&cur->sh));

1) fill_suspend_header never fails, perhaps make it return void.

2) If fill_suspend_header could fail, you should indicate why it won't fail here, and

3) BUG_ON(complex condition expression) is much less clear than:

	if (complex condition expression)
		BUG();

Cheers,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Andrew Morton

2004-01-21 05:30 UTC #6 lore raw

Review

Quoted 11 lines · Rusty Russell

In message <[email protected]> you write:
kernel/power/swsusp.c line 354
Quoted 3 lines
kernel/power/swsusp.c +1 -2
-	if (fill_suspend_header(&cur->sh))
-		panic("\nOut of memory while writing header");
+	BUG_ON (fill_suspend_header(&cur->sh));
...
3) BUG_ON(complex condition expression) is much less clear than:
(quoted diff) +0 -0
 	if (complex condition expression)
 		BUG();

Worse. If some smarty goes and makes BUG_ON a no-op (for space reasons), it will break software suspend. We should ensure that the expression which is supplied to BUG_ON() never has side-effects for this reason.

I'll drop that chunk.

Matt Mackall

2004-01-21 18:40 UTC #7 lore raw

Quoted 7 lines · Andrew Morton

Quoted 6 lines · Rusty Russell
In message <[email protected]> you write:
Quoted 3 lines
kernel/power/swsusp.c +1 -2
-	if (fill_suspend_header(&cur->sh))
-		panic("\nOut of memory while writing header");
+	BUG_ON (fill_suspend_header(&cur->sh));

Quoted 5 lines

Review
Quoted 5 lines
...
3) BUG_ON(complex condition expression) is much less clear than:
(quoted diff) +0 -0
 	if (complex condition expression)
 		BUG();

Disagree. All BUG_ON() stuff should read like:

/* check that impossible stuff didn't happen, move along, nothing to see */ BUG_ON(...);

Which is fine and good until the condition is actually doing more than just sanity checking.

Quoted 3 lines

Worse. If some smarty goes and makes BUG_ON a no-op (for space reasons), it will break software suspend. We should ensure that the expression which is supplied to BUG_ON() never has side-effects for this reason.

While I generally agree that "assertions" shouldn't have side-effects, a slightly smarter smarty would make sure that BUG_ON evaluated its condition. I have this in -tiny:

+#ifndef CONFIG_BUG
+#define BUG()
+#define WARN_ON(condition) do { if (condition) ; } while(0) +#define BUG_ON(condition) do { if (condition) ; } while(0) +#define PAGE_BUG(page)
+#else

Signature · 1 line

Matt Mackall : http://www.selenic.com : Linux development and consulting

Rusty Russell

2004-01-22 04:16 UTC #8 lore raw

Quoted 9 lines · On Wed, 21 Jan 2004 12:39:38 -0600 Matt Mackall <[email protected]> wrote:

Quoted 4 lines
Review
Quoted 4 lines
3) BUG_ON(complex condition expression) is much less clear than:
(quoted diff) +0 -0
 	if (complex condition expression)
 		BUG();
Disagree. All BUG_ON() stuff should read like:
/* check that impossible stuff didn't happen, move along, nothing to see */ BUG_ON(...);

You can disagree all you like.

But Linus only allowed BUG_ON() because of the branch-prediction problem, preferring explicit "if (x) BUG()". I happen to agree with him, especially if x is a complex expression.

Rusty.

Signature · 2 lines

   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy