[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 <benh@kernel.crashing.org>
---

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

--- linux-work.orig/drivers/pci/probe.c 2008-10-17 16:11:00.000000000 +1100
+++ linux-work/drivers/pci/probe.c 2008-10-20 14:44:26.000000000 +1100
@@ -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);
}
}

Index: linux-work/drivers/pci/setup-bus.c
===================================================================
--- linux-work.orig/drivers/pci/setup-bus.c 2008-10-17 16:11:00.000000000 +1100
+++ linux-work/drivers/pci/setup-bus.c 2008-10-20 14:44:26.000000000 +1100
@@ -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);
}
}

Index: linux-work/drivers/pci/setup-res.c
===================================================================
--- linux-work.orig/drivers/pci/setup-res.c 2008-10-17 16:11:00.000000000 +1100
+++ linux-work/drivers/pci/setup-res.c 2008-10-20 14:44:26.000000000 +1100
@@ -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;
}

Index: linux-work/drivers/pci/pci.c
===================================================================
--- linux-work.orig/drivers/pci/pci.c 2008-10-17 16:11:00.000000000 +1100
+++ linux-work/drivers/pci/pci.c 2008-10-20 14:44:26.000000000 +1100
@@ -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;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
[color=blue]
> 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 <benh@kernel.crashing.org>[/color]

cool!

Acked-by: Ingo Molnar <mingo@elte.hu>

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

Ingo

--------------------->
From 3b4f8fd4ff54a2da035e972a610b8d5ac8d8eabb Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
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 <mingo@elte.hu>
---
arch/x86/mm/ioremap.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index ae71e11..640c653 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Mon, 2008-10-20 at 09:12 +0200, Ingo Molnar wrote:[color=blue]
> * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>[color=green]
> > 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 <benh@kernel.crashing.org>[/color]
>
> cool!
>
> Acked-by: Ingo Molnar <mingo@elte.hu>
>
> there's also two places in arch/x86/mm/ioremap.c that could use this
> straight away - see the (untested) patch below.[/color]

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
[color=blue]
> On Mon, 2008-10-20 at 09:12 +0200, Ingo Molnar wrote:[color=green]
> > * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >[color=darkred]
> > > 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 <benh@kernel.crashing.org>[/color]
> >
> > cool!
> >
> > Acked-by: Ingo Molnar <mingo@elte.hu>
> >
> > there's also two places in arch/x86/mm/ioremap.c that could use this
> > straight away - see the (untested) patch below.[/color]
>
> No, you don't pass it a phys_addr_t or a resource_size_t, you pass it a
> struct resource *[/color]

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.
[color=blue]
> 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...[/color]

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

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

* Ingo Molnar <mingo@elte.hu> wrote:
[color=blue][color=green]
> > No, you don't pass it a phys_addr_t or a resource_size_t, you pass
> > it a struct resource *[/color]
>
> 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.[/color]

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

---------------->
From bec931cb306f0901b9e2017c845d5ad6016574e4 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Mon, 20 Oct 2008 12:39:17 +0200
Subject: [PATCH] vsprintf: implement %pr to print resource_size_t

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
lib/vsprintf.c | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a013bbc..ddcaa6e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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

From cfbd2eec241a53bc5c1b95bb68f269036e3e92a4 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
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 <mingo@elte.hu>
---
arch/x86/mm/ioremap.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index ae71e11..4725c7a 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

[color=blue]
> 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.[/color]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Mon, 2008-10-20 at 22:15 +1100, Benjamin Herrenschmidt wrote:[color=blue][color=green]
> > 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.[/color]
>
> 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.[/color]

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

Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

* Ingo Molnar <mingo@elte.hu> wrote:
[color=blue]
>
> * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>[color=green][color=darkred]
> > > Still not too happy with the pointer thing but that's the best we
> > > can do I suppose without losing gcc type checking.[/color]
> >
> > Oh, and I didn't like having the brackets around something that isn't
> > a range too but that's a minor details.[/color]
>
> it looked quite natural in printouts to me, but no strong feelings.[/color]

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 <mingo@elte.hu>
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 <mingo@elte.hu>

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index ae71e11..aaa81d9 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -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 <mingo@elte.hu>
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 <mingo@elte.hu>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a013bbc..aac4fff 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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 <benh@kernel.crashing.org>
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 <benh@kernel.crashing.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9884bb..dbe9f39 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
}

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dd9161a..d3db8b2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
}
}

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d5e2106..471a429 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -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);
}
}

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 1a5fc83..d4b5c69 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -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 <torvalds@linux-foundation.org>
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 <torvalds@linux-foundation.org>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index cceecb6..a013bbc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
[color=blue]
>[color=green]
> > 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.[/color]
>
> 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.[/color]

%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 <mingo@elte.hu>
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 <mingo@elte.hu>

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index ae71e11..aaa81d9 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -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 <mingo@elte.hu>
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 <mingo@elte.hu>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a013bbc..3baed89 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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 <benh@kernel.crashing.org>
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 <benh@kernel.crashing.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9884bb..dbe9f39 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
}

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dd9161a..d3db8b2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
}
}

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d5e2106..471a429 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -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);
}
}

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 1a5fc83..d4b5c69 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -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 <torvalds@linux-foundation.org>
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 <torvalds@linux-foundation.org>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index cceecb6..a013bbc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
[color=blue][color=green]
> > Still not too happy with the pointer thing but that's the best we
> > can do I suppose without losing gcc type checking.[/color]
>
> Oh, and I didn't like having the brackets around something that isn't
> a range too but that's a minor details.[/color]

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

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:
[color=blue]
> + /* room for the actual number, the "0x" and the final zero */
> + char sym[2*sizeof(phys_addr_t) + 2];[/color]

Buffer overflow.

johannes

-----BEGIN PGP SIGNATURE-----
Comment: Johannes Berg (powerbook)

iQIcBAABAgAGBQJI/IBbAAoJEKVg1VMiehFY7KgP/1FVB7epsiKR+CjLPmtRKjvK
gtpkBfes5COvdRNJ4dwHv8zXvhC/gwWDMTJfixZoPrIEdBiC2FT4oIlEhdoBT9My
v3T4LCqRPTOM9iWfgaE/16x855EZLvR12sDYGJiKcjixs2Tju2lBnDSZoZMJGmdx
hLE41nsr1NmXwnV1Kc6i/IMZhLxGk4Ofr6d8kS9gzidaRgd8kYs06JSk8Ko6EBxM
7QETMiFuHCsoJUtTikowtB/CN9yNrrZtSR1rmVM/k9m3D7mSLBukErujVpcG1Tuu
ZEmOG0T5ofo0c4ZVyjHY3zU9svxp53TbfYlZPcuph+v5as3BnmQzV0mVIHLg6JXE
Gz95A0jWWkfdxBG6h/mMPPG1DsGJ8DPd95VBZoXX2YDajzfoWTwqCZXZ0QR3uXO0
i3kPmBkV+bbCcgAgRyCRTVec+fMHhKRUKlBGq9NGR8KiPmCyHYrRxymO8dT+2DZF
Zhw8uSb7DNaRFKWrHKWs1Y7+E6vhWSgs5jq/NTVH0iJQLmfATpU4s/+754ARRGBe
FPwu+7u/HwX8/cO8GrcaVl0Xv4YFXtVv7t1b6Cittf8wNJWj5SvzuxThBwaplpGl
K0D5uqiCoxh55keYuB5GeAvQGAYLgoVMPCZNJ1Ve6/jpBamFKPbZoF5wBHFQPK/N
F0+eRoEhhiHrnIH0G8lx
=HApQ
-----END PGP SIGNATURE-----

On Mon, Oct 20, 2008 at 01:36:02PM +0200, Ingo Molnar wrote:[color=blue]
> 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)[/color]

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.

--
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."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

* Johannes Berg <johannes@sipsolutions.net> wrote:
[color=blue]
> On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:
>[color=green]
> > + /* room for the actual number, the "0x" and the final zero */
> > + char sym[2*sizeof(phys_addr_t) + 2];[/color]
>
> Buffer overflow.[/color]

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

Ingo

-------------------->
commit e8613ba0d3912d526c9de2317f40ca2a236c026d
Author: Ingo Molnar <mingo@elte.hu>
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 <mingo@elte.hu>

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index ae71e11..aaa81d9 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -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 <mingo@elte.hu>
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 <mingo@elte.hu>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a013bbc..d0a3e2c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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 <benh@kernel.crashing.org>
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 <benh@kernel.crashing.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9884bb..dbe9f39 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
}

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dd9161a..d3db8b2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
}
}

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d5e2106..471a429 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -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);
}
}

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 1a5fc83..d4b5c69 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -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 <torvalds@linux-foundation.org>
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 <torvalds@linux-foundation.org>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index cceecb6..a013bbc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:[color=blue]
> 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)[/color]

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

[color=blue]
> +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];[/color]

Aren't you missing one byte here ?

Cheers,
Ben.
[color=blue]
> + 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 <benh@kernel.crashing.org>
> 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 <benh@kernel.crashing.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c9884bb..dbe9f39 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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;
> }
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd9161a..d3db8b2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);
> }
> }
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index d5e2106..471a429 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -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);
> }
> }
>
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 1a5fc83..d4b5c69 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -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 <torvalds@linux-foundation.org>
> 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 <torvalds@linux-foundation.org>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index cceecb6..a013bbc 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -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[/color]

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Mon, 2008-10-20 at 14:58 +0200, Johannes Berg wrote:[color=blue]
> On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:
>[color=green]
> > + /* room for the actual number, the "0x" and the final zero */
> > + char sym[2*sizeof(phys_addr_t) + 2];[/color]
>
> Buffer overflow.[/color]

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

Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Tue, 2008-10-21 at 08:35 +1100, Benjamin Herrenschmidt wrote:[color=blue]
> On Mon, 2008-10-20 at 14:58 +0200, Johannes Berg wrote:[color=green]
> > On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:
> > [color=darkred]
> > > + /* room for the actual number, the "0x" and the final zero */
> > > + char sym[2*sizeof(phys_addr_t) + 2];[/color]
> >
> > Buffer overflow.[/color]
>
> It's actually not going to overflow as we pass the end of the buffer to
> "number" but we'll miss a digit.[/color]

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

johannes

-----BEGIN PGP SIGNATURE-----
Comment: Johannes Berg (powerbook)

iQIcBAABAgAGBQJI/Xr9AAoJEKVg1VMiehFYAtIP/iJV5VlO+Gnth9KzRnKG4B8k
bC0LF9Q5uIxxSv4pZgLOiLAfVsEpMb7N8BztQ+9DplkFbFOGDeVTpBgCaQb2arIO
lsxN9hhP2bVwjqQdMnOzB3i69rLpFG8vFMXWajQg+RF3+a/ntLyqnKtlIdQAikKS
PsVoEliwrTHf1tIKcjvVycGVYYayhlB3EOUh7MAjNIy4HT1CbPyKcaXt0hc22w3u
jWZzZ7ceU2uFlMKLPpeI+eX3GXHCGWMPmTVVFXzGd+nkv0aD42edUGIktYQNR9N8
r/BZVYxvo58UrsRKGqBbAdhyHOXnLoyNAdxXgcvDMCr8HCp0tJPpejchrj0kjuAa
GtkJ6oH/0RBSBLfqp2Ib2kuWlg19KkHSX1ctEKzFxWsL4GqKXofEzUqpG2fqocZX
U9SXQwyG/4l+cZJwZD3Qybj5GZbJXeee5fVinrqssSh42jh9/kQFMmUJah9Dcyi+
h2QNu8fFrlgqumK+wJ+KFmQZ9AdAZ76PH39jOKq7Hmn/GU1S6uFCDQPlhd2zY5Dd
bohXJ6UqN6voE41EAbQgdWdIR+/pfvC6h0cnXjJBoLOiIqSyE3Fn+ZXXCzyijgFM
8qX6Di3VXIjnMLLu201OLQ2BJ0TQf9okWTkKuTtzB/Qb4tFy+051wFZ8h35T9Ise
btYZuWGR+mNr+472dz3i
=Gl+r
-----END PGP SIGNATURE-----