2008-10-20 04:08:46

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: [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(-)

--- 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;
}


2008-10-20 07:12:21

by Ingo Molnar

[permalink] [raw]
Subject: [PATCH] x86, ioremap: use %pR in printk


* Benjamin Herrenschmidt <[email protected]> wrote:

> 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

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

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;

2008-10-20 08:35:47

by Benjamin Herrenschmidt

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

On Mon, 2008-10-20 at 09:12 +0200, Ingo Molnar wrote:
> * Benjamin Herrenschmidt <[email protected]> wrote:
>
> > 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.

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.

2008-10-20 09:05:29

by Ingo Molnar

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


* Benjamin Herrenschmidt <[email protected]> wrote:

> On Mon, 2008-10-20 at 09:12 +0200, Ingo Molnar wrote:
> > * Benjamin Herrenschmidt <[email protected]> wrote:
> >
> > > 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.
>
> 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.

> 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

2008-10-20 11:00:49

by Ingo Molnar

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


* Ingo Molnar <[email protected]> wrote:

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

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

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

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;

2008-10-20 11:16:20

by Benjamin Herrenschmidt

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


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

2008-10-20 11:23:25

by Benjamin Herrenschmidt

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

On Mon, 2008-10-20 at 22:15 +1100, Benjamin Herrenschmidt wrote:
> > 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.

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

Ben.

2008-10-20 11:30:47

by Ingo Molnar

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


* Benjamin Herrenschmidt <[email protected]> wrote:

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

%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]>

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

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

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

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

2008-10-20 11:32:11

by Ingo Molnar

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


* Benjamin Herrenschmidt <[email protected]> wrote:

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

2008-10-20 11:36:29

by Ingo Molnar

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


* Ingo Molnar <[email protected]> wrote:

>
> * Benjamin Herrenschmidt <[email protected]> wrote:
>
> > > 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]>

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

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

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

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

2008-10-20 12:58:33

by Johannes Berg

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

On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:

> + /* room for the actual number, the "0x" and the final zero */
> + char sym[2*sizeof(phys_addr_t) + 2];

Buffer overflow.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-10-20 13:04:55

by Matthew Wilcox

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

On Mon, Oct 20, 2008 at 01:36:02PM +0200, Ingo Molnar wrote:
> 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.

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

2008-10-20 13:16:32

by Ingo Molnar

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


* Johannes Berg <[email protected]> wrote:

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

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

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

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

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

2008-10-20 21:34:18

by Benjamin Herrenschmidt

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

On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:
> 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.

2008-10-20 21:35:29

by Benjamin Herrenschmidt

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


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

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

2008-10-20 21:36:15

by Benjamin Herrenschmidt

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

On Mon, 2008-10-20 at 14:58 +0200, Johannes Berg wrote:
> On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:
>
> > + /* 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.

2008-10-21 06:48:01

by Johannes Berg

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

On Tue, 2008-10-21 at 08:35 +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2008-10-20 at 14:58 +0200, Johannes Berg wrote:
> > On Mon, 2008-10-20 at 13:36 +0200, Ingo Molnar wrote:
> >
> > > + /* 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


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part