Patchwork Debugging facility improvements

login
register
about
Submitter Uwe Hermann
Date 2010-10-12 00:03:40
Message ID <20101012000339.GW3256@greenwood>
Download mbox | patch
Permalink /patch/2097/
State Superseded
Headers show

Comments

Uwe Hermann - 2010-10-12 00:03:40
See patch.


Uwe.
Peter Stuge - 2010-10-12 07:46:47
Uwe Hermann wrote:
> Debugging facility improvements.
> 
>  - Hook up malloc() debug code via CONFIG_DEBUG_MALLOC.
> 
>  - 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 5940)
> +++ src/Kconfig	(Arbeitskopie)
> @@ -553,6 +553,16 @@
>  
>  	  If unsure, say N.
>  
> +config DEBUG_MALLOC
> +	bool "Output verbose malloc debug messages"
> +	default n
> +	help
> +	  This option enables additional malloc related debug messages.
> +
> +	  Note: This option will increase the size of the coreboot image.
> +
> +	  If unsure, say N.

Should this also force debug level to SPEW?


> +++ 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

So that messages actually always show up when the option is selected?


//Peter
Warren Turkal - 2010-10-13 06:00:13
Does the following part of the help text add any value:
"Note: This option will increase the size of the coreboot image."

I would think that it would be obvious that additional debug functionality 
makes the coreboot image bigger. Could you just strip that from the help text?

Also, posting patches in the body of your messages might make it easier for 
reviewers to comment on specific lines. Would it be possible for you to do so 
in the future for smaller patches like this one?

Thanks,
wt
Uwe Hermann - 2010-10-24 13:29:08
On Tue, Oct 12, 2010 at 09:46:47AM +0200, Peter Stuge wrote:
> Uwe Hermann wrote:
> > Debugging facility improvements.
> > 
> >  - Hook up malloc() debug code via CONFIG_DEBUG_MALLOC.
> > 
> >  - 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 5940)
> > +++ src/Kconfig	(Arbeitskopie)
> > @@ -553,6 +553,16 @@
> >  
> >  	  If unsure, say N.
> >  
> > +config DEBUG_MALLOC
> > +	bool "Output verbose malloc debug messages"
> > +	default n
> > +	help
> > +	  This option enables additional malloc related debug messages.
> > +
> > +	  Note: This option will increase the size of the coreboot image.
> > +
> > +	  If unsure, say N.
> 
> Should this also force debug level to SPEW?

No idea, but it's unrelated to this patch. 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.


Uwe.
Peter Stuge - 2010-10-26 05:19:59
Uwe Hermann wrote:
> > > Debugging facility improvements.
> > > 
> > >  - Hook up malloc() debug code via CONFIG_DEBUG_MALLOC.
..

> > > +++ src/Kconfig	(Arbeitskopie)
> > > @@ -553,6 +553,16 @@
> > >  
> > >  	  If unsure, say N.
> > >  
> > > +config DEBUG_MALLOC
> > > +	bool "Output verbose malloc debug messages"
> > > +	default n
> > > +	help
> > > +	  This option enables additional malloc related debug messages.
> > > +
> > > +	  Note: This option will increase the size of the coreboot image.
> > > +
> > > +	  If unsure, say N.
> > 
> > Should this also force debug level to SPEW?
> 
> No idea, but it's unrelated to this patch.

I 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. :\


//Peter

Patch

Debugging facility improvements.

 - Hook up malloc() debug code via CONFIG_DEBUG_MALLOC.

 - 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 5940)
+++ src/Kconfig	(Arbeitskopie)
@@ -553,6 +553,16 @@ 
 
 	  If unsure, say N.
 
+config DEBUG_MALLOC
+	bool "Output verbose malloc debug messages"
+	default n
+	help
+	  This option enables additional malloc related debug messages.
+
+	  Note: This option will increase the size of the coreboot image.
+
+	  If unsure, say N.
+
 config REALMODE_DEBUG
 	bool "Enable debug messages for option ROM execution"
 	default n
Index: src/lib/malloc.c
===================================================================
--- src/lib/malloc.c	(Revision 5940)
+++ 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 5940)
+++ 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,