Skip to contentSkip to conversation

pci: Use new %pR to print resource ranges

PATCH In review Benjamin Herrenschmidt 4 participants 17 messages Oct 20, 2008 – Oct 21, 2008

Subsystems (5)
x86 PCI Library code x86 MM +1 more subsystems
lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 2/2 pci: Use new %pR to print resource ranges

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

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

---

I didn't go down the subdirs in there and I might have missed one or two but the bulk is converted.

 drivers/pci/pci.c       |    5 ++---
 drivers/pci/probe.c     |   28 +++++++++++++---------------
 drivers/pci/setup-bus.c |   13 ++++---------
 drivers/pci/setup-res.c |   40 +++++++++++++---------------------------
 4 files changed, 32 insertions(+), 54 deletions(-)
drivers/pci/probe.c +13 -15
@@ -238,9 +238,8 @@ static int __pci_read_base(struct pci_de
 		} else {
 			res->start = l64;
 			res->end = l64 + sz64;
-			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: [%llx, %llx]\n",
-				pci_name(dev), pos, (unsigned long long)res->start,
-				(unsigned long long)res->end);
+			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: %pR\n",
+				pci_name(dev), pos, res);
 		}
 	} else {
 		sz = pci_size(l, sz, mask);

@@ -250,9 +249,10 @@ static int __pci_read_base(struct pci_de
 
 		res->start = l;
 		res->end = l + sz;
-		printk(KERN_DEBUG "PCI: %s reg %x %s: [%llx, %llx]\n", pci_name(dev),
-			pos, (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
-			(unsigned long long)res->start, (unsigned long long)res->end);
+		printk(KERN_DEBUG "PCI: %s reg %x %s: %pR\n",
+		       pci_name(dev), pos,
+		       (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
+		       res);
 	}
 
  out:

@@ -323,9 +323,8 @@ void __devinit pci_read_bridge_bases(str
 			res->start = base;
 		if (!res->end)
 			res->end = limit + 0xfff;
-		printk(KERN_DEBUG "PCI: bridge %s io port: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s io port: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[1];

@@ -337,9 +336,8 @@ void __devinit pci_read_bridge_bases(str
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[2];

@@ -375,9 +373,9 @@ void __devinit pci_read_bridge_bases(str
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM | IORESOURCE_PREFETCH;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: [%llx, %llx]\n",
-			pci_name(dev), (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64" : "32",
-			(unsigned long long) res->start, (unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: %pR\n",
+		       pci_name(dev),
+		       (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64":"32", res);
 	}
 }
drivers/pci/setup-bus.c +4 -9
Index: linux-work/drivers/pci/setup-bus.c
===================================================================
@@ -356,10 +356,7 @@ static int pbus_size_mem(struct pci_bus 
 			order = __ffs(align) - 20;
 			if (order > 11) {
 				dev_warn(&dev->dev, "BAR %d bad alignment %llx: "
-				       "%#016llx-%#016llx\n", i,
-				       (unsigned long long)align,
-				       (unsigned long long)r->start,
-				       (unsigned long long)r->end);
+					 "%pR\n", i, (unsigned long long)align, r);
 				r->flags = 0;
 				continue;
 			}

@@ -539,11 +536,9 @@ static void pci_bus_dump_res(struct pci_
                 if (!res)
                         continue;
 
-		printk(KERN_INFO "bus: %02x index %x %s: [%llx, %llx]\n",
-			bus->number, i,
-			(res->flags & IORESOURCE_IO) ? "io port" : "mmio",
-			(unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_INFO "bus: %02x index %x %s: %pR\n",
+		       bus->number, i,
+		       (res->flags & IORESOURCE_IO) ? "io port" : "mmio", res);
         }
 }
drivers/pci/setup-res.c +13 -27
Index: linux-work/drivers/pci/setup-res.c
===================================================================
@@ -49,10 +49,8 @@ void pci_update_resource(struct pci_dev 
 
 	pcibios_resource_to_bus(dev, &region, res);
 
-	dev_dbg(&dev->dev, "BAR %d: got res [%#llx-%#llx] bus [%#llx-%#llx] "
-		"flags %#lx\n", resno,
-		 (unsigned long long)res->start,
-		 (unsigned long long)res->end,
+	dev_dbg(&dev->dev, "BAR %d: got res %pR bus [%#llx-%#llx] "
+		"flags %#lx\n", resno, res,
 		 (unsigned long long)region.start,
 		 (unsigned long long)region.end,
 		 (unsigned long)res->flags);

@@ -114,13 +112,11 @@ int pci_claim_resource(struct pci_dev *d
 		err = insert_resource(root, res);
 
 	if (err) {
-		dev_err(&dev->dev, "BAR %d: %s of %s [%#llx-%#llx]\n",
+		dev_err(&dev->dev, "BAR %d: %s of %s %pR\n",
 			resource,
 			root ? "address space collision on" :
 				"no parent found for",
-			dtype,
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+			dtype, res);
 	}
 
 	return err;

@@ -139,9 +135,8 @@ int pci_assign_resource(struct pci_dev *
 	align = resource_alignment(res);
 	if (!align) {
 		dev_err(&dev->dev, "BAR %d: can't allocate resource (bogus "
-			"alignment) [%#llx-%#llx] flags %#lx\n",
-			resno, (unsigned long long)res->start,
-			(unsigned long long)res->end, res->flags);
+			"alignment) %pR flags %#lx\n",
+			resno, res, res->flags);
 		return -EINVAL;
 	}
 

@@ -162,11 +157,8 @@ int pci_assign_resource(struct pci_dev *
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx]\n", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else {
 		res->flags &= ~IORESOURCE_STARTALIGN;
 		if (resno < PCI_BRIDGE_RESOURCES)

@@ -202,11 +194,8 @@ int pci_assign_resource_fixed(struct pci
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx\n]", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else if (resno < PCI_BRIDGE_RESOURCES) {
 		pci_update_resource(dev, res, resno);
 	}

@@ -237,9 +226,8 @@ void pdev_sort_resources(struct pci_dev 
 		r_align = resource_alignment(r);
 		if (!r_align) {
 			dev_warn(&dev->dev, "BAR %d: bogus alignment "
-				"[%#llx-%#llx] flags %#lx\n",
-				i, (unsigned long long)r->start,
-				(unsigned long long)r->end, r->flags);
+				"%pR flags %#lx\n",
+				i, r, r->flags);
 			continue;
 		}
 		for (list = head; ; list = list->next) {

@@ -287,9 +275,7 @@ int pci_enable_resources(struct pci_dev 
 
 		if (!r->parent) {
 			dev_err(&dev->dev, "device not available because of "
-				"BAR %d [%#llx-%#llx] collisions\n", i,
-				(unsigned long long) r->start,
-				(unsigned long long) r->end);
+				"BAR %d %pR collisions\n", i, r);
 			return -EINVAL;
 		}
drivers/pci/pci.c +2 -3
Index: linux-work/drivers/pci/pci.c
===================================================================
@@ -1358,11 +1358,10 @@ int pci_request_region(struct pci_dev *p
 	return 0;
 
 err_out:
-	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region [%#llx-%#llx]\n",
+	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region %pR\n",
 		 bar,
 		 pci_resource_flags(pdev, bar) & IORESOURCE_IO ? "I/O" : "mem",
-		 (unsigned long long)pci_resource_start(pdev, bar),
-		 (unsigned long long)pci_resource_end(pdev, bar));
+		 &pdev->resource[bar]);
 	return -EBUSY;
 }
#1 lore raw

PATCH 1/1 x86, ioremap: use %pR in printk

Quoted 5 lines · Benjamin Herrenschmidt

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

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

cool!

Acked-by Ingo Molnar <[email protected]>

there's also two places in arch/x86/mm/ioremap.c that could use this straight away - see the (untested) patch below.

Ingo

--------------------->

Quoted 1 line

From 3b4f8fd4ff54a2da035e972a610b8d5ac8d8eabb Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Mon, 20 Oct 2008 09:08:57 +0200
Subject: [PATCH] x86, ioremap: use %pR in printk

use the new %pR IO resource pointer/address/size printk type conversion specifier.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/mm/ioremap.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
arch/x86/mm/ioremap.c +5 -5
index ae71e11..640c653 100644
@@ -207,8 +207,8 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		return NULL;
 
 	if (!phys_addr_valid(phys_addr)) {
-		printk(KERN_WARNING "ioremap: invalid physical address %llx\n",
-		       (unsigned long long)phys_addr);
+		printk(KERN_WARNING "ioremap: invalid physical address %pR\n",
+		       phys_addr);
 		WARN_ON_ONCE(1);
 		return NULL;
 	}

@@ -267,9 +267,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		    (prot_val == _PAGE_CACHE_WC &&
 		     new_prot_val == _PAGE_CACHE_WB)) {
 			pr_debug(
-		"ioremap error for 0x%llx-0x%llx, requested 0x%lx, got 0x%lx\n",
-				(unsigned long long)phys_addr,
-				(unsigned long long)(phys_addr + size),
+		"ioremap error for %pR-%pR, requested 0x%lx, got 0x%lx\n",
+				phys_addr,
+				phys_addr + size,
 				prot_val, new_prot_val);
 			free_memtype(phys_addr, phys_addr + size);
 			return NULL;
author
#2 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 6 lines · Ingo Molnar
Quoted 4 lines · Benjamin Herrenschmidt

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

Quoted 8 lines
Quoted 1 line
Signed-off-by Benjamin Herrenschmidt <[email protected]>

cool!

Acked-by Ingo Molnar <[email protected]>

there's also two places in arch/x86/mm/ioremap.c that could use this straight away - see the (untested) patch below.

No, you don't pass it a phys_addr_t or a resource_size_t, you pass it a struct resource *

Now, I would have liked to have a way to print a resource_size_t (or phys_addr_t) but that's harder because we need pointers for the magic typechecking to work (among others).

Maybe we could create a special format for a pointer-to-resource_size_t and pass &foo on callers but that's fishy...

Cheers,
Ben.

#3 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 11 lines · Benjamin Herrenschmidt
Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Benjamin Herrenschmidt

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

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

cool!

Quoted 7 lines
Quoted 4 lines
Acked-by Ingo Molnar <[email protected]>

there's also two places in arch/x86/mm/ioremap.c that could use this straight away - see the (untested) patch below.

No, you don't pass it a phys_addr_t or a resource_size_t, you pass it a struct resource *

oops ... i only looked at:

+ char sym[4*sizeof(resource_size_t) + 8];

and assumed that it was a resource_size_t printout format :-/

while printing out ranges is useful too, it's by far not the only source of ugliness all around resource_size_t.

Quoted 7 lines

Now, I would have liked to have a way to print a resource_size_t (or phys_addr_t) but that's harder because we need pointers for the magic typechecking to work (among others).

Maybe we could create a special format for a pointer-to-resource_size_t and pass &foo on callers but that's fishy...

would be very nice to just have support for all the native types that the kernel uses. %pR is still a convenient shortcut.

Ingo

#4 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 11 lines · Ingo Molnar
Quoted 2 lines

No, you don't pass it a phys_addr_t or a resource_size_t, you pass it a struct resource *

oops ... i only looked at:

+ char sym[4*sizeof(resource_size_t) + 8];

and assumed that it was a resource_size_t printout format :-/

while printing out ranges is useful too, it's by far not the only source of ugliness all around resource_size_t.

so how about something like the two patches below (ontop of Linus's patch): the first patch introduces a "small" resource pointer printout
format: %pr - the little brother of %pR.

The output format is [0x00001234] - minimum width is 8.

The second patch takes advantage of it in ioremap.c.

Not tested yet but should work. One thing that would be nice is to extend it to phys_addr_t as well. Right now there's no guarantee that sizeof(resource_size_t) == sizeof(phys_addr_t).

What do you think? Passing it by reference makes it somewhat ugly [and restricts it to lvalues], but that's the safest we can get at the moment i guess, as it guarantees followup vararg parameter correctness, because gcc does not check these extensions.

Worst-case we get gibberish output if it's used incorrectly - but we dont get a kernel security hole if a %s follows it in the format string and an attacker can control some of the parameters, etc.

Ingo

---------------->

Quoted 1 line

From bec931cb306f0901b9e2017c845d5ad6016574e4 Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Mon, 20 Oct 2008 12:39:17 +0200
Subject: [PATCH] vsprintf: implement %pr to print resource_size_t

Signed-off-by Ingo Molnar <[email protected]>
---
 lib/vsprintf.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
lib/vsprintf.c +20 -0
index a013bbc..ddcaa6e 100644
@@ -581,6 +581,21 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
 	return string(buf, end, sym, field_width, precision, flags);
 }
 
+static char *resource_size_string(char *buf, char *end, resource_size_t *val, int field_width, int precision, int flags)
+{
+	/* room for the actual number, the one "0x", [, ] and the final zero */
+	char sym[2*sizeof(resource_size_t) + 5];
+	char *p = sym, *pend = sym + sizeof(sym);
+	int size = 8;
+
+	*p++ = '[';
+	p = number(p, pend, *val, 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

@@ -592,6 +607,8 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
  * - 'S' For symbolic direct pointers
  * - 'R' For a struct resource pointer, it prints the range of
  *       addresses (not the name nor the flags)
+ * - 'r' For a resource_size_t pointer, it prints the resource
+ *       addresses (in %pR format)
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a

@@ -607,6 +624,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
 	case 'R':
 		return resource_string(buf, end, ptr, field_width, precision, flags);
+	case 'r':
+		return resource_size_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {

@@ -627,6 +646,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
  * %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
+ * %pr output the address in a pointer to a resource_size_t type
  *
  * The return value is the number of characters which would
  * be generated for the given input, excluding the trailing
Quoted 1 line

From cfbd2eec241a53bc5c1b95bb68f269036e3e92a4 Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Mon, 20 Oct 2008 09:08:57 +0200
Subject: [PATCH] x86, ioremap: use %pr in printk

use the new %pr IO resource pointer/address/size printk type conversion specifier.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/mm/ioremap.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
arch/x86/mm/ioremap.c +5 -5
index ae71e11..4725c7a 100644
@@ -207,8 +207,8 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		return NULL;
 
 	if (!phys_addr_valid(phys_addr)) {
-		printk(KERN_WARNING "ioremap: invalid physical address %llx\n",
-		       (unsigned long long)phys_addr);
+		printk(KERN_WARNING "ioremap: invalid physical address %pr\n",
+		       &phys_addr);
 		WARN_ON_ONCE(1);
 		return NULL;
 	}

@@ -267,9 +267,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		    (prot_val == _PAGE_CACHE_WC &&
 		     new_prot_val == _PAGE_CACHE_WB)) {
 			pr_debug(
-		"ioremap error for 0x%llx-0x%llx, requested 0x%lx, got 0x%lx\n",
-				(unsigned long long)phys_addr,
-				(unsigned long long)(phys_addr + size),
+		"ioremap error for %pr [%lx], requested 0x%lx, got 0x%lx\n",
+				&phys_addr,
+				size,
 				prot_val, new_prot_val);
 			free_memtype(phys_addr, phys_addr + size);
 			return NULL;
author
#5 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 8 lines

so how about something like the two patches below (ontop of Linus's patch): the first patch introduces a "small" resource pointer printout
format: %pr - the little brother of %pR.

The output format is [0x00001234] - minimum width is 8.

The second patch takes advantage of it in ioremap.c.

Well, I did the exact same patch except I used the same function and just added a flag for "R" vs. "r". However, I didn't post it because I wasn't too happy with passing by pointer and I wasn't sure whether we wanted to keep the letter after p uppercase or not... In any case, I kept it as a thing to discuss after the first one goes in.

At this stage, I'm tempted to go for a %pP for printing a pointer to a phys_addr_t (and that's the same as resource_size_t, just more generic nowadays, since those were consolidated).

Still not too happy with the pointer thing but that's the best we can do I suppose without losing gcc type checking.

Cheers,
Ben.

author
#6 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 14 lines · Benjamin Herrenschmidt
Quoted 7 lines

so how about something like the two patches below (ontop of Linus's patch): the first patch introduces a "small" resource pointer printout
format: %pr - the little brother of %pR.

The output format is [0x00001234] - minimum width is 8.

The second patch takes advantage of it in ioremap.c.

Well, I did the exact same patch except I used the same function and just added a flag for "R" vs. "r". However, I didn't post it because I wasn't too happy with passing by pointer and I wasn't sure whether we wanted to keep the letter after p uppercase or not... In any case, I kept it as a thing to discuss after the first one goes in.

Quoted 6 lines

At this stage, I'm tempted to go for a %pP for printing a pointer to a phys_addr_t (and that's the same as resource_size_t, just more generic nowadays, since those were consolidated).

Still not too happy with the pointer thing but that's the best we can do I suppose without losing gcc type checking.

Oh, and I didn't like having the brackets around something that isn't a range too but that's a minor details.

Ben.

#7 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 5 lines · Benjamin Herrenschmidt
Quoted 2 lines

Still not too happy with the pointer thing but that's the best we can do I suppose without losing gcc type checking.

Oh, and I didn't like having the brackets around something that isn't a range too but that's a minor details.

it looked quite natural in printouts to me, but no strong feelings.

Ingo

#8 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 10 lines · Ingo Molnar
Quoted 5 lines · Benjamin Herrenschmidt
Quoted 2 lines

Still not too happy with the pointer thing but that's the best we can do I suppose without losing gcc type checking.

Oh, and I didn't like having the brackets around something that isn't a range too but that's a minor details.

it looked quite natural in printouts to me, but no strong feelings.

got rid of the brackets, see the patches below.

One open question would be whether to set the width to 8 on 32-bit platforms and 16 on 64-bit platforms - right now it's 8 on both. Since this is specifically a 'physical address' thing it might make sense to extend that on 64-bit systems. (although it's quite a bit of screen real estate so i think the current width of 8 should be fine)

Ingo

---------->
commit b74e806f605f2c3eda181066f60f2bf6585d835a

Author: Ingo Molnar <[email protected]>
Date:   Mon Oct 20 09:08:57 2008 +0200

x86, ioremap: use %pP in printk

use the new %pP physical address printk type conversion specifier.

Signed-off-by Ingo Molnar <[email protected]>
arch/x86/mm/ioremap.c +5 -5
index ae71e11..aaa81d9 100644
@@ -207,8 +207,8 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		return NULL;
 
 	if (!phys_addr_valid(phys_addr)) {
-		printk(KERN_WARNING "ioremap: invalid physical address %llx\n",
-		       (unsigned long long)phys_addr);
+		printk(KERN_WARNING "ioremap: invalid physical address %pP\n",
+		       &phys_addr);
 		WARN_ON_ONCE(1);
 		return NULL;
 	}

@@ -267,9 +267,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		    (prot_val == _PAGE_CACHE_WC &&
 		     new_prot_val == _PAGE_CACHE_WB)) {
 			pr_debug(
-		"ioremap error for 0x%llx-0x%llx, requested 0x%lx, got 0x%lx\n",
-				(unsigned long long)phys_addr,
-				(unsigned long long)(phys_addr + size),
+		"ioremap error for %pP [%lx], requested 0x%lx, got 0x%lx\n",
+				&phys_addr,
+				size,
 				prot_val, new_prot_val);
 			free_memtype(phys_addr, phys_addr + size);
 			return NULL;

commit 075279167da29f6bb701597a32e6b7311faa1782

Author: Ingo Molnar <[email protected]>
Date:   Mon Oct 20 12:39:17 2008 +0200

vsprintf: implement %pP to print phys_addr_t

Add %pP to print addresses in phys_addr_t variables. Passed by reference.

Signed-off-by Ingo Molnar <[email protected]>
lib/vsprintf.c +18 -0
index a013bbc..aac4fff 100644
@@ -581,6 +581,19 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
 	return string(buf, end, sym, field_width, precision, flags);
 }
 
+static char *phys_addr_string(char *buf, char *end, phys_addr_t *val, int field_width, int precision, int flags)
+{
+	/* room for the actual number, the "0x" and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 2];
+	char *p = sym, *pend = sym + sizeof(sym);
+	int size = 8;
+
+	p = number(p, pend, *val, 16, size, -1, SPECIAL | SMALL | ZEROPAD);
+	*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

@@ -592,6 +605,8 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
  * - 'S' For symbolic direct pointers
  * - 'R' For a struct resource pointer, it prints the range of
  *       addresses (not the name nor the flags)
+ * - 'P' For a phys_addr_t pointer, it prints the physical
+ *       addresses (with a minimum width of 8 characters)
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a

@@ -607,6 +622,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
 	case 'R':
 		return resource_string(buf, end, ptr, field_width, precision, flags);
+	case 'P':
+		return phys_addr_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {

@@ -627,6 +644,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
  * %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
+ * %pP output the address in a pointer to a phys_addr_t type
  *
  * The return value is the number of characters which would
  * be generated for the given input, excluding the trailing

commit 0d391458be88baf3c079e60c3ea331ebe12902c0

Author: Benjamin Herrenschmidt <[email protected]>
Date:   Mon Oct 20 15:07:37 2008 +1100

pci: use new %pR to print resource ranges

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
drivers/pci/pci.c +2 -3
index c9884bb..dbe9f39 100644
@@ -1358,11 +1358,10 @@ int pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
 	return 0;
 
 err_out:
-	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region [%#llx-%#llx]\n",
+	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region %pR\n",
 		 bar,
 		 pci_resource_flags(pdev, bar) & IORESOURCE_IO ? "I/O" : "mem",
-		 (unsigned long long)pci_resource_start(pdev, bar),
-		 (unsigned long long)pci_resource_end(pdev, bar));
+		 &pdev->resource[bar]);
 	return -EBUSY;
 }
drivers/pci/probe.c +13 -15
index dd9161a..d3db8b2 100644
@@ -304,9 +304,8 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		} else {
 			res->start = l64;
 			res->end = l64 + sz64;
-			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: [%llx, %llx]\n",
-				pci_name(dev), pos, (unsigned long long)res->start,
-				(unsigned long long)res->end);
+			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: %pR\n",
+				pci_name(dev), pos, res);
 		}
 	} else {
 		sz = pci_size(l, sz, mask);

@@ -316,9 +315,10 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 		res->start = l;
 		res->end = l + sz;
-		printk(KERN_DEBUG "PCI: %s reg %x %s: [%llx, %llx]\n", pci_name(dev),
-			pos, (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
-			(unsigned long long)res->start, (unsigned long long)res->end);
+		printk(KERN_DEBUG "PCI: %s reg %x %s: %pR\n",
+		       pci_name(dev), pos,
+		       (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
+		       res);
 	}
 
  out:

@@ -389,9 +389,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 			res->start = base;
 		if (!res->end)
 			res->end = limit + 0xfff;
-		printk(KERN_DEBUG "PCI: bridge %s io port: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s io port: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[1];

@@ -403,9 +402,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[2];

@@ -441,9 +439,9 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM | IORESOURCE_PREFETCH;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: [%llx, %llx]\n",
-			pci_name(dev), (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64" : "32",
-			(unsigned long long) res->start, (unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: %pR\n",
+		       pci_name(dev),
+		       (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64":"32", res);
 	}
 }
drivers/pci/setup-bus.c +4 -9
index d5e2106..471a429 100644
@@ -356,10 +356,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, unsigned long
 			order = __ffs(align) - 20;
 			if (order > 11) {
 				dev_warn(&dev->dev, "BAR %d bad alignment %llx: "
-				       "%#016llx-%#016llx\n", i,
-				       (unsigned long long)align,
-				       (unsigned long long)r->start,
-				       (unsigned long long)r->end);
+					 "%pR\n", i, (unsigned long long)align, r);
 				r->flags = 0;
 				continue;
 			}

@@ -539,11 +536,9 @@ static void pci_bus_dump_res(struct pci_bus *bus)
                 if (!res)
                         continue;
 
-		printk(KERN_INFO "bus: %02x index %x %s: [%llx, %llx]\n",
-			bus->number, i,
-			(res->flags & IORESOURCE_IO) ? "io port" : "mmio",
-			(unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_INFO "bus: %02x index %x %s: %pR\n",
+		       bus->number, i,
+		       (res->flags & IORESOURCE_IO) ? "io port" : "mmio", res);
         }
 }
drivers/pci/setup-res.c +13 -27
index 1a5fc83..d4b5c69 100644
@@ -49,10 +49,8 @@ void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
 
 	pcibios_resource_to_bus(dev, &region, res);
 
-	dev_dbg(&dev->dev, "BAR %d: got res [%#llx-%#llx] bus [%#llx-%#llx] "
-		"flags %#lx\n", resno,
-		 (unsigned long long)res->start,
-		 (unsigned long long)res->end,
+	dev_dbg(&dev->dev, "BAR %d: got res %pR bus [%#llx-%#llx] "
+		"flags %#lx\n", resno, res,
 		 (unsigned long long)region.start,
 		 (unsigned long long)region.end,
 		 (unsigned long)res->flags);

@@ -114,13 +112,11 @@ int pci_claim_resource(struct pci_dev *dev, int resource)
 		err = insert_resource(root, res);
 
 	if (err) {
-		dev_err(&dev->dev, "BAR %d: %s of %s [%#llx-%#llx]\n",
+		dev_err(&dev->dev, "BAR %d: %s of %s %pR\n",
 			resource,
 			root ? "address space collision on" :
 				"no parent found for",
-			dtype,
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+			dtype, res);
 	}
 
 	return err;

@@ -139,9 +135,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	align = resource_alignment(res);
 	if (!align) {
 		dev_err(&dev->dev, "BAR %d: can't allocate resource (bogus "
-			"alignment) [%#llx-%#llx] flags %#lx\n",
-			resno, (unsigned long long)res->start,
-			(unsigned long long)res->end, res->flags);
+			"alignment) %pR flags %#lx\n",
+			resno, res, res->flags);
 		return -EINVAL;
 	}
 

@@ -162,11 +157,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx]\n", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else {
 		res->flags &= ~IORESOURCE_STARTALIGN;
 		if (resno < PCI_BRIDGE_RESOURCES)

@@ -202,11 +194,8 @@ int pci_assign_resource_fixed(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx\n]", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else if (resno < PCI_BRIDGE_RESOURCES) {
 		pci_update_resource(dev, res, resno);
 	}

@@ -237,9 +226,8 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 		r_align = resource_alignment(r);
 		if (!r_align) {
 			dev_warn(&dev->dev, "BAR %d: bogus alignment "
-				"[%#llx-%#llx] flags %#lx\n",
-				i, (unsigned long long)r->start,
-				(unsigned long long)r->end, r->flags);
+				"%pR flags %#lx\n",
+				i, r, r->flags);
 			continue;
 		}
 		for (list = head; ; list = list->next) {

@@ -287,9 +275,7 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
 
 		if (!r->parent) {
 			dev_err(&dev->dev, "device not available because of "
-				"BAR %d [%#llx-%#llx] collisions\n", i,
-				(unsigned long long) r->start,
-				(unsigned long long) r->end);
+				"BAR %d %pR collisions\n", i, r);
 			return -EINVAL;
 		}

commit c21f132b1eca94808fe72b31aa159c7f0ad61d25

Author: Linus Torvalds <[email protected]>
Date:   Mon Oct 20 15:07:34 2008 +1100

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

Padding now defaults to 4 digits for IO and 8 for MEM

Signed-off-by Linus Torvalds <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
lib/vsprintf.c +43 -6

79 lines not on this page · raw message

#9 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

lib/vsprintf.c static char *resource_string(char *buf, char *end, struct resource *res, int fie · line 587

Quoted 2 lines · Ingo Molnar
lib/vsprintf.c +2 -0
+	/* room for the actual number, the "0x" and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 2];

Buffer overflow.

johannes

#10 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 6 lines · Johannes Berg

Review

Quoted 2 lines · Ingo Molnar
(quoted diff) +2 -0
+	/* room for the actual number, the "0x" and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 2];

Buffer overflow.

good spotting, that should be +3. And it's still untested ;-)

Ingo

-------------------->
commit e8613ba0d3912d526c9de2317f40ca2a236c026d

Author: Ingo Molnar <[email protected]>
Date:   Mon Oct 20 09:08:57 2008 +0200

x86, ioremap: use %pP in printk

use the new %pP physical address printk type conversion specifier.

Signed-off-by Ingo Molnar <[email protected]>
arch/x86/mm/ioremap.c +5 -5
index ae71e11..aaa81d9 100644
@@ -207,8 +207,8 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		return NULL;
 
 	if (!phys_addr_valid(phys_addr)) {
-		printk(KERN_WARNING "ioremap: invalid physical address %llx\n",
-		       (unsigned long long)phys_addr);
+		printk(KERN_WARNING "ioremap: invalid physical address %pP\n",
+		       &phys_addr);
 		WARN_ON_ONCE(1);
 		return NULL;
 	}

@@ -267,9 +267,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		    (prot_val == _PAGE_CACHE_WC &&
 		     new_prot_val == _PAGE_CACHE_WB)) {
 			pr_debug(
-		"ioremap error for 0x%llx-0x%llx, requested 0x%lx, got 0x%lx\n",
-				(unsigned long long)phys_addr,
-				(unsigned long long)(phys_addr + size),
+		"ioremap error for %pP [%lx], requested 0x%lx, got 0x%lx\n",
+				&phys_addr,
+				size,
 				prot_val, new_prot_val);
 			free_memtype(phys_addr, phys_addr + size);
 			return NULL;

commit 97cc157df4144c20ff2939f2acd4eaf96907ed55

Author: Ingo Molnar <[email protected]>
Date:   Mon Oct 20 12:39:17 2008 +0200

vsprintf: implement %pP to print phys_addr_t

Add %pP to print addresses in phys_addr_t variables. Passed by reference.

Signed-off-by Ingo Molnar <[email protected]>
lib/vsprintf.c +18 -0
index a013bbc..d0a3e2c 100644
@@ -581,6 +581,19 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
 	return string(buf, end, sym, field_width, precision, flags);
 }
 
+static char *phys_addr_string(char *buf, char *end, phys_addr_t *val, int field_width, int precision, int flags)
+{
+	/* room for the actual number, the "0x" and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 3];
+	char *p = sym, *pend = sym + sizeof(sym);
+	int size = 8;
+
+	p = number(p, pend, *val, 16, size, -1, SPECIAL | SMALL | ZEROPAD);
+	*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

@@ -592,6 +605,8 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
  * - 'S' For symbolic direct pointers
  * - 'R' For a struct resource pointer, it prints the range of
  *       addresses (not the name nor the flags)
+ * - 'P' For a phys_addr_t pointer, it prints the physical
+ *       addresses (with a minimum width of 8 characters)
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a

@@ -607,6 +622,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
 	case 'R':
 		return resource_string(buf, end, ptr, field_width, precision, flags);
+	case 'P':
+		return phys_addr_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {

@@ -627,6 +644,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
  * %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
+ * %pP output the address in a pointer to a phys_addr_t type
  *
  * The return value is the number of characters which would
  * be generated for the given input, excluding the trailing

commit 0d391458be88baf3c079e60c3ea331ebe12902c0

Author: Benjamin Herrenschmidt <[email protected]>
Date:   Mon Oct 20 15:07:37 2008 +1100

pci: use new %pR to print resource ranges

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
drivers/pci/pci.c +2 -3
index c9884bb..dbe9f39 100644
@@ -1358,11 +1358,10 @@ int pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
 	return 0;
 
 err_out:
-	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region [%#llx-%#llx]\n",
+	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region %pR\n",
 		 bar,
 		 pci_resource_flags(pdev, bar) & IORESOURCE_IO ? "I/O" : "mem",
-		 (unsigned long long)pci_resource_start(pdev, bar),
-		 (unsigned long long)pci_resource_end(pdev, bar));
+		 &pdev->resource[bar]);
 	return -EBUSY;
 }
drivers/pci/probe.c +13 -15
index dd9161a..d3db8b2 100644
@@ -304,9 +304,8 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		} else {
 			res->start = l64;
 			res->end = l64 + sz64;
-			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: [%llx, %llx]\n",
-				pci_name(dev), pos, (unsigned long long)res->start,
-				(unsigned long long)res->end);
+			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: %pR\n",
+				pci_name(dev), pos, res);
 		}
 	} else {
 		sz = pci_size(l, sz, mask);

@@ -316,9 +315,10 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 		res->start = l;
 		res->end = l + sz;
-		printk(KERN_DEBUG "PCI: %s reg %x %s: [%llx, %llx]\n", pci_name(dev),
-			pos, (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
-			(unsigned long long)res->start, (unsigned long long)res->end);
+		printk(KERN_DEBUG "PCI: %s reg %x %s: %pR\n",
+		       pci_name(dev), pos,
+		       (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
+		       res);
 	}
 
  out:

@@ -389,9 +389,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 			res->start = base;
 		if (!res->end)
 			res->end = limit + 0xfff;
-		printk(KERN_DEBUG "PCI: bridge %s io port: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s io port: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[1];

@@ -403,9 +402,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[2];

@@ -441,9 +439,9 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM | IORESOURCE_PREFETCH;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: [%llx, %llx]\n",
-			pci_name(dev), (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64" : "32",
-			(unsigned long long) res->start, (unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: %pR\n",
+		       pci_name(dev),
+		       (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64":"32", res);
 	}
 }
drivers/pci/setup-bus.c +4 -9
index d5e2106..471a429 100644
@@ -356,10 +356,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, unsigned long
 			order = __ffs(align) - 20;
 			if (order > 11) {
 				dev_warn(&dev->dev, "BAR %d bad alignment %llx: "
-				       "%#016llx-%#016llx\n", i,
-				       (unsigned long long)align,
-				       (unsigned long long)r->start,
-				       (unsigned long long)r->end);
+					 "%pR\n", i, (unsigned long long)align, r);
 				r->flags = 0;
 				continue;
 			}

@@ -539,11 +536,9 @@ static void pci_bus_dump_res(struct pci_bus *bus)
                 if (!res)
                         continue;
 
-		printk(KERN_INFO "bus: %02x index %x %s: [%llx, %llx]\n",
-			bus->number, i,
-			(res->flags & IORESOURCE_IO) ? "io port" : "mmio",
-			(unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_INFO "bus: %02x index %x %s: %pR\n",
+		       bus->number, i,
+		       (res->flags & IORESOURCE_IO) ? "io port" : "mmio", res);
         }
 }
drivers/pci/setup-res.c +13 -27
index 1a5fc83..d4b5c69 100644
@@ -49,10 +49,8 @@ void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
 
 	pcibios_resource_to_bus(dev, &region, res);
 
-	dev_dbg(&dev->dev, "BAR %d: got res [%#llx-%#llx] bus [%#llx-%#llx] "
-		"flags %#lx\n", resno,
-		 (unsigned long long)res->start,
-		 (unsigned long long)res->end,
+	dev_dbg(&dev->dev, "BAR %d: got res %pR bus [%#llx-%#llx] "
+		"flags %#lx\n", resno, res,
 		 (unsigned long long)region.start,
 		 (unsigned long long)region.end,
 		 (unsigned long)res->flags);

@@ -114,13 +112,11 @@ int pci_claim_resource(struct pci_dev *dev, int resource)
 		err = insert_resource(root, res);
 
 	if (err) {
-		dev_err(&dev->dev, "BAR %d: %s of %s [%#llx-%#llx]\n",
+		dev_err(&dev->dev, "BAR %d: %s of %s %pR\n",
 			resource,
 			root ? "address space collision on" :
 				"no parent found for",
-			dtype,
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+			dtype, res);
 	}
 
 	return err;

@@ -139,9 +135,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	align = resource_alignment(res);
 	if (!align) {
 		dev_err(&dev->dev, "BAR %d: can't allocate resource (bogus "
-			"alignment) [%#llx-%#llx] flags %#lx\n",
-			resno, (unsigned long long)res->start,
-			(unsigned long long)res->end, res->flags);
+			"alignment) %pR flags %#lx\n",
+			resno, res, res->flags);
 		return -EINVAL;
 	}
 

@@ -162,11 +157,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx]\n", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else {
 		res->flags &= ~IORESOURCE_STARTALIGN;
 		if (resno < PCI_BRIDGE_RESOURCES)

@@ -202,11 +194,8 @@ int pci_assign_resource_fixed(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx\n]", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else if (resno < PCI_BRIDGE_RESOURCES) {
 		pci_update_resource(dev, res, resno);
 	}

@@ -237,9 +226,8 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 		r_align = resource_alignment(r);
 		if (!r_align) {
 			dev_warn(&dev->dev, "BAR %d: bogus alignment "
-				"[%#llx-%#llx] flags %#lx\n",
-				i, (unsigned long long)r->start,
-				(unsigned long long)r->end, r->flags);
+				"%pR flags %#lx\n",
+				i, r, r->flags);
 			continue;
 		}
 		for (list = head; ; list = list->next) {

@@ -287,9 +275,7 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
 
 		if (!r->parent) {
 			dev_err(&dev->dev, "device not available because of "
-				"BAR %d [%#llx-%#llx] collisions\n", i,
-				(unsigned long long) r->start,
-				(unsigned long long) r->end);
+				"BAR %d %pR collisions\n", i, r);
 			return -EINVAL;
 		}

commit c21f132b1eca94808fe72b31aa159c7f0ad61d25

Author: Linus Torvalds <[email protected]>
Date:   Mon Oct 20 15:07:34 2008 +1100

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

Padding now defaults to 4 digits for IO and 8 for MEM

Signed-off-by Linus Torvalds <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
lib/vsprintf.c +43 -6

79 lines not on this page · raw message

author
#11 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

In reply to Johannes Berg
Quoted 6 lines · Johannes Berg

lib/vsprintf.c static char *resource_string(char *buf, char *end, struct resource *res, int fie · line 587

Quoted 2 lines · Ingo Molnar
lib/vsprintf.c +2 -0
+	/* room for the actual number, the "0x" and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 2];

Buffer overflow.

It's actually not going to overflow as we pass the end of the buffer to "number" but we'll miss a digit.

Ben.

#12 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

Quoted 10 lines · Benjamin Herrenschmidt
Quoted 6 lines · Johannes Berg

lib/vsprintf.c static char *resource_string(char *buf, char *end, struct resource *res, int fie · line 587

Quoted 2 lines · Ingo Molnar
lib/vsprintf.c +2 -0
+	/* room for the actual number, the "0x" and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 2];

Buffer overflow.

It's actually not going to overflow as we pass the end of the buffer to "number" but we'll miss a digit.

"buf" won't overflow, but "sym" will, no? Anyway, +3 is the correct thing to do.

johannes

#13 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

In reply to Ingo Molnar
Quoted 5 lines · Ingo Molnar

One open question would be whether to set the width to 8 on 32-bit platforms and 16 on 64-bit platforms - right now it's 8 on both. Since this is specifically a 'physical address' thing it might make sense to extend that on 64-bit systems. (although it's quite a bit of screen real estate so i think the current width of 8 should be fine)

Maybe we should let architectures configure it -- after all, they know the real size of a physical address. I'm thinking something like this:

#ifndef PHYS_ADDR_T_SIZE
#ifdef CONFIG_64BIT
#define PHYS_ADDR_T_SIZE 64
#else
#define PHYS_ADDR_T_SIZE 32
#endif
#endif

static char *phys_addr_string(char *buf, char *end, phys_addr_t *val, int field_width, int precision, int flags)

{
	/* room for the actual number, the "0x" and the final zero */
	char sym[2*sizeof(phys_addr_t) + 3];
	char *p = sym, *pend = sym + sizeof(sym);
	int size = DIV_ROUND_UP(PHYS_ADDR_T_SIZE, 4);

	p = number(p, pend, *val, 16, size, -1, SPECIAL | SMALL | ZEROPAD);
	*p = 0;

	return string(buf, end, sym, field_width, precision, flags);
}

Architectures can define PHYS_ADDR_T_SIZE to a variable if they want to be able to detect it at boot-time. Or just define it to a number (eg 36 for PAE).

By the way, the patch I'm replying to has a bug; you mis-sized 'sym'. It needs three extra bytes, not two.

Signature · 4 lines
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
author
#14 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

In reply to Ingo Molnar
Quoted 7 lines · Ingo Molnar

got rid of the brackets, see the patches below.

One open question would be whether to set the width to 8 on 32-bit platforms and 16 on 64-bit platforms - right now it's 8 on both. Since this is specifically a 'physical address' thing it might make sense to extend that on 64-bit systems. (although it's quite a bit of screen real estate so i think the current width of 8 should be fine)

A -lot- of 64-bit platforms (though not all of them) have most of their stuff still in the 32 bit space, especially when IO is concerned. Keeping it to 8 thus makes the output nicer on those, and as Linus mentioned before, it's not like we lose digits anyway.

If you want, you can re-use the #ifdef/#define I did for resources and thus give archs the option to have a different default.

Cheers,
Ben.

author
#15 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

In reply to Ingo Molnar

lib/vsprintf.c static char *resource_string(char *buf, char *end, struct resource *res, int fie · line 587

Quoted 5 lines
lib/vsprintf.c +4 -0
+static char *phys_addr_string(char *buf, char *end, phys_addr_t *val, int field_width, int precision, int flags)
+{
+	/* room for the actual number, the "0x" and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 2];

Aren't you missing one byte here ?

Cheers,
Ben.

Quoted 367 lines
lib/vsprintf.c +14 -0
+	char *p = sym, *pend = sym + sizeof(sym);
+	int size = 8;
+
+	p = number(p, pend, *val, 16, size, -1, SPECIAL | SMALL | ZEROPAD);
+	*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

@@ -592,6 +605,8 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
  * - 'S' For symbolic direct pointers
  * - 'R' For a struct resource pointer, it prints the range of
  *       addresses (not the name nor the flags)
+ * - 'P' For a phys_addr_t pointer, it prints the physical
+ *       addresses (with a minimum width of 8 characters)
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a

@@ -607,6 +622,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
 	case 'R':
 		return resource_string(buf, end, ptr, field_width, precision, flags);
+	case 'P':
+		return phys_addr_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {

@@ -627,6 +644,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
  * %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
+ * %pP output the address in a pointer to a phys_addr_t type
  *
  * The return value is the number of characters which would
  * be generated for the given input, excluding the trailing

commit 0d391458be88baf3c079e60c3ea331ebe12902c0

Author: Benjamin Herrenschmidt <[email protected]>
Date:   Mon Oct 20 15:07:37 2008 +1100

pci: use new %pR to print resource ranges

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
drivers/pci/pci.c +2 -3
index c9884bb..dbe9f39 100644
@@ -1358,11 +1358,10 @@ int pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
 	return 0;
 
 err_out:
-	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region [%#llx-%#llx]\n",
+	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region %pR\n",
 		 bar,
 		 pci_resource_flags(pdev, bar) & IORESOURCE_IO ? "I/O" : "mem",
-		 (unsigned long long)pci_resource_start(pdev, bar),
-		 (unsigned long long)pci_resource_end(pdev, bar));
+		 &pdev->resource[bar]);
 	return -EBUSY;
 }
drivers/pci/probe.c +13 -15
index dd9161a..d3db8b2 100644
@@ -304,9 +304,8 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		} else {
 			res->start = l64;
 			res->end = l64 + sz64;
-			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: [%llx, %llx]\n",
-				pci_name(dev), pos, (unsigned long long)res->start,
-				(unsigned long long)res->end);
+			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: %pR\n",
+				pci_name(dev), pos, res);
 		}
 	} else {
 		sz = pci_size(l, sz, mask);

@@ -316,9 +315,10 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 		res->start = l;
 		res->end = l + sz;
-		printk(KERN_DEBUG "PCI: %s reg %x %s: [%llx, %llx]\n", pci_name(dev),
-			pos, (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
-			(unsigned long long)res->start, (unsigned long long)res->end);
+		printk(KERN_DEBUG "PCI: %s reg %x %s: %pR\n",
+		       pci_name(dev), pos,
+		       (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
+		       res);
 	}
 
  out:

@@ -389,9 +389,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 			res->start = base;
 		if (!res->end)
 			res->end = limit + 0xfff;
-		printk(KERN_DEBUG "PCI: bridge %s io port: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s io port: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[1];

@@ -403,9 +402,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[2];

@@ -441,9 +439,9 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM | IORESOURCE_PREFETCH;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: [%llx, %llx]\n",
-			pci_name(dev), (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64" : "32",
-			(unsigned long long) res->start, (unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: %pR\n",
+		       pci_name(dev),
+		       (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64":"32", res);
 	}
 }
drivers/pci/setup-bus.c +4 -9
index d5e2106..471a429 100644
@@ -356,10 +356,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, unsigned long
 			order = __ffs(align) - 20;
 			if (order > 11) {
 				dev_warn(&dev->dev, "BAR %d bad alignment %llx: "
-				       "%#016llx-%#016llx\n", i,
-				       (unsigned long long)align,
-				       (unsigned long long)r->start,
-				       (unsigned long long)r->end);
+					 "%pR\n", i, (unsigned long long)align, r);
 				r->flags = 0;
 				continue;
 			}

@@ -539,11 +536,9 @@ static void pci_bus_dump_res(struct pci_bus *bus)
                 if (!res)
                         continue;
 
-		printk(KERN_INFO "bus: %02x index %x %s: [%llx, %llx]\n",
-			bus->number, i,
-			(res->flags & IORESOURCE_IO) ? "io port" : "mmio",
-			(unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_INFO "bus: %02x index %x %s: %pR\n",
+		       bus->number, i,
+		       (res->flags & IORESOURCE_IO) ? "io port" : "mmio", res);
         }
 }
drivers/pci/setup-res.c +13 -27
index 1a5fc83..d4b5c69 100644
@@ -49,10 +49,8 @@ void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
 
 	pcibios_resource_to_bus(dev, &region, res);
 
-	dev_dbg(&dev->dev, "BAR %d: got res [%#llx-%#llx] bus [%#llx-%#llx] "
-		"flags %#lx\n", resno,
-		 (unsigned long long)res->start,
-		 (unsigned long long)res->end,
+	dev_dbg(&dev->dev, "BAR %d: got res %pR bus [%#llx-%#llx] "
+		"flags %#lx\n", resno, res,
 		 (unsigned long long)region.start,
 		 (unsigned long long)region.end,
 		 (unsigned long)res->flags);

@@ -114,13 +112,11 @@ int pci_claim_resource(struct pci_dev *dev, int resource)
 		err = insert_resource(root, res);
 
 	if (err) {
-		dev_err(&dev->dev, "BAR %d: %s of %s [%#llx-%#llx]\n",
+		dev_err(&dev->dev, "BAR %d: %s of %s %pR\n",
 			resource,
 			root ? "address space collision on" :
 				"no parent found for",
-			dtype,
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+			dtype, res);
 	}
 
 	return err;

@@ -139,9 +135,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	align = resource_alignment(res);
 	if (!align) {
 		dev_err(&dev->dev, "BAR %d: can't allocate resource (bogus "
-			"alignment) [%#llx-%#llx] flags %#lx\n",
-			resno, (unsigned long long)res->start,
-			(unsigned long long)res->end, res->flags);
+			"alignment) %pR flags %#lx\n",
+			resno, res, res->flags);
 		return -EINVAL;
 	}
 

@@ -162,11 +157,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx]\n", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else {
 		res->flags &= ~IORESOURCE_STARTALIGN;
 		if (resno < PCI_BRIDGE_RESOURCES)

@@ -202,11 +194,8 @@ int pci_assign_resource_fixed(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx\n]", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else if (resno < PCI_BRIDGE_RESOURCES) {
 		pci_update_resource(dev, res, resno);
 	}

@@ -237,9 +226,8 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 		r_align = resource_alignment(r);
 		if (!r_align) {
 			dev_warn(&dev->dev, "BAR %d: bogus alignment "
-				"[%#llx-%#llx] flags %#lx\n",
-				i, (unsigned long long)r->start,
-				(unsigned long long)r->end, r->flags);
+				"%pR flags %#lx\n",
+				i, r, r->flags);
 			continue;
 		}
 		for (list = head; ; list = list->next) {

@@ -287,9 +275,7 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
 
 		if (!r->parent) {
 			dev_err(&dev->dev, "device not available because of "
-				"BAR %d [%#llx-%#llx] collisions\n", i,
-				(unsigned long long) r->start,
-				(unsigned long long) r->end);
+				"BAR %d %pR collisions\n", i, r);
 			return -EINVAL;
 		}

commit c21f132b1eca94808fe72b31aa159c7f0ad61d25

Author: Linus Torvalds <[email protected]>
Date:   Mon Oct 20 15:07:34 2008 +1100

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

Padding now defaults to 4 digits for IO and 8 for MEM

Signed-off-by Linus Torvalds <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
lib/vsprintf.c +43 -6
index cceecb6..a013bbc 100644
@@ -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,51 @@ static char *symbol_string(char *buf, char *end, void *ptr, int field_width, int
 #endif
 }
 
+static char *resource_string(char *buf, char *end, struct resource *res, int field_width, int precision, int flags)
+{
+#ifndef IO_RSRC_PRINTK_SIZE
+#define IO_RSRC_PRINTK_SIZE	4
+#endif
+
+#ifndef MEM_RSRC_PRINTK_SIZE
+#define MEM_RSRC_PRINTK_SIZE	8
+#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 = -1;
+
+	if (res->flags & IORESOURCE_IO)
+		size = IO_RSRC_PRINTK_SIZE;
+	else if (res->flags & IORESOURCE_MEM)
+		size = MEM_RSRC_PRINTK_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).
+ * Right now we handle:
+ *
+ * - 'F' For symbolic function descriptor pointers
+ * - 'S' For symbolic direct pointers
+ * - 'R' For a struct resource pointer, it prints the range of
+ *       addresses (not the name nor the flags)
  *
- * 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. 
+ * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
+ * function pointers are really function descriptors, which contain a
+ * pointer to the real address.
  */
 static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field_width, int precision, int flags)
 {

@@ -571,6 +605,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
 		/* 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 +626,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
  * 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
#16 lore raw

Re: [PATCH] x86, ioremap: use %pR in printk

In reply to Benjamin Herrenschmidt
Quoted 15 lines · Benjamin Herrenschmidt
Quoted 8 lines

so how about something like the two patches below (ontop of Linus's patch): the first patch introduces a "small" resource pointer printout
format: %pr - the little brother of %pR.

The output format is [0x00001234] - minimum width is 8.

The second patch takes advantage of it in ioremap.c.

Well, I did the exact same patch except I used the same function and just added a flag for "R" vs. "r". However, I didn't post it because I wasn't too happy with passing by pointer and I wasn't sure whether we wanted to keep the letter after p uppercase or not... In any case, I kept it as a thing to discuss after the first one goes in.

Quoted 6 lines

At this stage, I'm tempted to go for a %pP for printing a pointer to a phys_addr_t (and that's the same as resource_size_t, just more generic nowadays, since those were consolidated).

Still not too happy with the pointer thing but that's the best we can do I suppose without losing gcc type checking.

%pP sounds very good, and phys_addr_t is indeed the better choice as recently we mapped resource_size_t to phys_addr_t (they used to be separate for no good reason, up to a few days ago).

Below are the updated patches that implement this.

Ingo

-------------->
commit 73301b25ba6df2a54727a9fc27614787a5143dca

Author: Ingo Molnar <[email protected]>
Date:   Mon Oct 20 09:08:57 2008 +0200

x86, ioremap: use %pP in printk

use the new %pP physical address printk type conversion specifier.

Signed-off-by Ingo Molnar <[email protected]>
arch/x86/mm/ioremap.c +5 -5
index ae71e11..aaa81d9 100644
@@ -207,8 +207,8 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		return NULL;
 
 	if (!phys_addr_valid(phys_addr)) {
-		printk(KERN_WARNING "ioremap: invalid physical address %llx\n",
-		       (unsigned long long)phys_addr);
+		printk(KERN_WARNING "ioremap: invalid physical address %pP\n",
+		       &phys_addr);
 		WARN_ON_ONCE(1);
 		return NULL;
 	}

@@ -267,9 +267,9 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 		    (prot_val == _PAGE_CACHE_WC &&
 		     new_prot_val == _PAGE_CACHE_WB)) {
 			pr_debug(
-		"ioremap error for 0x%llx-0x%llx, requested 0x%lx, got 0x%lx\n",
-				(unsigned long long)phys_addr,
-				(unsigned long long)(phys_addr + size),
+		"ioremap error for %pP [%lx], requested 0x%lx, got 0x%lx\n",
+				&phys_addr,
+				size,
 				prot_val, new_prot_val);
 			free_memtype(phys_addr, phys_addr + size);
 			return NULL;

commit 7fd44cc79290a613c2de83ca9ac624f6b04d7908

Author: Ingo Molnar <[email protected]>
Date:   Mon Oct 20 12:39:17 2008 +0200

vsprintf: implement %pP to print phys_addr_t

Add %pP to print addresses in phys_addr_t variables. Passed by reference.

Signed-off-by Ingo Molnar <[email protected]>
lib/vsprintf.c +20 -0
index a013bbc..3baed89 100644
@@ -581,6 +581,21 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
 	return string(buf, end, sym, field_width, precision, flags);
 }
 
+static char *phys_addr_string(char *buf, char *end, phys_addr_t *val, int field_width, int precision, int flags)
+{
+	/* room for the actual number, the one "0x", [, ] and the final zero */
+	char sym[2*sizeof(phys_addr_t) + 5];
+	char *p = sym, *pend = sym + sizeof(sym);
+	int size = 8;
+
+	*p++ = '[';
+	p = number(p, pend, *val, 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

@@ -592,6 +607,8 @@ static char *resource_string(char *buf, char *end, struct resource *res, int fie
  * - 'S' For symbolic direct pointers
  * - 'R' For a struct resource pointer, it prints the range of
  *       addresses (not the name nor the flags)
+ * - 'P' For a phys_addr_t pointer, it prints the physical
+ *       addresses (in %pR format)
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a

@@ -607,6 +624,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
 		return symbol_string(buf, end, ptr, field_width, precision, flags);
 	case 'R':
 		return resource_string(buf, end, ptr, field_width, precision, flags);
+	case 'P':
+		return phys_addr_string(buf, end, ptr, field_width, precision, flags);
 	}
 	flags |= SMALL;
 	if (field_width == -1) {

@@ -627,6 +646,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field
  * %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
+ * %pP output the address in a pointer to a phys_addr_t type
  *
  * The return value is the number of characters which would
  * be generated for the given input, excluding the trailing

commit 0d391458be88baf3c079e60c3ea331ebe12902c0

Author: Benjamin Herrenschmidt <[email protected]>
Date:   Mon Oct 20 15:07:37 2008 +1100

pci: use new %pR to print resource ranges

This converts things in drivers/pci to use %pR to printout the content of a struct resource instead of hand-casted %llx or other variants.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
drivers/pci/pci.c +2 -3
index c9884bb..dbe9f39 100644
@@ -1358,11 +1358,10 @@ int pci_request_region(struct pci_dev *pdev, int bar, const char *res_name)
 	return 0;
 
 err_out:
-	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region [%#llx-%#llx]\n",
+	dev_warn(&pdev->dev, "BAR %d: can't reserve %s region %pR\n",
 		 bar,
 		 pci_resource_flags(pdev, bar) & IORESOURCE_IO ? "I/O" : "mem",
-		 (unsigned long long)pci_resource_start(pdev, bar),
-		 (unsigned long long)pci_resource_end(pdev, bar));
+		 &pdev->resource[bar]);
 	return -EBUSY;
 }
drivers/pci/probe.c +13 -15
index dd9161a..d3db8b2 100644
@@ -304,9 +304,8 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		} else {
 			res->start = l64;
 			res->end = l64 + sz64;
-			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: [%llx, %llx]\n",
-				pci_name(dev), pos, (unsigned long long)res->start,
-				(unsigned long long)res->end);
+			printk(KERN_DEBUG "PCI: %s reg %x 64bit mmio: %pR\n",
+				pci_name(dev), pos, res);
 		}
 	} else {
 		sz = pci_size(l, sz, mask);

@@ -316,9 +315,10 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 		res->start = l;
 		res->end = l + sz;
-		printk(KERN_DEBUG "PCI: %s reg %x %s: [%llx, %llx]\n", pci_name(dev),
-			pos, (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
-			(unsigned long long)res->start, (unsigned long long)res->end);
+		printk(KERN_DEBUG "PCI: %s reg %x %s: %pR\n",
+		       pci_name(dev), pos,
+		       (res->flags & IORESOURCE_IO) ? "io port":"32bit mmio",
+		       res);
 	}
 
  out:

@@ -389,9 +389,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 			res->start = base;
 		if (!res->end)
 			res->end = limit + 0xfff;
-		printk(KERN_DEBUG "PCI: bridge %s io port: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s io port: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[1];

@@ -403,9 +402,8 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: [%llx, %llx]\n",
-			pci_name(dev), (unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s 32bit mmio: %pR\n",
+		       pci_name(dev), res);
 	}
 
 	res = child->resource[2];

@@ -441,9 +439,9 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM | IORESOURCE_PREFETCH;
 		res->start = base;
 		res->end = limit + 0xfffff;
-		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: [%llx, %llx]\n",
-			pci_name(dev), (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64" : "32",
-			(unsigned long long) res->start, (unsigned long long) res->end);
+		printk(KERN_DEBUG "PCI: bridge %s %sbit mmio pref: %pR\n",
+		       pci_name(dev),
+		       (res->flags & PCI_PREF_RANGE_TYPE_64) ? "64":"32", res);
 	}
 }
drivers/pci/setup-bus.c +4 -9
index d5e2106..471a429 100644
@@ -356,10 +356,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, unsigned long
 			order = __ffs(align) - 20;
 			if (order > 11) {
 				dev_warn(&dev->dev, "BAR %d bad alignment %llx: "
-				       "%#016llx-%#016llx\n", i,
-				       (unsigned long long)align,
-				       (unsigned long long)r->start,
-				       (unsigned long long)r->end);
+					 "%pR\n", i, (unsigned long long)align, r);
 				r->flags = 0;
 				continue;
 			}

@@ -539,11 +536,9 @@ static void pci_bus_dump_res(struct pci_bus *bus)
                 if (!res)
                         continue;
 
-		printk(KERN_INFO "bus: %02x index %x %s: [%llx, %llx]\n",
-			bus->number, i,
-			(res->flags & IORESOURCE_IO) ? "io port" : "mmio",
-			(unsigned long long) res->start,
-			(unsigned long long) res->end);
+		printk(KERN_INFO "bus: %02x index %x %s: %pR\n",
+		       bus->number, i,
+		       (res->flags & IORESOURCE_IO) ? "io port" : "mmio", res);
         }
 }
drivers/pci/setup-res.c +13 -27
index 1a5fc83..d4b5c69 100644
@@ -49,10 +49,8 @@ void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
 
 	pcibios_resource_to_bus(dev, &region, res);
 
-	dev_dbg(&dev->dev, "BAR %d: got res [%#llx-%#llx] bus [%#llx-%#llx] "
-		"flags %#lx\n", resno,
-		 (unsigned long long)res->start,
-		 (unsigned long long)res->end,
+	dev_dbg(&dev->dev, "BAR %d: got res %pR bus [%#llx-%#llx] "
+		"flags %#lx\n", resno, res,
 		 (unsigned long long)region.start,
 		 (unsigned long long)region.end,
 		 (unsigned long)res->flags);

@@ -114,13 +112,11 @@ int pci_claim_resource(struct pci_dev *dev, int resource)
 		err = insert_resource(root, res);
 
 	if (err) {
-		dev_err(&dev->dev, "BAR %d: %s of %s [%#llx-%#llx]\n",
+		dev_err(&dev->dev, "BAR %d: %s of %s %pR\n",
 			resource,
 			root ? "address space collision on" :
 				"no parent found for",
-			dtype,
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+			dtype, res);
 	}
 
 	return err;

@@ -139,9 +135,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	align = resource_alignment(res);
 	if (!align) {
 		dev_err(&dev->dev, "BAR %d: can't allocate resource (bogus "
-			"alignment) [%#llx-%#llx] flags %#lx\n",
-			resno, (unsigned long long)res->start,
-			(unsigned long long)res->end, res->flags);
+			"alignment) %pR flags %#lx\n",
+			resno, res, res->flags);
 		return -EINVAL;
 	}
 

@@ -162,11 +157,8 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx]\n", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else {
 		res->flags &= ~IORESOURCE_STARTALIGN;
 		if (resno < PCI_BRIDGE_RESOURCES)

@@ -202,11 +194,8 @@ int pci_assign_resource_fixed(struct pci_dev *dev, int resno)
 	}
 
 	if (ret) {
-		dev_err(&dev->dev, "BAR %d: can't allocate %s resource "
-			"[%#llx-%#llx\n]", resno,
-			res->flags & IORESOURCE_IO ? "I/O" : "mem",
-			(unsigned long long)res->start,
-			(unsigned long long)res->end);
+		dev_err(&dev->dev, "BAR %d: can't allocate %s resource %pR\n",
+			resno, res->flags & IORESOURCE_IO ? "I/O" : "mem", res);
 	} else if (resno < PCI_BRIDGE_RESOURCES) {
 		pci_update_resource(dev, res, resno);
 	}

@@ -237,9 +226,8 @@ void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
 		r_align = resource_alignment(r);
 		if (!r_align) {
 			dev_warn(&dev->dev, "BAR %d: bogus alignment "
-				"[%#llx-%#llx] flags %#lx\n",
-				i, (unsigned long long)r->start,
-				(unsigned long long)r->end, r->flags);
+				"%pR flags %#lx\n",
+				i, r, r->flags);
 			continue;
 		}
 		for (list = head; ; list = list->next) {

@@ -287,9 +275,7 @@ int pci_enable_resources(struct pci_dev *dev, int mask)
 
 		if (!r->parent) {
 			dev_err(&dev->dev, "device not available because of "
-				"BAR %d [%#llx-%#llx] collisions\n", i,
-				(unsigned long long) r->start,
-				(unsigned long long) r->end);
+				"BAR %d %pR collisions\n", i, r);
 			return -EINVAL;
 		}

commit c21f132b1eca94808fe72b31aa159c7f0ad61d25

Author: Linus Torvalds <[email protected]>
Date:   Mon Oct 20 15:07:34 2008 +1100

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

Padding now defaults to 4 digits for IO and 8 for MEM

Signed-off-by Linus Torvalds <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
lib/vsprintf.c +43 -6

79 lines not on this page · raw message