[PATCH] improve grub_mm_init_region() debug message

[PATCH] improve grub_mm_init_region() debug message

[Robert Millan]

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


This has bothered me for a while.  By changing %u to %p this debug
message becomes actually useful for checking offset problems.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

[-- Attachment #2: memdebug.diff --]
[-- Type: text/x-diff, Size: 597 bytes --]


	* kern/mm.c (grub_mm_init_region): Improve debug message.

diff -u -x configure -x config.h.in -ur grub2/kern/mm.c memdebug/kern/mm.c
--- grub2/kern/mm.c	2007-12-30 09:52:04.000000000 +0100
+++ memdebug/kern/mm.c	2008-01-15 18:07:14.000000000 +0100
@@ -143,7 +143,7 @@
   grub_mm_header_t h;
   grub_mm_region_t r, *p, q;
 
-  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n", addr, (unsigned int) size);
+  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n", addr, addr + size);
 
   /* If this region is too small, ignore it.  */
   if (size < GRUB_MM_ALIGN * 2)

Re: [PATCH] improve grub_mm_init_region() debug message

[Pavel Roskin]

On Tue, 2008-01-15 at 18:17 +0100, Robert Millan wrote:
> -  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n",
> addr, (unsigned int) size);
> +  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n",
> addr, addr + size);

Maybe addr+size-1 would be better?  Inclusive boundaries are more
intuitive.  Just run "cat /proc/iomem" to see what I mean.

-- 
Regards,
Pavel Roskin

Re: [PATCH] improve grub_mm_init_region() debug message

[Robert Millan]

On Tue, Jan 15, 2008 at 12:42:39PM -0500, Pavel Roskin wrote:
> 
> On Tue, 2008-01-15 at 18:17 +0100, Robert Millan wrote:
> > -  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n",
> > addr, (unsigned int) size);
> > +  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n",
> > addr, addr + size);
> 
> Maybe addr+size-1 would be better?  Inclusive boundaries are more
> intuitive.  Just run "cat /proc/iomem" to see what I mean.

Sounds better to me.

Does this change (with addr + size -1) sound fine to everyone else?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] improve grub_mm_init_region() debug message

[Robert Millan]

On Tue, Jan 15, 2008 at 07:00:55PM +0100, Robert Millan wrote:
> On Tue, Jan 15, 2008 at 12:42:39PM -0500, Pavel Roskin wrote:
> > 
> > On Tue, 2008-01-15 at 18:17 +0100, Robert Millan wrote:
> > > -  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n",
> > > addr, (unsigned int) size);
> > > +  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n",
> > > addr, addr + size);
> > 
> > Maybe addr+size-1 would be better?  Inclusive boundaries are more
> > intuitive.  Just run "cat /proc/iomem" to see what I mean.
> 
> Sounds better to me.
> 
> Does this change (with addr + size -1) sound fine to everyone else?

Uhm actually, since we already substract one on ieee1275 to workaround
firmware bugs, this might get confusing.

How about just "addr + size" ?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] improve grub_mm_init_region() debug message

[Pavel Roskin]

On Sat, 2008-01-19 at 22:50 +0100, Robert Millan wrote:
> On Tue, Jan 15, 2008 at 07:00:55PM +0100, Robert Millan wrote:
> > On Tue, Jan 15, 2008 at 12:42:39PM -0500, Pavel Roskin wrote:
> > > 
> > > On Tue, 2008-01-15 at 18:17 +0100, Robert Millan wrote:
> > > > -  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n",
> > > > addr, (unsigned int) size);
> > > > +  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n",
> > > > addr, addr + size);
> > > 
> > > Maybe addr+size-1 would be better?  Inclusive boundaries are more
> > > intuitive.  Just run "cat /proc/iomem" to see what I mean.
> > 
> > Sounds better to me.
> > 
> > Does this change (with addr + size -1) sound fine to everyone else?
> 
> Uhm actually, since we already substract one on ieee1275 to workaround
> firmware bugs, this might get confusing.

Oh, I didn't know that.

> How about just "addr + size" ?

Fine with me.

-- 
Regards,
Pavel Roskin

Re: [PATCH] improve grub_mm_init_region() debug message

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Tue, Jan 15, 2008 at 12:42:39PM -0500, Pavel Roskin wrote:
>> 
>> On Tue, 2008-01-15 at 18:17 +0100, Robert Millan wrote:
>> > -  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n",
>> > addr, (unsigned int) size);
>> > +  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n",
>> > addr, addr + size);
>> 
>> Maybe addr+size-1 would be better?  Inclusive boundaries are more
>> intuitive.  Just run "cat /proc/iomem" to see what I mean.
>
> Sounds better to me.
>
> Does this change (with addr + size -1) sound fine to everyone else?

Why not showing both the range and the size.  Isn't the size useful
sometimes?  I hate calculating such things by hand...

--
Marco

Re: [PATCH] improve grub_mm_init_region() debug message

[Robert Millan]

On Mon, Jan 21, 2008 at 06:02:20PM +0100, Marco Gerards wrote:
> Robert Millan <rmh@aybabtu.com> writes:
> 
> > On Tue, Jan 15, 2008 at 12:42:39PM -0500, Pavel Roskin wrote:
> >> 
> >> On Tue, 2008-01-15 at 18:17 +0100, Robert Millan wrote:
> >> > -  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n",
> >> > addr, (unsigned int) size);
> >> > +  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n",
> >> > addr, addr + size);
> >> 
> >> Maybe addr+size-1 would be better?  Inclusive boundaries are more
> >> intuitive.  Just run "cat /proc/iomem" to see what I mean.
> >
> > Sounds better to me.
> >
> > Does this change (with addr + size -1) sound fine to everyone else?
> 
> Why not showing both the range and the size.  Isn't the size useful
> sometimes?

When?  What you typicaly want to debug is whether your mem region overlaps
with something else, or so.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: [PATCH] improve grub_mm_init_region() debug message

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Mon, Jan 21, 2008 at 06:02:20PM +0100, Marco Gerards wrote:
>> Robert Millan <rmh@aybabtu.com> writes:
>> 
>> > On Tue, Jan 15, 2008 at 12:42:39PM -0500, Pavel Roskin wrote:
>> >> 
>> >> On Tue, 2008-01-15 at 18:17 +0100, Robert Millan wrote:
>> >> > -  grub_dprintf ("mem", "Using memory for heap: addr=%p, size=%u\n",
>> >> > addr, (unsigned int) size);
>> >> > +  grub_dprintf ("mem", "Using memory for heap: start=%p, end=%p\n",
>> >> > addr, addr + size);
>> >> 
>> >> Maybe addr+size-1 would be better?  Inclusive boundaries are more
>> >> intuitive.  Just run "cat /proc/iomem" to see what I mean.
>> >
>> > Sounds better to me.
>> >
>> > Does this change (with addr + size -1) sound fine to everyone else?
>> 
>> Why not showing both the range and the size.  Isn't the size useful
>> sometimes?
>
> When?  What you typicaly want to debug is whether your mem region overlaps
> with something else, or so.

Well, it is fine for me.  If I need the size, I will bring this up
again.  Feel free to commit the patch as suggested.

--
Marco

Re: [PATCH] improve grub_mm_init_region() debug message

[Robert Millan]

On Mon, Jan 21, 2008 at 06:42:42PM +0100, Marco Gerards wrote:
> 
> Well, it is fine for me.  If I need the size, I will bring this up
> again.  Feel free to commit the patch as suggested.

Ok then.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)