Re: [PATCH RFC 1/4] Generic BUG handling.

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3943 bytes --]

On Thu, 2006-09-28 at 15:54 -0700, Jeremy Fitzhardinge wrote:
> plain text document attachment (generic-bug.patch)
> This patch adds common handling for kernel BUGs, for use by
> architectures as they wish.  The code is derived from arch/powerpc.
> 
> The advantages of having common BUG handling are:
>  - consistent BUG reporting across architectures
>  - shared implementation of out-of-line file/line data

Nice work.

> +       printk(KERN_EMERG "------------[ cut here ]------------\n");

I'm not sure I'm big on the cut here marker.

> i386 implements CONFIG_DEBUG_BUGVERBOSE, but x86-64 and powerpc do
> not.  This should probably be made more consistent.

It looks like if you do this you _might_ be able to share struct
bug_entry, or at least have consistent members for each arch. Which
would eliminate some of the inlines you have for accessing the bug
struct.

It needed a bit of work to get going on powerpc:

Generic BUG handling, Powerpc fixups

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

Index: to-merge/arch/powerpc/kernel/traps.c
===================================================================
--- to-merge.orig/arch/powerpc/kernel/traps.c
+++ to-merge/arch/powerpc/kernel/traps.c
@@ -731,32 +731,9 @@ static int emulate_instruction(struct pt
 	return -EINVAL;
 }
 
-/*
- * Look through the list of trap instructions that are used for BUG(),
- * BUG_ON() and WARN_ON() and see if we hit one.  At this point we know
- * that the exception was caused by a trap instruction of some kind.
- * Returns 1 if we should continue (i.e. it was a WARN_ON) or 0
- * otherwise.
- */
-extern struct bug_entry __start___bug_table[], __stop___bug_table[];
-
-#ifndef CONFIG_MODULES
-#define module_find_bug(x)	NULL
-#endif
-
-struct bug_entry *find_bug(unsigned long bugaddr)
-{
-	struct bug_entry *bug;
-
-	for (bug = __start___bug_table; bug < __stop___bug_table; ++bug)
-		if (bugaddr == bug->bug_addr)
-			return bug;
-	return module_find_bug(bugaddr);
-}
-
 int is_valid_bugaddr(unsigned long addr)
 {
-	return addr >= PAGE_OFFSET;
+	return is_kernel_addr(addr);
 }
 
 void __kprobes program_check_exception(struct pt_regs *regs)
Index: to-merge/include/asm-powerpc/bug.h
===================================================================
--- to-merge.orig/include/asm-powerpc/bug.h
+++ to-merge/include/asm-powerpc/bug.h
@@ -20,8 +20,6 @@ struct bug_entry {
 	const char	*function;
 };
 
-struct bug_entry *find_bug(unsigned long bugaddr);
-
 /*
  * If this bit is set in the line number it means that the trap
  * is for WARN_ON rather than BUG or BUG_ON.
Index: to-merge/include/linux/bug.h
===================================================================
--- to-merge.orig/include/linux/bug.h
+++ to-merge/include/linux/bug.h
@@ -6,14 +6,16 @@
 #ifdef CONFIG_GENERIC_BUG
 #include <linux/module.h>
 
-int report_bug(unsigned long bug_addr);
+extern int report_bug(unsigned long bug_addr);
 
-int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
+extern int  module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
 			 struct module *);
-void module_bug_cleanup(struct module *);
+extern void module_bug_cleanup(struct module *);
+
+extern const struct bug_entry *find_bug(unsigned long bugaddr);
 
 /* These are defined by the architecture */
-int is_valid_bugaddr(unsigned long addr);
+extern int is_valid_bugaddr(unsigned long addr);
 
 #endif	/* CONFIG_GENERIC_BUG */
 #endif	/* _LINUX_BUG_H */
Index: to-merge/lib/bug.c
===================================================================
--- to-merge.orig/lib/bug.c
+++ to-merge/lib/bug.c
@@ -21,7 +21,7 @@ static const struct bug_entry *module_fi
 	return NULL;
 }
 
-static const struct bug_entry *find_bug(unsigned long bugaddr)
+const struct bug_entry *find_bug(unsigned long bugaddr)
 {
 	const struct bug_entry *bug;
 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]