Patchwork Debugging facility improvements

login
register
about
Submitter Uwe Hermann
Date 2010-11-09 23:28:56
Message ID <20101109232856.GZ3256@greenwood>
Download mbox | patch
Permalink /patch/2286/
State Accepted
Headers show

Comments

Uwe Hermann - 2010-11-09 23:28:56
On Tue, Oct 26, 2010 at 07:19:59AM +0200, Peter Stuge wrote:
> > > Should this also force debug level to SPEW?
> > 
> > No idea, but it's unrelated to this patch.
> 
> I disagree.

We agree to disagree :)


> > As far as I can see all other such DEBUG_* mechanisms don't force
> > the debug level to SPEW either, so if we want to do that (I'm not
> > sure we do) that's material for another patch.
> 
> My point is that the patch claims to improve debugging, but in fact
> it adds the possibility that a user creates a configuration where
> they expect malloc debug messages but in fact do not get any because
> there is a glitch in Kconfig between the option they enabled and the
> requirement in code for SPEW loglevel for that option to have any
> effect. I don't think we should accept glitches like that, it is
> potentially a huge waste of time for the user, as well as extremely
> annoying. It makes coreboot look really unprofessional. :\

Yep. Updated patch as discussed on IRC. We only show the various DEBUG
options if at least DEBUG or SPEW was selected as loglevel in
menuconfig, as otherwise the additional debug code would not be printed
anyway.


Uwe.
Peter Stuge - 2010-11-09 23:35:40
Uwe Hermann wrote:
> Debugging facility improvements.
> 
>  - Hook up malloc() debug code via CONFIG_DEBUG_MALLOC. Only show it in
>    menuconfig if at least DEBUG or SPEW are selected as loglevel, as this
>    code does additional printk(BIOS_DEBUG, ...) calls which would otherwise
>    not be visible anyway.
> 
>  - Similarly, make DEBUG_CAR and REALMODE_DEBUG only visible if thr DEBUG or
>    SPEW loglevel is selected.
> 
>  - Get rid of a custom "debug" macro, use printk() as usual.
> 
> Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>

Acked-by: Peter Stuge <peter@stuge.se>
Uwe Hermann - 2010-11-10 00:15:05
On Wed, Nov 10, 2010 at 12:35:40AM +0100, Peter Stuge wrote:
> Acked-by: Peter Stuge <peter@stuge.se>

Thanks, r6054.


Uwe.

Patch

Debugging facility improvements.

 - Hook up malloc() debug code via CONFIG_DEBUG_MALLOC. Only show it in
   menuconfig if at least DEBUG or SPEW are selected as loglevel, as this
   code does additional printk(BIOS_DEBUG, ...) calls which would otherwise
   not be visible anyway.

 - Similarly, make DEBUG_CAR and REALMODE_DEBUG only visible if thr DEBUG or
   SPEW loglevel is selected.

 - Get rid of a custom "debug" macro, use printk() as usual.

Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>

Index: src/Kconfig
===================================================================
--- src/Kconfig	(Revision 6052)
+++ src/Kconfig	(Arbeitskopie)
@@ -502,10 +502,13 @@ 
 config HAVE_DEBUG_CAR
 	def_bool n
 
+# Only visible if debug level is DEBUG (7) or SPEW (8) as it does additional
+# printk(BIOS_DEBUG, ...) calls.
 config DEBUG_CAR
 	bool "Output verbose Cache-as-RAM debug messages"
 	default n
-	depends on HAVE_DEBUG_CAR
+	depends on HAVE_DEBUG_CAR && \
+		   (DEFAULT_CONSOLE_LOGLEVEL_7 || DEFAULT_CONSOLE_LOGLEVEL_8)
 	help
 	  This option enables additional CAR related debug messages.
 
@@ -553,10 +556,26 @@ 
 
 	  If unsure, say N.
 
+# Only visible if debug level is DEBUG (7) or SPEW (8) as it does additional
+# printk(BIOS_DEBUG, ...) calls.
+config DEBUG_MALLOC
+	bool "Output verbose malloc debug messages"
+	default n
+	depends on DEFAULT_CONSOLE_LOGLEVEL_7 || DEFAULT_CONSOLE_LOGLEVEL_8
+	help
+	  This option enables additional malloc related debug messages.
+
+	  Note: This option will increase the size of the coreboot image.
+
+	  If unsure, say N.
+
+# Only visible if debug level is DEBUG (7) or SPEW (8) as it does additional
+# printk(BIOS_DEBUG, ...) calls.
 config REALMODE_DEBUG
 	bool "Enable debug messages for option ROM execution"
 	default n
-	depends on PCI_OPTION_ROM_RUN_REALMODE
+	depends on PCI_OPTION_ROM_RUN_REALMODE && \
+		   (DEFAULT_CONSOLE_LOGLEVEL_7 || DEFAULT_CONSOLE_LOGLEVEL_8)
 	help
 	  This option enables additional x86emu related debug messages.
 
Index: src/lib/malloc.c
===================================================================
--- src/lib/malloc.c	(Revision 6052)
+++ src/lib/malloc.c	(Arbeitskopie)
@@ -1,11 +1,12 @@ 
 #include <stdlib.h>
 #include <console/console.h>
 
-#if 0
+#if CONFIG_DEBUG_MALLOC
+#define MALLOCDBG(x...) printk(BIOS_SPEW, x)
+#else
 #define MALLOCDBG(x)
-#else
-#define MALLOCDBG(x...) printk(BIOS_SPEW, x)
 #endif
+
 extern unsigned char _heap, _eheap;
 static void *free_mem_ptr = &_heap;		/* Start of heap */
 static void *free_mem_end_ptr = &_eheap;	/* End of heap */
Index: src/lib/cbmem.c
===================================================================
--- src/lib/cbmem.c	(Revision 6052)
+++ src/lib/cbmem.c	(Arbeitskopie)
@@ -22,12 +22,6 @@ 
 #include <cbmem.h>
 #include <console/console.h>
 
-#if 1
-#define debug(x...) printk(BIOS_DEBUG, x)
-#else
-#define debug(x...)
-#endif
-
 // The CBMEM TOC reserves 512 bytes to keep
 // the other entries somewhat aligned.
 // Increase if MAX_CBMEM_ENTRIES exceeds 21
@@ -68,10 +62,11 @@ 
 	bss_cbmem_toc = cbmem_toc;
 #endif
 
-	debug("Initializing CBMEM area to 0x%llx (%lld bytes)\n", baseaddr, size);
+	printk(BIOS_DEBUG, "Initializing CBMEM area to 0x%llx (%lld bytes)\n",
+	       baseaddr, size);
 
 	if (size < (64 * 1024)) {
-		debug("Increase CBMEM size!!\n");
+		printk(BIOS_DEBUG, "Increase CBMEM size!\n");
 		for (;;) ;
 	}
 
@@ -90,7 +85,9 @@ 
 	struct cbmem_entry *cbmem_toc;
 	cbmem_toc = (struct cbmem_entry *)(unsigned long)baseaddr;
 
-	debug("Re-Initializing CBMEM area to 0x%lx\n", (unsigned long)baseaddr);
+	printk(BIOS_DEBUG, "Re-Initializing CBMEM area to 0x%lx\n",
+	       (unsigned long)baseaddr);
+
 #ifndef __PRE_RAM__
 	bss_cbmem_toc = cbmem_toc;
 #endif
@@ -135,7 +132,7 @@ 
 		return NULL;
 	}
 
-	debug("Adding CBMEM entry as no. %d\n", i);
+	printk(BIOS_DEBUG, "Adding CBMEM entry as no. %d\n", i);
 
 	cbmem_toc[i] = (struct cbmem_entry) {
 		.magic = CBMEM_MAGIC,