The current pci_assign_unassigned_resources() code doesn't work properly
on 32 bits platforms with 64 bits resources. The main reason is the use
of unsigned long in various places instead of resource_size_t.
This fixes it, along with some tricks to avoid casting to 64 bits on
platforms that don't need it in every printk around.
This is a pre-requisite for making powerpc use the generic code instead of
its own half-useful implementation.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
This version fixes some stupid warnings when using 32 bits resources
drivers/pci/pci.h | 11 +++++++++++
drivers/pci/setup-bus.c | 32 +++++++++++++++++---------------
drivers/pci/setup-res.c | 5 ++---
include/linux/pci.h | 4 ++--
4 files changed, 32 insertions(+), 20 deletions(-)
Index: linux-work/drivers/pci/pci.h
===================================================================
--- linux-work.orig/drivers/pci/pci.h 2007-12-05 11:55:49.000000000 +1100
+++ linux-work/drivers/pci/pci.h 2007-12-05 13:37:45.000000000 +1100
@@ -91,3 +91,14 @@ pci_match_one_device(const struct pci_de
}
struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
+
+#ifdef CONFIG_RESOURCES_64BIT
+#define RESOURCE_ORDER(order) (1ULL << (order))
+#define RES_PR "%016llx"
+#else
+#define RESOURCE_ORDER(order) (1UL << (order))
+#define RES_PR "%08x"
+#endif
+
+#define RANGE_PR RES_PR "-" RES_PR
+
Index: linux-work/drivers/pci/setup-bus.c
===================================================================
--- linux-work.orig/drivers/pci/setup-bus.c 2007-12-05 11:55:49.000000000 +1100
+++ linux-work/drivers/pci/setup-bus.c 2007-12-05 12:04:35.000000000 +1100
@@ -26,6 +26,7 @@
#include <linux/cache.h>
#include <linux/slab.h>
+#include "pci.h"
#define DEBUG_CONFIG 1
#if DEBUG_CONFIG
@@ -89,8 +90,9 @@ void pci_setup_cardbus(struct pci_bus *b
* The IO resource is allocated a range twice as large as it
* would normally need. This allows us to set both IO regs.
*/
- printk(" IO window: %08lx-%08lx\n",
- region.start, region.end);
+ printk(KERN_INFO " IO window: 0x%08lx-0x%08lx\n",
+ (unsigned long)region.start,
+ (unsigned long)region.end);
pci_write_config_dword(bridge, PCI_CB_IO_BASE_0,
region.start);
pci_write_config_dword(bridge, PCI_CB_IO_LIMIT_0,
@@ -99,7 +101,7 @@ void pci_setup_cardbus(struct pci_bus *b
pcibios_resource_to_bus(bridge, ®ion, bus->resource[1]);
if (bus->resource[1]->flags & IORESOURCE_IO) {
- printk(" IO window: %08lx-%08lx\n",
+ printk(KERN_INFO " IO window: "RANGE_PR"\n",
region.start, region.end);
pci_write_config_dword(bridge, PCI_CB_IO_BASE_1,
region.start);
@@ -109,7 +111,7 @@ void pci_setup_cardbus(struct pci_bus *b
pcibios_resource_to_bus(bridge, ®ion, bus->resource[2]);
if (bus->resource[2]->flags & IORESOURCE_MEM) {
- printk(" PREFETCH window: %08lx-%08lx\n",
+ printk(KERN_INFO " PREFETCH window: "RANGE_PR"\n",
region.start, region.end);
pci_write_config_dword(bridge, PCI_CB_MEMORY_BASE_0,
region.start);
@@ -119,7 +121,7 @@ void pci_setup_cardbus(struct pci_bus *b
pcibios_resource_to_bus(bridge, ®ion, bus->resource[3]);
if (bus->resource[3]->flags & IORESOURCE_MEM) {
- printk(" MEM window: %08lx-%08lx\n",
+ printk(KERN_INFO " MEM window: "RANGE_PR"\n",
region.start, region.end);
pci_write_config_dword(bridge, PCI_CB_MEMORY_BASE_1,
region.start);
@@ -159,7 +161,8 @@ pci_setup_bridge(struct pci_bus *bus)
/* Set up upper 16 bits of I/O base/limit. */
io_upper16 = (region.end & 0xffff0000) | (region.start >> 16);
DBG(KERN_INFO " IO window: %04lx-%04lx\n",
- region.start, region.end);
+ (unsigned long)region.start,
+ (unsigned long)region.end);
}
else {
/* Clear upper 16 bits of I/O base/limit. */
@@ -180,7 +183,7 @@ pci_setup_bridge(struct pci_bus *bus)
if (bus->resource[1]->flags & IORESOURCE_MEM) {
l = (region.start >> 16) & 0xfff0;
l |= region.end & 0xfff00000;
- DBG(KERN_INFO " MEM window: %08lx-%08lx\n",
+ DBG(KERN_INFO " MEM window: "RANGE_PR"\n",
region.start, region.end);
}
else {
@@ -199,7 +202,7 @@ pci_setup_bridge(struct pci_bus *bus)
if (bus->resource[2]->flags & IORESOURCE_PREFETCH) {
l = (region.start >> 16) & 0xfff0;
l |= region.end & 0xfff00000;
- DBG(KERN_INFO " PREFETCH window: %08lx-%08lx\n",
+ DBG(KERN_INFO " PREFETCH window: "RANGE_PR"\n",
region.start, region.end);
}
else {
@@ -323,8 +326,8 @@ static void pbus_size_io(struct pci_bus
static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, unsigned long type)
{
struct pci_dev *dev;
- unsigned long min_align, align, size;
- unsigned long aligns[12]; /* Alignments from 1Mb to 2Gb */
+ resource_size_t min_align, align, size;
+ resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */
int order, max_order;
struct resource *b_res = find_free_bus_resource(bus, type);
@@ -340,7 +343,7 @@ static int pbus_size_mem(struct pci_bus
for (i = 0; i < PCI_NUM_RESOURCES; i++) {
struct resource *r = &dev->resource[i];
- unsigned long r_size;
+ resource_size_t r_size;
if (r->parent || (r->flags & mask) != type)
continue;
@@ -350,10 +353,9 @@ static int pbus_size_mem(struct pci_bus
order = __ffs(align) - 20;
if (order > 11) {
printk(KERN_WARNING "PCI: region %s/%d "
- "too large: %llx-%llx\n",
+ "too large: " RANGE_PR "\n",
pci_name(dev), i,
- (unsigned long long)r->start,
- (unsigned long long)r->end);
+ r->start, r->end);
r->flags = 0;
continue;
}
@@ -372,7 +374,7 @@ static int pbus_size_mem(struct pci_bus
align = 0;
min_align = 0;
for (order = 0; order <= max_order; order++) {
- unsigned long align1 = 1UL << (order + 20);
+ resource_size_t align1 = RESOURCE_ORDER(order + 20);
if (!align)
min_align = align1;
Index: linux-work/drivers/pci/setup-res.c
===================================================================
--- linux-work.orig/drivers/pci/setup-res.c 2007-12-05 11:55:49.000000000 +1100
+++ linux-work/drivers/pci/setup-res.c 2007-12-05 12:04:35.000000000 +1100
@@ -51,9 +51,8 @@ pci_update_resource(struct pci_dev *dev,
pcibios_resource_to_bus(dev, ®ion, res);
- pr_debug(" got res [%llx:%llx] bus [%lx:%lx] flags %lx for "
- "BAR %d of %s\n", (unsigned long long)res->start,
- (unsigned long long)res->end,
+ pr_debug(" got res ["RANGE_PR"] bus ["RANGE_PR"] flags %lx for "
+ "BAR %d of %s\n", res->start, res->end,
region.start, region.end, res->flags, resno, pci_name(dev));
new = region.start | (res->flags & PCI_REGION_FLAG_MASK);
Index: linux-work/include/linux/pci.h
===================================================================
--- linux-work.orig/include/linux/pci.h 2007-12-05 11:55:49.000000000 +1100
+++ linux-work/include/linux/pci.h 2007-12-05 12:04:35.000000000 +1100
@@ -314,8 +314,8 @@ struct pci_raw_ops {
extern struct pci_raw_ops *raw_pci_ops;
struct pci_bus_region {
- unsigned long start;
- unsigned long end;
+ resource_size_t start;
+ resource_size_t end;
};
struct pci_dynids {
On Wed, 2007-12-05 at 17:40 +1100, Benjamin Herrenschmidt wrote:
> The current pci_assign_unassigned_resources() code doesn't work properly
> on 32 bits platforms with 64 bits resources. The main reason is the use
> of unsigned long in various places instead of resource_size_t.
>
> This fixes it, along with some tricks to avoid casting to 64 bits on
> platforms that don't need it in every printk around.
>
> This is a pre-requisite for making powerpc use the generic code instead of
> its own half-useful implementation.
>
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
> ---
>
> This version fixes some stupid warnings when using 32 bits resources
... and has warnings on 64 bits platforms... GRRRR
This whole issue of printk vs. resource_size_t is a terrible mess :-(
Part of the problem is that resource_size_t can be either u32 or u64..
that is it can be either unsigned int, unsigned long or unsigned long
long... and we have no way to reliably printk that.
Any clever idea before I start pushing filthy macros up linux/types.h ?
Cheers,
Ben.
On Thu, Dec 06, 2007 at 02:22:27PM +1100, Benjamin Herrenschmidt wrote:
>
> On Wed, 2007-12-05 at 17:40 +1100, Benjamin Herrenschmidt wrote:
> > The current pci_assign_unassigned_resources() code doesn't work properly
> > on 32 bits platforms with 64 bits resources. The main reason is the use
> > of unsigned long in various places instead of resource_size_t.
> >
> > This fixes it, along with some tricks to avoid casting to 64 bits on
> > platforms that don't need it in every printk around.
> >
> > This is a pre-requisite for making powerpc use the generic code instead of
> > its own half-useful implementation.
> >
> > Signed-off-by: Benjamin Herrenschmidt <[email protected]>
> > ---
> >
> > This version fixes some stupid warnings when using 32 bits resources
>
> ... and has warnings on 64 bits platforms... GRRRR
>
> This whole issue of printk vs. resource_size_t is a terrible mess :-(
>
> Part of the problem is that resource_size_t can be either u32 or u64..
>
> that is it can be either unsigned int, unsigned long or unsigned long
> long... and we have no way to reliably printk that.
We do this already just fine. Take a look in the kernel, I think we
just always cast it to long long to be uniform.
> Any clever idea before I start pushing filthy macros up linux/types.h ?
I don't think any macros are needed.
thanks,
greg k-h
On Wed, 2007-12-05 at 22:39 -0800, Greg KH wrote:
> > that is it can be either unsigned int, unsigned long or unsigned
> long
> > long... and we have no way to reliably printk that.
>
> We do this already just fine. Take a look in the kernel, I think we
> just always cast it to long long to be uniform.
I wanted to avoid that for two reasons:
- casts are fugly
- it adds support code to cast & handle 64 bits to 32 bits platforms
that wouldn't normally need it
Now, if you really think that's the way to go, I'll respin with casts
(I've used cast in subsequent patches merging bits & pieces of the
powerpc 32 and 64 bits PCI code too in fact).
I was just hoping somebody had a better idea, like a way to add a new
format specifier to printk without losing gcc type checking :-)
Cheers,
Ben.
On Thu, 6 Dec 2007, Benjamin Herrenschmidt wrote:
> On Wed, 2007-12-05 at 22:39 -0800, Greg KH wrote:
> > > that is it can be either unsigned int, unsigned long or unsigned
> > long
> > > long... and we have no way to reliably printk that.
> >
> > We do this already just fine. Take a look in the kernel, I think we
> > just always cast it to long long to be uniform.
>
> I wanted to avoid that for two reasons:
>
> - casts are fugly
> - it adds support code to cast & handle 64 bits to 32 bits platforms
> that wouldn't normally need it
Indeed. I still have a few places to fix it up in the Zorro bus code and
drivers. Everything is 32 bit, but allyesconfig sets CONFIG_RESOURCES_64BIT...
In some way I liked the recent suggestion to make CONFIG_RESOURCES_64BIT
depend on something...
> Now, if you really think that's the way to go, I'll respin with casts
> (I've used cast in subsequent patches merging bits & pieces of the
> powerpc 32 and 64 bits PCI code too in fact).
And casting to `unsigned long' for Zorro bus may sound fine, except what if
one day we'll need 64-bit resources for one platform?
> I was just hoping somebody had a better idea, like a way to add a new
> format specifier to printk without losing gcc type checking :-)
#define PRI* (...) ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Dec 06, 2007 at 06:58:54PM +1100, Benjamin Herrenschmidt wrote:
> I was just hoping somebody had a better idea, like a way to add a new
> format specifier to printk without losing gcc type checking :-)
It's been discussed before. Some of the solutions discussed:
- Add something like PRI_RES which can be concatenated into a printk.
Ugly.
- Patch gcc to allow user-definable types. I think OpenBSD has a patch
for this. Then we have to get that patch propagated to all the
people who compile the kernel. Unappetising.
- Disable gcc's printk checking, teach sparse to typecheck printk.
Most people don't run sparse yet.
--
Intel are signing my paycheques ... these opinions are still mine
"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."
On Dec 6, 2007, at 2:16 AM, Matthew Wilcox wrote:
> On Thu, Dec 06, 2007 at 06:58:54PM +1100, Benjamin Herrenschmidt
> wrote:
>> I was just hoping somebody had a better idea, like a way to add a new
>> format specifier to printk without losing gcc type checking :-)
>
> It's been discussed before. Some of the solutions discussed:
>
> - Add something like PRI_RES which can be concatenated into a printk.
> Ugly.
> - Patch gcc to allow user-definable types. I think OpenBSD has a
> patch
> for this. Then we have to get that patch propagated to all the
> people who compile the kernel. Unappetising.
> - Disable gcc's printk checking, teach sparse to typecheck printk.
> Most people don't run sparse yet.
How does gcc deal with glibc extension to allow people to add their
own specifiers?
http://www.gnu.org/software/libc/manual/html_node/Customizing-Printf.html
- k
Kumar Gala <[email protected]> writes:
> How does gcc deal with glibc extension to allow people to add their
> own specifiers?
It doesn't, gcc only knows about the standard specifiers. But it also
defines attributes that check other formats, like its own asm_fprintf
format specs.
Andreas.
--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstra?e 5, 90409 N?rnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
> It's been discussed before. Some of the solutions discussed:
>
> - Add something like PRI_RES which can be concatenated into a printk.
> Ugly.
> - Patch gcc to allow user-definable types. I think OpenBSD has a patch
> for this. Then we have to get that patch propagated to all the
> people who compile the kernel. Unappetising.
> - Disable gcc's printk checking, teach sparse to typecheck printk.
> Most people don't run sparse yet.
So you seem to dislike all 3, which one do you dislike the less ?
Cheers,
Ben.
On Thu, Dec 06, 2007 at 06:58:54PM +1100, Benjamin Herrenschmidt wrote:
>
> On Wed, 2007-12-05 at 22:39 -0800, Greg KH wrote:
> > > that is it can be either unsigned int, unsigned long or unsigned
> > long
> > > long... and we have no way to reliably printk that.
> >
> > We do this already just fine. Take a look in the kernel, I think we
> > just always cast it to long long to be uniform.
>
> I wanted to avoid that for two reasons:
>
> - casts are fugly
> - it adds support code to cast & handle 64 bits to 32 bits platforms
> that wouldn't normally need it
But that is how we already handle this today, in numerous places in the
kernel for this very type.
So, you can disagree that this is what we need to do, and if so, feel
free to fix up a whole lot of files in the tree :)
thanks,
greg k-h
On Thu, 2007-12-06 at 17:00 -0800, Greg KH wrote:
> But that is how we already handle this today, in numerous places in
> the
> kernel for this very type.
>
> So, you can disagree that this is what we need to do, and if so, feel
> free to fix up a whole lot of files in the tree :)
Heh, ok, allright, I'll respin with the casts.
Cheers,
Ben.