Fix broken debug output reserve_region_with_split()

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

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

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 :-)

Quoted 4 lines · Benjamin Herrenschmidt

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

Quoted 8 lines

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

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 :-)

Quoted 1 line

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?

Quoted 5 lines

Quoted 4 lines · Benjamin Herrenschmidt

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

Quoted 8 lines

Quoted 8 lines

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

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.

Quoted 2 lines

Quoted 1 line

Quoted 2 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?

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?

Quoted 12 lines · 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 <[email protected]>

---

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.

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 ?

From: Benjamin Herrenschmidt <[email protected]>
Date: Tue, 14 Oct 2008 12:41:27 +1100

Quoted 3 lines

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

Quoted 2 lines

Quoted 1 line

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.

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

From: "H. Peter Anvin" <[email protected]>
Date: Mon, 13 Oct 2008 19:54:33 -0700

Quoted 2 lines

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.

Quoted 8 lines

From: "H. Peter Anvin" <[email protected]>
Date: Mon, 13 Oct 2008 19:54:33 -0700

Quoted 2 lines

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 :-)

Quoted 3 lines

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.

From: Geert Uytterhoeven <[email protected]>
Date: Thu, 16 Oct 2008 10:30:52 +0200 (CEST)

Quoted 11 lines · David Miller

Quoted 10 lines

From: "H. Peter Anvin" <[email protected]>
Date: Mon, 13 Oct 2008 19:54:33 -0700

Quoted 2 lines

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.

Quoted 3 lines

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.

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

Quoted 7 lines

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

Quoted 3 lines

+ * 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).

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?

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.

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.

+ * - 'R' For a struct resouce pointer, it prints the range of
+ * addresses (not the name nor the flags)

lib/vsprintf.c +3 -0

+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+#define MEM_RSRC_SIZE	16
+#else

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

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.

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.

Quoted 5 lines · Linus Torvalds

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.

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.

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 :-)

Quoted 4 lines · Benjamin Herrenschmidt

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.

Quoted 2 lines

Quoted 1 line

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.

I personally appreciate seeing the full 64-bit address on sparc64,

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.

Quoted 4 lines · Benjamin Herrenschmidt

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

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

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 ? :-)

Quoted 1 line

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 ?

From: Linus Torvalds <[email protected]>

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 <[email protected]>

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

And this one has updated comments.

Index: linux-work/lib/vsprintf.c
===================================================================

lib/vsprintf.c 2008-10-17 14:04:58.000000000 +1100 renamed from lib/vsprintf.c 2008-10-17 13:30:18.000000000 +1100 +5 -0

@@ -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];

Quoted 6 lines

lib/vsprintf.c +4 -0

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

lib/vsprintf.c static char *symbol_string(char *buf, ch · line 567

Quoted 10 lines

lib/vsprintf.c +10 -0

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