[PATCH] Fix broken debug output reserve_region_with_split()

[PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

That debug outpout in kernel/resource.c is busted on 32-bit
machines, fix it with appropriate casts.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

One day Yinghai will figure out that resource_size_t can be 32-bit
and thus as printk arguments must be cast to (unsigned long long)
explicitely when using %llx... hopefully, that day, Ingo will also
catch these before committing them as it's not the first one like
this :-)


 kernel/resource.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- linux-work.orig/kernel/resource.c	2008-10-14 11:48:31.000000000 +1100
+++ linux-work/kernel/resource.c	2008-10-14 11:48:50.000000000 +1100
@@ -550,8 +550,9 @@ static void __init __reserve_region_with
 
 	if (!res) {
 		printk(KERN_DEBUG "    __reserve_region_with_split: (%s) [%llx, %llx], res: (%s) [%llx, %llx]\n",
-			 conflict->name, conflict->start, conflict->end,
-			 name, start, end);
+		       conflict->name, (unsigned long long)conflict->start,
+		       (unsigned long long)conflict->end,
+		       name, (unsigned long long)start, (unsigned long long)end);
 
 		/* failed, split and try again */
 

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

Benjamin Herrenschmidt wrote:
> That debug outpout in kernel/resource.c is busted on 32-bit
> machines, fix it with appropriate casts.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> One day Yinghai will figure out that resource_size_t can be 32-bit
> and thus as printk arguments must be cast to (unsigned long long)
> explicitely when using %llx... hopefully, that day, Ingo will also
> catch these before committing them as it's not the first one like
> this :-)
> 

I really think Linus' solution (add a resource printf modifier, that can 
contain the whole format) is much better.

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Yinghai Lu]

On Mon, Oct 13, 2008 at 6:04 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> Benjamin Herrenschmidt wrote:
>>
>> That debug outpout in kernel/resource.c is busted on 32-bit
>> machines, fix it with appropriate casts.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> ---
>>
>> One day Yinghai will figure out that resource_size_t can be 32-bit
>> and thus as printk arguments must be cast to (unsigned long long)
>> explicitely when using %llx... hopefully, that day, Ingo will also
>> catch these before committing them as it's not the first one like
>> this :-)

we had patch to remove that two debug lines.

>>
>
> I really think Linus' solution (add a resource printf modifier, that can
> contain the whole format) is much better.

yes. some pci resource print out etc could use that too.
to get rid of the annoying casting.

BTW: can you just enforce resource_t to u64?

YH

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

Yinghai Lu wrote:
> 
> yes. some pci resource print out etc could use that too.
> to get rid of the annoying casting.
> 
> BTW: can you just enforce resource_t to u64?
> 

We definitely shouldn't.  It'd suck on embedded systems.

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: "Yinghai Lu" <yinghai@kernel.org>
Date: Mon, 13 Oct 2008 18:14:21 -0700

> On Mon, Oct 13, 2008 at 6:04 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> > Benjamin Herrenschmidt wrote:
> >>
> >> That debug outpout in kernel/resource.c is busted on 32-bit
> >> machines, fix it with appropriate casts.
> >>
> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> ---
> >>
> >> One day Yinghai will figure out that resource_size_t can be 32-bit
> >> and thus as printk arguments must be cast to (unsigned long long)
> >> explicitely when using %llx... hopefully, that day, Ingo will also
> >> catch these before committing them as it's not the first one like
> >> this :-)
> 
> we had patch to remove that two debug lines.
> 
> >>
> >
> > I really think Linus' solution (add a resource printf modifier, that can
> > contain the whole format) is much better.
> 
> yes. some pci resource print out etc could use that too.
> to get rid of the annoying casting.
> 
> BTW: can you just enforce resource_t to u64?

Casting to u64 won't work.  That can be either "unsigned long" or
"unsigned long long" depending upon the architecture, so you'd still
need to cast.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

> yes. some pci resource print out etc could use that too.
> to get rid of the annoying casting.
> 
> BTW: can you just enforce resource_t to u64?

Why the waste on embedded platforms that don't need it ?

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Mon, 2008-10-13 at 18:04 -0700, H. Peter Anvin wrote:
> Benjamin Herrenschmidt wrote:
> > That debug outpout in kernel/resource.c is busted on 32-bit
> > machines, fix it with appropriate casts.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > 
> > One day Yinghai will figure out that resource_size_t can be 32-bit
> > and thus as printk arguments must be cast to (unsigned long long)
> > explicitely when using %llx... hopefully, that day, Ingo will also
> > catch these before committing them as it's not the first one like
> > this :-)
> > 
> 
> I really think Linus' solution (add a resource printf modifier, that can 
> contain the whole format) is much better.

I agree. In fact I may even have a patch somewhere for that. 

It has to be a pointer tho, thus what do we want ? Something that takes
a pointer to a resource_size_t or to a whole struct resource ? In the
later case, do we want to print the whole flags too ?

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Tue, 14 Oct 2008 12:41:27 +1100

> It has to be a pointer tho, thus what do we want ? Something that takes
> a pointer to a resource_size_t or to a whole struct resource ? In the
> later case, do we want to print the whole flags too ?

Yes, that's sort of the ugly aspect of this.

Taking a resource would be cool, and you could use some post modifier
character to say which part to print.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Mon, 2008-10-13 at 18:45 -0700, David Miller wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Tue, 14 Oct 2008 12:41:27 +1100
> 
> > It has to be a pointer tho, thus what do we want ? Something that takes
> > a pointer to a resource_size_t or to a whole struct resource ? In the
> > later case, do we want to print the whole flags too ?
> 
> Yes, that's sort of the ugly aspect of this.
> 
> Taking a resource would be cool, and you could use some post modifier
> character to say which part to print

That's a good idea, there's quite a few areas where we don't want the
flags.. I'll see if I can cook up something after I've finished
preparing the powerpc tree for Linus to pull.

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

Benjamin Herrenschmidt wrote:
>>>
>> I really think Linus' solution (add a resource printf modifier, that can 
>> contain the whole format) is much better.
> 
> I agree. In fact I may even have a patch somewhere for that. 
> 
> It has to be a pointer tho, thus what do we want ? Something that takes
> a pointer to a resource_size_t or to a whole struct resource ? In the
> later case, do we want to print the whole flags too ?
> 

Linus already posted a patch.  He took a struct resource
and printed it as a range.  I pointed out that we should use the flags 
to determine formatting options, e.g. I/O space is only 16 bits on x86.

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

> Linus already posted a patch.  He took a struct resource
> and printed it as a range.  I pointed out that we should use the flags 
> to determine formatting options, e.g. I/O space is only 16 bits on x86.

And x86 only please :-)

Allright, let's dig it out. The reason I wasn't too sure about what to
do with resources was whether to have it also print the name and flags,
since there's quite a few areas where were really don't care.

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Mon, 13 Oct 2008 19:54:33 -0700

> I pointed out that we should use the flags to determine formatting
> options, e.g. I/O space is only 16 bits on x86.

It's 64-bit on sparc64 :-)

There's also the bit where we use a 32-bit resource_t on sparc32
but encode the top 4-bits of the 36 bit physical I/O address in
the resource flags member.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1023 bytes --]

On Mon, 13 Oct 2008, David Miller wrote:
> From: "H. Peter Anvin" <hpa@zytor.com>
> Date: Mon, 13 Oct 2008 19:54:33 -0700
> 
> > I pointed out that we should use the flags to determine formatting
> > options, e.g. I/O space is only 16 bits on x86.
> 
> It's 64-bit on sparc64 :-)
> 
> There's also the bit where we use a 32-bit resource_t on sparc32
> but encode the top 4-bits of the 36 bit physical I/O address in
> the resource flags member.

So wouldn't it be better to switch those sparc32 configs to a 64-bit
resource_t? I think all other platforms with 36-bit physical addresses went
that way.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Date: Thu, 16 Oct 2008 10:30:52 +0200 (CEST)

> On Mon, 13 Oct 2008, David Miller wrote:
> > From: "H. Peter Anvin" <hpa@zytor.com>
> > Date: Mon, 13 Oct 2008 19:54:33 -0700
> > 
> > > I pointed out that we should use the flags to determine formatting
> > > options, e.g. I/O space is only 16 bits on x86.
> > 
> > It's 64-bit on sparc64 :-)
> > 
> > There's also the bit where we use a 32-bit resource_t on sparc32
> > but encode the top 4-bits of the 36 bit physical I/O address in
> > the resource flags member.
> 
> So wouldn't it be better to switch those sparc32 configs to a 64-bit
> resource_t? I think all other platforms with 36-bit physical addresses went
> that way.

Absolutely, this is what should happen.

I'll try to make this happen in 2.6.29, it's a moderately invasive
change because it effects how ioremap() and friends are implemented.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

David Miller wrote:
> From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> Date: Thu, 16 Oct 2008 10:30:52 +0200 (CEST)
> 
>> On Mon, 13 Oct 2008, David Miller wrote:
>>> From: "H. Peter Anvin" <hpa@zytor.com>
>>> Date: Mon, 13 Oct 2008 19:54:33 -0700
>>>
>>>> I pointed out that we should use the flags to determine formatting
>>>> options, e.g. I/O space is only 16 bits on x86.
>>> It's 64-bit on sparc64 :-)
>>>
>>> There's also the bit where we use a 32-bit resource_t on sparc32
>>> but encode the top 4-bits of the 36 bit physical I/O address in
>>> the resource flags member.
>> So wouldn't it be better to switch those sparc32 configs to a 64-bit
>> resource_t? I think all other platforms with 36-bit physical addresses went
>> that way.
> 
> Absolutely, this is what should happen.
> 
> I'll try to make this happen in 2.6.29, it's a moderately invasive
> change because it effects how ioremap() and friends are implemented.

Either way, it totally underscores how desirable it is to centralize 
this particular class of formatting.

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

From: Linus Torvalds <torvalds@linux-foundation.org>

Implement %pR to print struct resource content

Add a %pR option to the kernel vsnprintf that prints the range of
addresses inside a struct resource passed by pointer.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Time that we sort that out once for all. I have patches for converting
some of PCI that I'll send later, and doing more as we speak.

I took Linus variant, added 0x and made it print lowercase. There's
still the question of whether we want to also make it pad with zeros
(the number of zeros being based on the size of resource_size_t,
maybe with hacks to limit IO size on x86 ?)

Let me know and I'll respin if necessary.

Index: linux-work/lib/vsprintf.c
===================================================================
--- linux-work.orig/lib/vsprintf.c	2008-10-17 13:30:18.000000000 +1100
+++ linux-work/lib/vsprintf.c	2008-10-17 13:55:25.000000000 +1100
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
+#include <linux/ioport.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -550,6 +551,22 @@ static char *symbol_string(char *buf, ch
 #endif
 }
 
+static char *resource_string(char *buf, char *end, struct resource *res, int field_width, int precision, int flags)
+{
+	/* room for the actual numbers, the two "0x", -, [, ] and the final zero */
+	char sym[4*sizeof(resource_size_t) + 8];
+	char *p = sym, *pend = sym + sizeof(sym);
+
+	*p++ = '[';
+	p = number(p, pend, res->start, 16, -1, -1, SPECIAL | SMALL);
+	*p++ = '-';
+	p = number(p, pend, res->end, 16, -1, -1, SPECIAL | SMALL);
+	*p++ = ']';
+	*p = 0;
+
+	return string(buf, end, sym, field_width, precision, flags);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -571,6 +588,8 @@ static char *pointer(const char *fmt, ch
 		/* Fallthrough */
 	case 'S':
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
+	case 'R':
+		return resource_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

From: Linus Torvalds <torvalds@linux-foundation.org>

Implement %pR to print struct resource content

Add a %pR option to the kernel vsnprintf that prints the range of
addresses inside a struct resource passed by pointer.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

And this one has updated comments.

Index: linux-work/lib/vsprintf.c
===================================================================
--- linux-work.orig/lib/vsprintf.c	2008-10-17 13:30:18.000000000 +1100
+++ linux-work/lib/vsprintf.c	2008-10-17 14:04:58.000000000 +1100
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
+#include <linux/ioport.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -550,18 +551,38 @@ static char *symbol_string(char *buf, ch
 #endif
 }
 
+static char *resource_string(char *buf, char *end, struct resource *res, int field_width, int precision, int flags)
+{
+	/* room for the actual numbers, the two "0x", -, [, ] and the final zero */
+	char sym[4*sizeof(resource_size_t) + 8];
+	char *p = sym, *pend = sym + sizeof(sym);
+
+	*p++ = '[';
+	p = number(p, pend, res->start, 16, -1, -1, SPECIAL | SMALL);
+	*p++ = '-';
+	p = number(p, pend, res->end, 16, -1, -1, SPECIAL | SMALL);
+	*p++ = ']';
+	*p = 0;
+
+	return string(buf, end, sym, field_width, precision, flags);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
  * specifiers.
  *
- * Right now we just handle 'F' (for symbolic Function descriptor pointers)
- * and 'S' (for Symbolic direct pointers), but this can easily be
- * extended in the future (network address types etc).
- *
- * The difference between 'S' and 'F' is that on ia64 and ppc64 function
- * pointers are really function descriptors, which contain a pointer the
- * real address. 
+ * Right now we handle:
+ *
+ * - 'F' For symbolic Function descriptor pointers
+ * - 'S' For Symbolic direct pointers), but this can easily be
+ *       extended in the future (network address types etc).
+ * - 'R' For a struct resouce pointer, it prints the range of
+ *       addresses (not the name nor the flags)
+ *
+ * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
+ * function pointers are really function descriptors, which contain a
+ * pointer the real address.
  */
 static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field_width, int precision, int flags)
 {
@@ -571,6 +592,8 @@ static char *pointer(const char *fmt, ch
 		/* Fallthrough */
 	case 'S':
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
+	case 'R':
+		return resource_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {
@@ -590,6 +613,7 @@ static char *pointer(const char *fmt, ch
  * This function follows C99 vsnprintf, but has some extensions:
  * %pS output the name of a text symbol
  * %pF output the name of a function pointer
+ * %pR output the address range in a struct resource
  *
  * The return value is the number of characters which would
  * be generated for the given input, excluding the trailing

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Roland Dreier]

 > + * Right now we handle:
 > + *
 > + * - 'F' For symbolic Function descriptor pointers
 > + * - 'S' For Symbolic direct pointers), but this can easily be
 > + *       extended in the future (network address types etc).
 > + * - 'R' For a struct resouce pointer, it prints the range of
 > + *       addresses (not the name nor the flags)

A little too much cut and paste -- the "easily be extended" bit at 'S'
doesn't really make sense in the middle of the list.

 > + * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
 > + * function pointers are really function descriptors, which contain a
 > + * pointer the real address.

you cut and pasted Linus's typo too -- "a pointer the real address"
should probably be "a pointer *TO* the real address"

 - R.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Thu, 2008-10-16 at 20:13 -0700, Roland Dreier wrote:
> > + * Right now we handle:
>  > + *
>  > + * - 'F' For symbolic Function descriptor pointers
>  > + * - 'S' For Symbolic direct pointers), but this can easily be
>  > + *       extended in the future (network address types etc).
>  > + * - 'R' For a struct resouce pointer, it prints the range of
>  > + *       addresses (not the name nor the flags)
> 
> A little too much cut and paste -- the "easily be extended" bit at 'S'
> doesn't really make sense in the middle of the list.

Right :-)

>  > + * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>  > + * function pointers are really function descriptors, which contain a
>  > + * pointer the real address.
> 
> you cut and pasted Linus's typo too -- "a pointer the real address"
> should probably be "a pointer *TO* the real address"

Ok, I'll wait for more comments before a respin to :-) I'm mostly
wondering whether to use fixed sized format rather than the current
variable size (and pad with zeros).

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Linus Torvalds]

On Fri, 17 Oct 2008, Benjamin Herrenschmidt wrote:
> 
> Ok, I'll wait for more comments before a respin to :-) I'm mostly
> wondering whether to use fixed sized format rather than the current
> variable size (and pad with zeros).

Sadly, I think that gcc warns if we were to support %08pR, so we can't 
pass down flags.

That said, I don't much like SPECIAL in the flags. We never do that thing. 
Doing a git grep for some range printouts, we tend to do things like

	[%lx,%lx]
	[%x - %x]
	[%llx, %llx]
	[%016lx - %016lx]
	<%016llx-%016llx>

But using "0x%llx" or "%#llx" is very rare. It happens (notably 
drivers/pci/setup-res.c), but it's not common.

But it would be kind of nice to be able to do extended flags. Maybe we 
could do it with

	%p04R

and the 04 would be parsed as flags, the same way we do %04x. And then if 
you want the 0x, you can use %p#08R..

			Linus

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 16 Oct 2008 20:49:00 -0700 (PDT)

> But it would be kind of nice to be able to do extended flags. Maybe we 
> could do it with
> 
> 	%p04R
> 
> and the 04 would be parsed as flags, the same way we do %04x. And then if 
> you want the 0x, you can use %p#08R..

These modifiers are interesting, but wouldn't it be even nicer to just
pick one output style and use it consistently in kernel messages for
resources?

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Linus Torvalds]

On Thu, 16 Oct 2008, David Miller wrote:
> 
> These modifiers are interesting, but wouldn't it be even nicer to just
> pick one output style and use it consistently in kernel messages for
> resources?

Sure, but I don't think we agree on what it would be. And quite frankly, 
it might depend on the resource.

For example, in traditional PCI, PIO resources would easily want to use 
%04x, which MMIO would use %08x. Sure, Linux _allows_ for bigger resources 
(ie you can have even PIO resources with the full 64-bit data), but it's 
not what you'd expect for any traditional stuff, and so it makes sense to 
make PIO resources show as %04x to get the old-fashioned resources shown 
in an expected manner.

We can do it inside the %pR code itself (just look at IORESOURCE_IO vs 
IORESOURCE_MEM), and maybe that's even the right approach. Maybe we want 
to even add flag bits, and show things like "IORESOURCE_PREFETCH" as a 
small marking automatically. But maybe people want to make it explicit.

I dunno. I'd certainly be perfectly happy with having the flags and 
field_width be specified by the resource flags. eg

	flags = ZEROPAD | SMALL;
	fieldwidth = (res->flags & IORESOURCE_IO) ? 4 : 8;

or something like that. But would it be acceptable to everybody?

			Linus

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Thu, 2008-10-16 at 21:18 -0700, Linus Torvalds wrote:
> 
> Sure, but I don't think we agree on what it would be. And quite frankly, 
> it might depend on the resource.
> 
> For example, in traditional PCI, PIO resources would easily want to use 
> %04x, which MMIO would use %08x. Sure, Linux _allows_ for bigger resources 
> (ie you can have even PIO resources with the full 64-bit data), but it's 
> not what you'd expect for any traditional stuff, and so it makes sense to 
> make PIO resources show as %04x to get the old-fashioned resources shown 
> in an expected manner.

Well, I was proposing that in the patch comments I think :-) We can do
some more specific formatting details in there based on the resource
flags, though we might also need some arch ifdef's in there.

It's better I think than having callers get it wrong in hundreds of
different ways.

> We can do it inside the %pR code itself (just look at IORESOURCE_IO vs 
> IORESOURCE_MEM), and maybe that's even the right approach. Maybe we want 
> to even add flag bits, and show things like "IORESOURCE_PREFETCH" as a 
> small marking automatically. But maybe people want to make it explicit.

I've been going on/off about the flags and decided to leave them alone
for now unless somebody comes up with a nice way to handle them. But
that doesn't mean we can't -use- the flags as you suggested to adjust
the format of the output.

The only little nit here is that the amount of digits needed for PIO is
fairly arch specific. So I might just stick an few ifdef's for the 4
possible cases (4, 8 and 16 digits) and let arch maintainers fix them up
(I'll default to 16 for everybody except x86 -> 4 and ppc -> 8).

As for the 36 bits trick of sparc32, I'll let DaveM do another arch hack
in there for now until he sorts it out.

I'll do a patch in a minute...

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Fri, 17 Oct 2008 16:00:27 +1100

> As for the 36 bits trick of sparc32, I'll let DaveM do another arch hack
> in there for now until he sorts it out.

Don't sweat sparc32, I'll convert it over to a 64-bit resource_size_t
eventually so for now just printing the 32-bit parts is not a disaster.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

From: Linus Torvalds <torvalds@linux-foundation.org>

Implement %pR to print struct resource content

Add a %pR option to the kernel vsnprintf that prints the range of
addresses inside a struct resource passed by pointer.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

This one does more fancy formatting.

Index: linux-work/lib/vsprintf.c
===================================================================
--- linux-work.orig/lib/vsprintf.c	2008-10-17 13:30:18.000000000 +1100
+++ linux-work/lib/vsprintf.c	2008-10-17 16:10:49.000000000 +1100
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
+#include <linux/ioport.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
@@ -550,18 +551,58 @@ static char *symbol_string(char *buf, ch
 #endif
 }
 
+static char *resource_string(char *buf, char *end, struct resource *res, int field_width, int precision, int flags)
+{
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+#define MEM_RSRC_SIZE	16
+#else
+#define MEM_RSRC_SIZE	8
+#endif
+
+#ifdef CONFIG_X86
+#define IO_RSRC_SIZE	4
+#elif CONFIG_PPC
+#define IO_RSRC_SIZE	8
+#else
+#define IO_RSRC_SIZE	MEM_RSRC_SIZE
+#endif
+
+	/* room for the actual numbers, the two "0x", -, [, ] and the final zero */
+	char sym[4*sizeof(resource_size_t) + 8];
+	char *p = sym, *pend = sym + sizeof(sym);
+	int size = 2 * sizeof(resource_size_t);
+
+	if (res->flags & IORESOURCE_MEM)
+		size = MEM_RSRC_SIZE;
+	else if (res->flags & IORESOURCE_IO)
+		size = IO_RSRC_SIZE;
+
+	*p++ = '[';
+	p = number(p, pend, res->start, 16, size, -1, SPECIAL | SMALL | ZEROPAD);
+	*p++ = '-';
+	p = number(p, pend, res->end, 16, size, -1, SPECIAL | SMALL | ZEROPAD);
+	*p++ = ']';
+	*p = 0;
+
+	return string(buf, end, sym, field_width, precision, flags);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
  * specifiers.
  *
- * Right now we just handle 'F' (for symbolic Function descriptor pointers)
- * and 'S' (for Symbolic direct pointers), but this can easily be
- * extended in the future (network address types etc).
- *
- * The difference between 'S' and 'F' is that on ia64 and ppc64 function
- * pointers are really function descriptors, which contain a pointer the
- * real address. 
+ * Right now we handle:
+ *
+ * - 'F' For symbolic Function descriptor pointers
+ * - 'S' For Symbolic direct pointers), but this can easily be
+ *       extended in the future (network address types etc).
+ * - 'R' For a struct resouce pointer, it prints the range of
+ *       addresses (not the name nor the flags)
+ *
+ * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
+ * function pointers are really function descriptors, which contain a
+ * pointer the real address.
  */
 static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field_width, int precision, int flags)
 {
@@ -571,6 +612,8 @@ static char *pointer(const char *fmt, ch
 		/* Fallthrough */
 	case 'S':
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
+	case 'R':
+		return resource_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {
@@ -590,6 +633,7 @@ static char *pointer(const char *fmt, ch
  * This function follows C99 vsnprintf, but has some extensions:
  * %pS output the name of a text symbol
  * %pF output the name of a function pointer
+ * %pR output the address range in a struct resource
  *
  * The return value is the number of characters which would
  * be generated for the given input, excluding the trailing

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Fri, 2008-10-17 at 16:12 +1100, Benjamin Herrenschmidt wrote:
> +#ifdef CONFIG_X86
> +#define IO_RSRC_SIZE   4
> +#elif CONFIG_PPC
> +#define IO_RSRC_SIZE   8
> +#else
> +#define IO_RSRC_SIZE   MEM_RSRC_SIZE
> +#endif

And I'm obviously tired, the #elif CONFIG_PPC is bogus, feel free to fix
it up, else I'll send a new patch tomorrow or next week. No big deal.

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Olivier Galibert]

On Fri, Oct 17, 2008 at 04:12:19PM +1100, Benjamin Herrenschmidt wrote:
> + * - 'R' For a struct resouce pointer, it prints the range of
> + *       addresses (not the name nor the flags)

You missed an 'r' here.

  OG.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Linus Torvalds]

On Fri, 17 Oct 2008, Benjamin Herrenschmidt wrote:
>
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> +#define MEM_RSRC_SIZE	16
> +#else
> +#define MEM_RSRC_SIZE	8
> +#endif
> +
> +#ifdef CONFIG_X86
> +#define IO_RSRC_SIZE	4
> +#elif CONFIG_PPC
> +#define IO_RSRC_SIZE	8
> +#else
> +#define IO_RSRC_SIZE	MEM_RSRC_SIZE
> +#endif

Why?

Do you think alpha wants to see its PIO resources shown as

	[00000000000001f0-00000000000001f7]

or

	[01f0-01f7]

and which do you think is more readable?

And even on sparc, with the full 64 bits in use, it's not like we'd be 
*truncating* to 4 hex digits with %04x.

		Linus

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> And even on sparc, with the full 64 bits in use, it's not like we'd be 
> *truncating* to 4 hex digits with %04x.
> 

This reminds me to add support for the ' modifier in the kernel printk...

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Linus Torvalds]

On Fri, 17 Oct 2008, H. Peter Anvin wrote:
> 
> This reminds me to add support for the ' modifier in the kernel printk...

So if you end up doing that... SUS seems to only specify it for decimal 
numbers, I wonder if it makes sense to extend it to hex too - but group by 
four. Octal? I have no clue, nor do I care.

		Linus

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

> Do you think alpha wants to see its PIO resources shown as
> 
> 	[00000000000001f0-00000000000001f7]
> 
> or
> 
> 	[01f0-01f7]
> 
> and which do you think is more readable?
> 
> And even on sparc, with the full 64 bits in use, it's not like we'd be 
> *truncating* to 4 hex digits with %04x.

I said archs could add themselves to the list who wants smaller
resources. I don't know which arch wants what here, I just provided
some defaults. Maybe we could move those define to asm/*.h tho.

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Linus Torvalds]

On Sat, 18 Oct 2008, Benjamin Herrenschmidt wrote:
> 
> I said archs could add themselves to the list who wants smaller
> resources.

But why even _bother_ with crazy resource widths like 16? 

Absolutely nobody wants those, even if they could use it?

So the default is all the wrong way around. Make the defaults the sane 
ones: 04x for IO, and 08x for MMIO, and then see if anybody wants anything 
else. I seriously doubt they do. The whole ".. but but sparc" argument 
seems to be entirely based on the total mis-conception of truncation that 
has no relevance.

		Linus

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Fri, 2008-10-17 at 14:11 -0700, Linus Torvalds wrote:

> So the default is all the wrong way around. Make the defaults the sane 
> ones: 04x for IO, and 08x for MMIO, and then see if anybody wants anything 
> else. I seriously doubt they do. The whole ".. but but sparc" argument 
> seems to be entirely based on the total mis-conception of truncation that 
> has no relevance.

Well, I definitely want 08x for IO on powerpc and I suspect anything
non-x86 or alpha does as well... So I'm fine but I'd like to keep
the default for IO at 08x unless you really have a strong opinion
against it.

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Sat, 2008-10-18 at 08:45 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2008-10-17 at 14:11 -0700, Linus Torvalds wrote:
> 
> > So the default is all the wrong way around. Make the defaults the sane 
> > ones: 04x for IO, and 08x for MMIO, and then see if anybody wants anything 
> > else. I seriously doubt they do. The whole ".. but but sparc" argument 
> > seems to be entirely based on the total mis-conception of truncation that 
> > has no relevance.
> 
> Well, I definitely want 08x for IO on powerpc and I suspect anything
> non-x86 or alpha does as well... So I'm fine but I'd like to keep
> the default for IO at 08x unless you really have a strong opinion
> against it.

Actually, screw it. 04x is fine, as you said, it's not going to truncate
anyway. The reason I wanted 08x on powerpc is that we have legitimate
negative-looking values in there due to the way we do PIO on 32-bit and
we have most PCI IO looking like it's >64K on 64-but but that's no big
deal, ie, we don't actually need the padding for IO space.

So I'm happy with just a padding of 8 for memory which is even good
looking for most 64-bit archs in fact, and 4 for IO. I left the ability
for archs to pick a different value if they want to tho.

I'll send the patch separately after a test or two with a better mail
subject.

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 16 Oct 2008 21:18:08 -0700 (PDT)

> I dunno. I'd certainly be perfectly happy with having the flags and 
> field_width be specified by the resource flags. eg
> 
> 	flags = ZEROPAD | SMALL;
> 	fieldwidth = (res->flags & IORESOURCE_IO) ? 4 : 8;
> 
> or something like that. But would it be acceptable to everybody?

I personally appreciate seeing the full 64-bit address on sparc64,
even for IORESOURCE_IO, and that's helped me debug resource
calculation bugs on more than one occasion in the past.

But that's just me :)

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Thu, 2008-10-16 at 22:21 -0700, David Miller wrote:
> 
> I personally appreciate seeing the full 64-bit address on sparc64,
> even for IORESOURCE_IO, and that's helped me debug resource
> calculation bugs on more than one occasion in the past.

The latest patch I posted will do full size of resource_size_t unless
you arch explicitely adds a #define to do it differently so that
should work for you :-)

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

Benjamin Herrenschmidt wrote:
> 
> The latest patch I posted will do full size of resource_size_t unless
> you arch explicitely adds a #define to do it differently so that
> should work for you :-)
> 

IMNSHO I think you can default to 8 digits for I/O for PCI, since at 
least last I looked there was no 64-bit address space possible for I/O.

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Thu, 16 Oct 2008 23:39:18 -0700

> Benjamin Herrenschmidt wrote:
> > The latest patch I posted will do full size of resource_size_t unless
> > you arch explicitely adds a #define to do it differently so that
> > should work for you :-)
> > 
> 
> IMNSHO I think you can default to 8 digits for I/O for PCI, since at least last I looked there was no 64-bit address space possible for I/O.

Resources are representing different things on different platforms.

On sparc64 it's actually the full 64-bit physical I/O address on the
system bus being stored in there.  So even IORESOURCE_IO objects have
64-bits of relevancy.

Like I said, printing out the entire thing has helped me find real
bugs, so printing the whole thing is not without merit.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

David Miller wrote:
>>>
>> IMNSHO I think you can default to 8 digits for I/O for PCI, since at least last I looked there was no 64-bit address space possible for I/O.
> 
> Resources are representing different things on different platforms.
> 
> On sparc64 it's actually the full 64-bit physical I/O address on the
> system bus being stored in there.  So even IORESOURCE_IO objects have
> 64-bits of relevancy.
> 

And then it should be printed as such on that platform.

> Like I said, printing out the entire thing has helped me find real
> bugs, so printing the whole thing is not without merit.

No argument there.  I was referring only to platforms where the resource 
contains a PCI I/O address, which is 32 bits long unless limited by 
additional constraints (like x86's 16-bit limit.)

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Linus Torvalds]

On Thu, 16 Oct 2008, David Miller wrote:
> 
> I personally appreciate seeing the full 64-bit address on sparc64,

Umm.

Why do you think %08x wouldn't do that?

The "8" part is the _minimal_ part. If there really are more digits than 
that, then you'll see them.

		Linus

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 17 Oct 2008 08:51:58 -0700 (PDT)

> Why do you think %08x wouldn't do that?
> 
> The "8" part is the _minimal_ part. If there really are more digits than 
> that, then you'll see them.

You probably mean "%08lx"  since plain "x" expects an "int" which is
32-bit.  At least that's my understanding.

But sure, that would work too.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

On Thu, 2008-10-16 at 20:49 -0700, Linus Torvalds wrote:
> 
> On Fri, 17 Oct 2008, Benjamin Herrenschmidt wrote:
> > 
> > Ok, I'll wait for more comments before a respin to :-) I'm mostly
> > wondering whether to use fixed sized format rather than the current
> > variable size (and pad with zeros).
> 
> Sadly, I think that gcc warns if we were to support %08pR, so we can't 
> pass down flags.
> 
> That said, I don't much like SPECIAL in the flags. We never do that thing. 
> Doing a git grep for some range printouts, we tend to do things like

I hesitated, but DaveM convinced me if you get a range of IO ports such
as 138-139, do you know on a first look that it's hex ? :-)

> and the 04 would be parsed as flags, the same way we do %04x. And then if 
> you want the 0x, you can use %p#08R..

Shouldn't we be consistent and have resources always be printed using
the same format ?

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

Benjamin Herrenschmidt wrote:
> 
> I hesitated, but DaveM convinced me if you get a range of IO ports such
> as 138-139, do you know on a first look that it's hex ? :-)
> 

Yes.  I mean, at least in the x86 world, we talk about "port eighty" for 
delays, which is really port 128 (0x80).

>> and the 04 would be parsed as flags, the same way we do %04x. And then if 
>> you want the 0x, you can use %p#08R..
> 
> Shouldn't we be consistent and have resources always be printed using
> the same format ?

Yes, please.

	-hpa

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Yinghai Lu]

On Thu, Oct 16, 2008 at 8:06 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
>
> Implement %pR to print struct resource content
>
> Add a %pR option to the kernel vsnprintf that prints the range of
> addresses inside a struct resource passed by pointer.
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> And this one has updated comments.
>
> Index: linux-work/lib/vsprintf.c
> ===================================================================
> --- linux-work.orig/lib/vsprintf.c      2008-10-17 13:30:18.000000000 +1100
> +++ linux-work/lib/vsprintf.c   2008-10-17 14:04:58.000000000 +1100
> @@ -24,6 +24,7 @@
>  #include <linux/kernel.h>
>  #include <linux/kallsyms.h>
>  #include <linux/uaccess.h>
> +#include <linux/ioport.h>
>
>  #include <asm/page.h>          /* for PAGE_SIZE */
>  #include <asm/div64.h>
> @@ -550,18 +551,38 @@ static char *symbol_string(char *buf, ch
>  #endif
>  }
>
> +static char *resource_string(char *buf, char *end, struct resource *res, int field_width, int precision, int flags)
> +{
> +       /* room for the actual numbers, the two "0x", -, [, ] and the final zero */
> +       char sym[4*sizeof(resource_size_t) + 8];
> +       char *p = sym, *pend = sym + sizeof(sym);
> +
> +       *p++ = '[';
> +       p = number(p, pend, res->start, 16, -1, -1, SPECIAL | SMALL);
> +       *p++ = '-';
> +       p = number(p, pend, res->end, 16, -1, -1, SPECIAL | SMALL);
> +       *p++ = ']';
> +       *p = 0;
> +
> +       return string(buf, end, sym, field_width, precision, flags);

so x64 64bit,io ports will be print out 8 digital?

YH

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[Benjamin Herrenschmidt]

> >
> > +static char *resource_string(char *buf, char *end, struct resource *res, int field_width, int precision, int flags)
> > +{
> > +       /* room for the actual numbers, the two "0x", -, [, ] and the final zero */
> > +       char sym[4*sizeof(resource_size_t) + 8];
> > +       char *p = sym, *pend = sym + sizeof(sym);
> > +
> > +       *p++ = '[';
> > +       p = number(p, pend, res->start, 16, -1, -1, SPECIAL | SMALL);
> > +       *p++ = '-';
> > +       p = number(p, pend, res->end, 16, -1, -1, SPECIAL | SMALL);
> > +       *p++ = ']';
> > +       *p = 0;
> > +
> > +       return string(buf, end, sym, field_width, precision, flags);
> 
> so x64 64bit,io ports will be print out 8 digital?

With that patch, it will use just as many digits as necessary to display
a given number. The question I'm asking in the comments is whether we
want to instead use fixed digits with zero padding, and in that case,
do we want a hook or something for archs to decide how many digits
for IO vs. memory.

But at least with that in, we can start converting callers. I have a
patch converting PCI that I'll post once that goes in and I'll start
scrubbing arch/powerpc soon.

Cheers,
Ben.

Re: [PATCH] Fix broken debug output reserve_region_with_split()

[H. Peter Anvin]

Benjamin Herrenschmidt wrote:
> 
> With that patch, it will use just as many digits as necessary to display
> a given number. The question I'm asking in the comments is whether we
> want to instead use fixed digits with zero padding, and in that case,
> do we want a hook or something for archs to decide how many digits
> for IO vs. memory.
> 

Yes, and yes.

We want to pad to keep people from mistaking, say, c000000 from 
c0000000.  At least on x86, it also acts as a visual cue for the type of 
resource space, as people are used to seeing I/O ports as 16-bit numbers 
with four hexdigits, and 8- or 16-hexdigit numbers for memory addresses.

	-hpa