PCI: Try best to allocate pref mmio 64bit above 4g

When one of children resources does not support MEM_64, MEM_64 for bridge get reset, so pull down whole pref resource on the bridge under 4G.

If the bridge support pref mem 64, will only allocate that with pref mem64 to children that support it.
For children resources if they only support pref mem 32, will allocate them from non pref mem instead.

If the bridge only support 32bit pref mmio, will still have all children pref mmio under that.

Signed-off-by Yinghai Lu <[email protected]>

Tested-by Guo Chao <[email protected]>

Tested-by Wei Yang <[email protected]>

---
 drivers/pci/setup-bus.c |  138 ++++++++++++++++++++++++++++++++----------------
 drivers/pci/setup-res.c |   20 ++++++
 2 files changed, 111 insertions(+), 47 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================

drivers/pci/setup-bus.c +93 -45

@@ -713,12 +713,11 @@ static void pci_bridge_check_ranges(stru
    bus resource of a given type. Note: we intentionally skip
    the bus resources which have already been assigned (that is,
    have non-NULL parent resource). */
-static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type)
+static struct resource *find_free_bus_resource(struct pci_bus *bus,
+                        unsigned long type_mask, unsigned long type)
 {
        int i;
        struct resource *r;
-       unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-                                 IORESOURCE_PREFETCH;
 
        pci_bus_for_each_resource(bus, r, i) {
                if (r == &ioport_resource || r == &iomem_resource)

@@ -815,7 +814,8 @@ static void pbus_size_io(struct pci_bus
                resource_size_t add_size, struct list_head *realloc_head)
 {
        struct pci_dev *dev;
-       struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
+       struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
+                                                       IORESOURCE_IO);
        resource_size_t size = 0, size0 = 0, size1 = 0;
        resource_size_t children_add_size = 0;
        resource_size_t min_align, align;

@@ -915,15 +915,17 @@ static inline resource_size_t calculate_
  * guarantees that all child resources fit in this size.
  */
 static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
-                        unsigned long type, resource_size_t min_size,
-                       resource_size_t add_size,
-                       struct list_head *realloc_head)
+                        unsigned long type, unsigned long type2,
+                        unsigned long type3,
+                        resource_size_t min_size, resource_size_t add_size,
+                        struct list_head *realloc_head)
 {
        struct pci_dev *dev;
        resource_size_t min_align, align, size, size0, size1;
        resource_size_t aligns[12];     /* Alignments from 1Mb to 2Gb */
        int order, max_order;
-       struct resource *b_res = find_free_bus_resource(bus, type);
+       struct resource *b_res = find_free_bus_resource(bus,
+                                        mask | IORESOURCE_PREFETCH, type);
        unsigned int mem64_mask = 0;
        resource_size_t children_add_size = 0;
 

@@ -944,7 +946,9 @@ static int pbus_size_mem(struct pci_bus
                        struct resource *r = &dev->resource[i];
                        resource_size_t r_size;
 
-                       if (r->parent || (r->flags & mask) != type)
+                       if (r->parent || ((r->flags & mask) != type &&
+                                         (r->flags & mask) != type2 &&
+                                         (r->flags & mask) != type3))
                                continue;
                        r_size = resource_size(r);
 #ifdef CONFIG_PCI_IOV

@@ -1117,8 +1121,9 @@ void __ref __pci_bus_size_bridges(struct
                        struct list_head *realloc_head)
 {
        struct pci_dev *dev;
-       unsigned long mask, prefmask;
+       unsigned long mask, prefmask, type2 = 0, type3 = 0;
        resource_size_t additional_mem_size = 0, additional_io_size = 0;
+       struct resource *b_res;
 
        list_for_each_entry(dev, &bus->devices, bus_list) {
                struct pci_bus *b = dev->subordinate;

@@ -1163,15 +1168,34 @@ void __ref __pci_bus_size_bridges(struct
                   has already been allocated by arch code, try
                   non-prefetchable range for both types of PCI memory
                   resources. */
+               b_res = &bus->self->resource[PCI_BRIDGE_RESOURCES];
                mask = IORESOURCE_MEM;
                prefmask = IORESOURCE_MEM | IORESOURCE_PREFETCH;
-               if (pbus_size_mem(bus, prefmask, prefmask,
+               if (b_res[2].flags & IORESOURCE_MEM_64) {
+                       prefmask |= IORESOURCE_MEM_64;
+                       if (pbus_size_mem(bus, prefmask, prefmask,
+                                 prefmask, prefmask,
                                  realloc_head ? 0 : additional_mem_size,
-                                 additional_mem_size, realloc_head))
-                       mask = prefmask; /* Success, size non-prefetch only. */
-               else
-                       additional_mem_size += additional_mem_size;
-               pbus_size_mem(bus, mask, IORESOURCE_MEM,
+                                 additional_mem_size, realloc_head)) {
+                                       /* Success, size non-pref64 only. */
+                                       mask = prefmask;
+                                       type2 = prefmask & ~IORESOURCE_MEM_64;
+                                       type3 = prefmask & ~IORESOURCE_PREFETCH;
+                       }
+               }
+               if (!type2) {
+                       prefmask &= ~IORESOURCE_MEM_64;
+                       if (pbus_size_mem(bus, prefmask, prefmask,
+                                        prefmask, prefmask,
+                                        realloc_head ? 0 : additional_mem_size,
+                                        additional_mem_size, realloc_head)) {
+                               /* Success, size non-prefetch only. */
+                               mask = prefmask;
+                       } else
+                               additional_mem_size += additional_mem_size;
+                       type2 = type3 = IORESOURCE_MEM;
+               }
+               pbus_size_mem(bus, mask, IORESOURCE_MEM, type2, type3,
                                realloc_head ? 0 : additional_mem_size,
                                additional_mem_size, realloc_head);
                break;

@@ -1257,42 +1281,66 @@ static void __ref __pci_bridge_assign_re
 static void pci_bridge_release_resources(struct pci_bus *bus,
                                          unsigned long type)
 {
-       int idx;
-       bool changed = false;
-       struct pci_dev *dev;
+       struct pci_dev *dev = bus->self;
        struct resource *r;
        unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-                                 IORESOURCE_PREFETCH;
+                                 IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
+       unsigned old_flags = 0;
+       struct resource *b_res;
+       int idx = 1;
 
-       dev = bus->self;
-       for (idx = PCI_BRIDGE_RESOURCES; idx <= PCI_BRIDGE_RESOURCE_END;
-            idx++) {
-               r = &dev->resource[idx];
-               if ((r->flags & type_mask) != type)
-                       continue;
-               if (!r->parent)
-                       continue;
-               /*
-                * if there are children under that, we should release them
-                *  all
-                */
-               release_child_resources(r);
-               if (!release_resource(r)) {
-                       dev_printk(KERN_DEBUG, &dev->dev,
-                                "resource %d %pR released\n", idx, r);
-                       /* keep the old size */
-                       r->end = resource_size(r) - 1;
-                       r->start = 0;
-                       r->flags = 0;
-                       changed = true;
-               }
-       }
+       b_res = &dev->resource[PCI_BRIDGE_RESOURCES];
+
+       /*
+        *     1. if there is io port assign fail, will release bridge
+        *        io port.
+        *     2. if there is non pref mmio assign fail, release bridge
+        *        nonpref mmio.
+        *     3. if there is 64bit pref mmio assign fail, and bridge pref
+        *        is 64bit, release bridge pref mmio.
+        *     4. if there is pref mmio assign fail, and bridge pref is
+        *        32bit mmio, release bridge pref mmio
+        *     5. if there is pref mmio assign fail, and bridge pref is not
+        *        assigned, release bridge nonpref mmio.
+        */
+       if (type & IORESOURCE_IO)
+               idx = 0;
+       else if (!(type & IORESOURCE_PREFETCH))
+               idx = 1;
+       else if ((type & IORESOURCE_MEM_64) &&
+                (b_res[2].flags & IORESOURCE_MEM_64))
+               idx = 2;
+       else if (!(b_res[2].flags & IORESOURCE_MEM_64) &&
+                (b_res[2].flags & IORESOURCE_PREFETCH))
+               idx = 2;
+       else
+               idx = 1;
+
+       r = &b_res[idx];
+
+       if (!r->parent)
+               return;
+
+       /*
+        * if there are children under that, we should release them
+        *  all
+        */
+       release_child_resources(r);
+       if (!release_resource(r)) {
+               type = old_flags = r->flags & type_mask;
+               dev_printk(KERN_DEBUG, &dev->dev, "resource %d %pR released\n",
+                                       PCI_BRIDGE_RESOURCES + idx, r);
+               /* keep the old size */
+               r->end = resource_size(r) - 1;
+               r->start = 0;
+               r->flags = 0;
 
-       if (changed) {
                /* avoiding touch the one without PREF */
                if (type & IORESOURCE_PREFETCH)
                        type = IORESOURCE_PREFETCH;
                __pci_setup_bridge(bus, type);
+               /* for next child res under same bridge */
+               r->flags = old_flags;
        }
 }
 

@@ -1471,7 +1519,7 @@ void pci_assign_unassigned_root_bus_reso
        LIST_HEAD(fail_head);
        struct pci_dev_resource *fail_res;
        unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-                                 IORESOURCE_PREFETCH;
+                                 IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
        int pci_try_num = 1;
        enum enable_type enable_local;
 

Index: linux-2.6/drivers/pci/setup-res.c
===================================================================

drivers/pci/setup-res.c +18 -2

@@ -208,15 +208,31 @@ static int __pci_assign_resource(struct
 
        /* First, try exact prefetching match.. */
        ret = pci_bus_alloc_resource(bus, res, size, align, min,
-                                    IORESOURCE_PREFETCH,
+                                    IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
                                     pcibios_align_resource, dev);
 
-       if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
+       if (ret < 0 &&
+           (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) ==
+            (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
+               /*
+                * That failed.
+                *
+                * Try below 4g pref
+                */
+               ret = pci_bus_alloc_resource(bus, res, size, align, min,
+                                            IORESOURCE_PREFETCH,
+                                            pcibios_align_resource, dev);
+       }
+
+       if (ret < 0 &&
+           (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))) {
                /*
                 * That failed.
                 *
                 * But a prefetching area can handle a non-prefetching
                 * window (it will just not perform as well).
+                *
+                * Also can put 64bit under 32bit range. (below 4g).
                 */
                ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
                                             pcibios_align_resource, dev);

Quoted 11 lines · Yinghai Lu

When one of children resources does not support MEM_64, MEM_64 for bridge get reset, so pull down whole pref resource on the bridge under 4G.

If the bridge support pref mem 64, will only allocate that with pref mem64 to children that support it.
For children resources if they only support pref mem 32, will allocate them from non pref mem instead.

If the bridge only support 32bit pref mmio, will still have all children pref mmio under that.

Review

Quoted 8 lines

(quoted diff) +0 -4

-v2: Add release bridge res support with bridge mem res for pref_mem children res.
-v3: refresh and make it can be applied early before for_each_dev_res patchset.
-v4: fix non-pref mmio 64bit support found by Guo Chao.
-v7: repost as ibm's powerpc system work again when
    1. petiboot boot kernel have this patch.
    2. or mellanox driver added new .shutdown member.
    discussion could be found at:
         http://marc.info/?t=138968064500001&r=1&w=2

I think this fixes some sort of bug, and I suppose if I spent a few hours decoding the discussion you mention (the 44 message-long "mlx4_core probe error after applying Yinghai's patch" discussion), I might be able to figure out what it is.

But maybe somebody who already knows what the bug is can summarize it for me, and maybe even open a kernel.org bugzilla with a dmesg log as an example.

I do intend to use this message [1] as a source to help construct a changelog, but I'd like to also describe a specific problem that is solved by this patch. Ideally this would already be in the changelog, but it's not, so any help in figuring it out would be appreciated.

[1] http://lkml.kernel.org/r/CAErSpo5VWREGptW0MU0wLJUa3h23DXMZPGkdJFMTx5WAGL8J6Q@mail.gmail.com

Quoted 265 lines

Signed-off-by Yinghai Lu <[email protected]>

Tested-by Guo Chao <[email protected]>

Tested-by Wei Yang <[email protected]>

---
 drivers/pci/setup-bus.c |  138 ++++++++++++++++++++++++++++++++----------------
 drivers/pci/setup-res.c |   20 ++++++
 2 files changed, 111 insertions(+), 47 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================

drivers/pci/setup-bus.c +93 -45

@@ -713,12 +713,11 @@ static void pci_bridge_check_ranges(stru
    bus resource of a given type. Note: we intentionally skip
    the bus resources which have already been assigned (that is,
    have non-NULL parent resource). */
-static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type)
+static struct resource *find_free_bus_resource(struct pci_bus *bus,
+                        unsigned long type_mask, unsigned long type)
 {
        int i;
        struct resource *r;
-       unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-                                 IORESOURCE_PREFETCH;
 
        pci_bus_for_each_resource(bus, r, i) {
                if (r == &ioport_resource || r == &iomem_resource)

@@ -815,7 +814,8 @@ static void pbus_size_io(struct pci_bus
                resource_size_t add_size, struct list_head *realloc_head)
 {
        struct pci_dev *dev;
-       struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
+       struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
+                                                       IORESOURCE_IO);
        resource_size_t size = 0, size0 = 0, size1 = 0;
        resource_size_t children_add_size = 0;
        resource_size_t min_align, align;

@@ -915,15 +915,17 @@ static inline resource_size_t calculate_
  * guarantees that all child resources fit in this size.
  */
 static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
-                        unsigned long type, resource_size_t min_size,
-                       resource_size_t add_size,
-                       struct list_head *realloc_head)
+                        unsigned long type, unsigned long type2,
+                        unsigned long type3,
+                        resource_size_t min_size, resource_size_t add_size,
+                        struct list_head *realloc_head)
 {
        struct pci_dev *dev;
        resource_size_t min_align, align, size, size0, size1;
        resource_size_t aligns[12];     /* Alignments from 1Mb to 2Gb */
        int order, max_order;
-       struct resource *b_res = find_free_bus_resource(bus, type);
+       struct resource *b_res = find_free_bus_resource(bus,
+                                        mask | IORESOURCE_PREFETCH, type);
        unsigned int mem64_mask = 0;
        resource_size_t children_add_size = 0;
 

@@ -944,7 +946,9 @@ static int pbus_size_mem(struct pci_bus
                        struct resource *r = &dev->resource[i];
                        resource_size_t r_size;
 
-                       if (r->parent || (r->flags & mask) != type)
+                       if (r->parent || ((r->flags & mask) != type &&
+                                         (r->flags & mask) != type2 &&
+                                         (r->flags & mask) != type3))
                                continue;
                        r_size = resource_size(r);
 #ifdef CONFIG_PCI_IOV

@@ -1117,8 +1121,9 @@ void __ref __pci_bus_size_bridges(struct
                        struct list_head *realloc_head)
 {
        struct pci_dev *dev;
-       unsigned long mask, prefmask;
+       unsigned long mask, prefmask, type2 = 0, type3 = 0;
        resource_size_t additional_mem_size = 0, additional_io_size = 0;
+       struct resource *b_res;
 
        list_for_each_entry(dev, &bus->devices, bus_list) {
                struct pci_bus *b = dev->subordinate;

@@ -1163,15 +1168,34 @@ void __ref __pci_bus_size_bridges(struct
                   has already been allocated by arch code, try
                   non-prefetchable range for both types of PCI memory
                   resources. */
+               b_res = &bus->self->resource[PCI_BRIDGE_RESOURCES];
                mask = IORESOURCE_MEM;
                prefmask = IORESOURCE_MEM | IORESOURCE_PREFETCH;
-               if (pbus_size_mem(bus, prefmask, prefmask,
+               if (b_res[2].flags & IORESOURCE_MEM_64) {
+                       prefmask |= IORESOURCE_MEM_64;
+                       if (pbus_size_mem(bus, prefmask, prefmask,
+                                 prefmask, prefmask,
                                  realloc_head ? 0 : additional_mem_size,
-                                 additional_mem_size, realloc_head))
-                       mask = prefmask; /* Success, size non-prefetch only. */
-               else
-                       additional_mem_size += additional_mem_size;
-               pbus_size_mem(bus, mask, IORESOURCE_MEM,
+                                 additional_mem_size, realloc_head)) {
+                                       /* Success, size non-pref64 only. */
+                                       mask = prefmask;
+                                       type2 = prefmask & ~IORESOURCE_MEM_64;
+                                       type3 = prefmask & ~IORESOURCE_PREFETCH;
+                       }
+               }
+               if (!type2) {
+                       prefmask &= ~IORESOURCE_MEM_64;
+                       if (pbus_size_mem(bus, prefmask, prefmask,
+                                        prefmask, prefmask,
+                                        realloc_head ? 0 : additional_mem_size,
+                                        additional_mem_size, realloc_head)) {
+                               /* Success, size non-prefetch only. */
+                               mask = prefmask;
+                       } else
+                               additional_mem_size += additional_mem_size;
+                       type2 = type3 = IORESOURCE_MEM;
+               }
+               pbus_size_mem(bus, mask, IORESOURCE_MEM, type2, type3,
                                realloc_head ? 0 : additional_mem_size,
                                additional_mem_size, realloc_head);
                break;

@@ -1257,42 +1281,66 @@ static void __ref __pci_bridge_assign_re
 static void pci_bridge_release_resources(struct pci_bus *bus,
                                          unsigned long type)
 {
-       int idx;
-       bool changed = false;
-       struct pci_dev *dev;
+       struct pci_dev *dev = bus->self;
        struct resource *r;
        unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-                                 IORESOURCE_PREFETCH;
+                                 IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
+       unsigned old_flags = 0;
+       struct resource *b_res;
+       int idx = 1;
 
-       dev = bus->self;
-       for (idx = PCI_BRIDGE_RESOURCES; idx <= PCI_BRIDGE_RESOURCE_END;
-            idx++) {
-               r = &dev->resource[idx];
-               if ((r->flags & type_mask) != type)
-                       continue;
-               if (!r->parent)
-                       continue;
-               /*
-                * if there are children under that, we should release them
-                *  all
-                */
-               release_child_resources(r);
-               if (!release_resource(r)) {
-                       dev_printk(KERN_DEBUG, &dev->dev,
-                                "resource %d %pR released\n", idx, r);
-                       /* keep the old size */
-                       r->end = resource_size(r) - 1;
-                       r->start = 0;
-                       r->flags = 0;
-                       changed = true;
-               }
-       }
+       b_res = &dev->resource[PCI_BRIDGE_RESOURCES];
+
+       /*
+        *     1. if there is io port assign fail, will release bridge
+        *        io port.
+        *     2. if there is non pref mmio assign fail, release bridge
+        *        nonpref mmio.
+        *     3. if there is 64bit pref mmio assign fail, and bridge pref
+        *        is 64bit, release bridge pref mmio.
+        *     4. if there is pref mmio assign fail, and bridge pref is
+        *        32bit mmio, release bridge pref mmio
+        *     5. if there is pref mmio assign fail, and bridge pref is not
+        *        assigned, release bridge nonpref mmio.
+        */
+       if (type & IORESOURCE_IO)
+               idx = 0;
+       else if (!(type & IORESOURCE_PREFETCH))
+               idx = 1;
+       else if ((type & IORESOURCE_MEM_64) &&
+                (b_res[2].flags & IORESOURCE_MEM_64))
+               idx = 2;
+       else if (!(b_res[2].flags & IORESOURCE_MEM_64) &&
+                (b_res[2].flags & IORESOURCE_PREFETCH))
+               idx = 2;
+       else
+               idx = 1;
+
+       r = &b_res[idx];
+
+       if (!r->parent)
+               return;
+
+       /*
+        * if there are children under that, we should release them
+        *  all
+        */
+       release_child_resources(r);
+       if (!release_resource(r)) {
+               type = old_flags = r->flags & type_mask;
+               dev_printk(KERN_DEBUG, &dev->dev, "resource %d %pR released\n",
+                                       PCI_BRIDGE_RESOURCES + idx, r);
+               /* keep the old size */
+               r->end = resource_size(r) - 1;
+               r->start = 0;
+               r->flags = 0;
 
-       if (changed) {
                /* avoiding touch the one without PREF */
                if (type & IORESOURCE_PREFETCH)
                        type = IORESOURCE_PREFETCH;
                __pci_setup_bridge(bus, type);
+               /* for next child res under same bridge */
+               r->flags = old_flags;
        }
 }
 

@@ -1471,7 +1519,7 @@ void pci_assign_unassigned_root_bus_reso
        LIST_HEAD(fail_head);
        struct pci_dev_resource *fail_res;
        unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-                                 IORESOURCE_PREFETCH;
+                                 IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
        int pci_try_num = 1;
        enum enable_type enable_local;
 

Index: linux-2.6/drivers/pci/setup-res.c
===================================================================

drivers/pci/setup-res.c +18 -2

@@ -208,15 +208,31 @@ static int __pci_assign_resource(struct
 
        /* First, try exact prefetching match.. */
        ret = pci_bus_alloc_resource(bus, res, size, align, min,
-                                    IORESOURCE_PREFETCH,
+                                    IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
                                     pcibios_align_resource, dev);
 
-       if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
+       if (ret < 0 &&
+           (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) ==
+            (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
+               /*
+                * That failed.
+                *
+                * Try below 4g pref
+                */
+               ret = pci_bus_alloc_resource(bus, res, size, align, min,
+                                            IORESOURCE_PREFETCH,
+                                            pcibios_align_resource, dev);
+       }
+
+       if (ret < 0 &&
+           (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))) {
                /*
                 * That failed.
                 *
                 * But a prefetching area can handle a non-prefetching
                 * window (it will just not perform as well).
+                *
+                * Also can put 64bit under 32bit range. (below 4g).
                 */
                ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
                                             pcibios_align_resource, dev);

Quoted 12 lines · Bjorn Helgaas

Quoted 11 lines · Yinghai Lu

When one of children resources does not support MEM_64, MEM_64 for bridge get reset, so pull down whole pref resource on the bridge under 4G.

If the bridge support pref mem 64, will only allocate that with pref mem64 to children that support it.
For children resources if they only support pref mem 32, will allocate them from non pref mem instead.

If the bridge only support 32bit pref mmio, will still have all children pref mmio under that.

Quoted 13 lines

Review

Quoted 8 lines

(quoted diff) +0 -4

-v2: Add release bridge res support with bridge mem res for pref_mem children res.
-v3: refresh and make it can be applied early before for_each_dev_res patchset.
-v4: fix non-pref mmio 64bit support found by Guo Chao.
-v7: repost as ibm's powerpc system work again when
     1. petiboot boot kernel have this patch.
     2. or mellanox driver added new .shutdown member.
     discussion could be found at:
          http://marc.info/?t=138968064500001&r=1&w=2

I think this fixes some sort of bug, and I suppose if I spent a few hours decoding the discussion you mention (the 44 message-long "mlx4_core probe error after applying Yinghai's patch" discussion), I might be able to figure out what it is.

The result of 44 message-long discussion is: the Mellanox card's failure is due to a bug in its driver instead of this patch.

The patch refines the logic of using prefetchable window, so that 64-bit prefetchable BARs have a chance to be really prefetchable. It does not fix any bugs, instead, it exposes one.

Quoted 26 lines · Guo Chao

Quoted 12 lines · Bjorn Helgaas

Quoted 11 lines · Yinghai Lu

When one of children resources does not support MEM_64, MEM_64 for bridge get reset, so pull down whole pref resource on the bridge under 4G.

If the bridge support pref mem 64, will only allocate that with pref mem64 to children that support it.
For children resources if they only support pref mem 32, will allocate them from non pref mem instead.

If the bridge only support 32bit pref mmio, will still have all children pref mmio under that.

Quoted 13 lines

Review

Quoted 8 lines

(quoted diff) +0 -4

-v2: Add release bridge res support with bridge mem res for pref_mem children res.
-v3: refresh and make it can be applied early before for_each_dev_res patchset.
-v4: fix non-pref mmio 64bit support found by Guo Chao.
-v7: repost as ibm's powerpc system work again when
    1. petiboot boot kernel have this patch.
    2. or mellanox driver added new .shutdown member.
    discussion could be found at:
         http://marc.info/?t=138968064500001&r=1&w=2

I think this fixes some sort of bug, and I suppose if I spent a few hours decoding the discussion you mention (the 44 message-long "mlx4_core probe error after applying Yinghai's patch" discussion), I might be able to figure out what it is.

Quoted 2 lines

The result of 44 message-long discussion is: the Mellanox card's failure is due to a bug in its driver instead of this patch.

So just to make sure we're on the same page -- fixing the bug is carried out through netdev, e.g this 3.14-rc8
upstream commit 97a5221 "net/mlx4_core: pass pci_device_id.driver_data to __mlx4_init_one during reset" and now this one more fix which is under discussion
http://marc.info/?l=linux-pci&m=139675010015646&w=2, right?

Quoted 7 lines

The patch refines the logic of using prefetchable window, so that 64-bit prefetchable BARs have a chance to be really prefetchable. It does not fix any bugs, instead, it exposes one.

Quoted 6 lines

Quoted 6 lines

So just to make sure we're on the same page -- fixing the bug is carried out through netdev, e.g this 3.14-rc8
upstream commit 97a5221 "net/mlx4_core: pass pci_device_id.driver_data to __mlx4_init_one during reset" and now this one more fix which is under discussion
http://marc.info/?l=linux-pci&m=139675010015646&w=2, right?

No, this onehttp://marc.info/?l=linux-pci&m=139675010015646&w=2 is another bug in mlx4 driver, missing a proper pci_dev_data in mlx4_init_one.

The bug discussing in this thread is the driver "forget" to release the "owner" during kexec. So this device couldn't be usded after kexec.

Quoted 6 lines · Wei Yang

Quoted 6 lines

Quoted 6 lines

So just to make sure we're on the same page -- fixing the bug is carried out through netdev, e.g this 3.14-rc8
upstream commit 97a5221 "net/mlx4_core: pass pci_device_id.driver_data to __mlx4_init_one during reset" and now this one more fix which is under discussion
http://marc.info/?l=linux-pci&m=139675010015646&w=2, right?

Quoted 6 lines

No, this onehttp://marc.info/?l=linux-pci&m=139675010015646&w=2 is another bug in mlx4 driver, missing a proper pci_dev_data in mlx4_init_one.

The bug discussing in this thread is the driver "forget" to release the "owner" during kexec. So this device couldn't be usded after kexec.

Can you guys please open a kernel.org bugzilla ticket, or better... work on a patch?

Quoted 12 lines · Bjorn Helgaas

Quoted 11 lines · Yinghai Lu

When one of children resources does not support MEM_64, MEM_64 for bridge get reset, so pull down whole pref resource on the bridge under 4G.

If the bridge support pref mem 64, will only allocate that with pref mem64 to children that support it.
For children resources if they only support pref mem 32, will allocate them from non pref mem instead.

If the bridge only support 32bit pref mmio, will still have all children pref mmio under that.

Quoted 14 lines

Review

Quoted 8 lines

(quoted diff) +0 -4

-v2: Add release bridge res support with bridge mem res for pref_mem children res.
-v3: refresh and make it can be applied early before for_each_dev_res patchset.
-v4: fix non-pref mmio 64bit support found by Guo Chao.
-v7: repost as ibm's powerpc system work again when
    1. petiboot boot kernel have this patch.
    2. or mellanox driver added new .shutdown member.
    discussion could be found at:
         http://marc.info/?t=138968064500001&r=1&w=2

I think this fixes some sort of bug, and I suppose if I spent a few hours decoding the discussion you mention (the 44 message-long "mlx4_core probe error after applying Yinghai's patch" discussion), I might be able to figure out what it is.

The result of 44 message-long discussion is: the Mellanox card's failure is due to a bug in its driver instead of this patch.

The patch refines the logic of using prefetchable window, so that 64-bit prefetchable BARs have a chance to be really prefetchable. It does not fix any bugs, instead, it exposes one.

Quoted 28 lines · Guo Chao

Quoted 12 lines · Bjorn Helgaas

Quoted 11 lines · Yinghai Lu

When one of children resources does not support MEM_64, MEM_64 for bridge get reset, so pull down whole pref resource on the bridge under 4G.

If the bridge support pref mem 64, will only allocate that with pref mem64 to children that support it.
For children resources if they only support pref mem 32, will allocate them from non pref mem instead.

If the bridge only support 32bit pref mmio, will still have all children pref mmio under that.

Quoted 14 lines

Review

Quoted 8 lines

(quoted diff) +0 -4

-v2: Add release bridge res support with bridge mem res for pref_mem children res.
-v3: refresh and make it can be applied early before for_each_dev_res patchset.
-v4: fix non-pref mmio 64bit support found by Guo Chao.
-v7: repost as ibm's powerpc system work again when
    1. petiboot boot kernel have this patch.
    2. or mellanox driver added new .shutdown member.
    discussion could be found at:
         http://marc.info/?t=138968064500001&r=1&w=2

I think this fixes some sort of bug, and I suppose if I spent a few hours decoding the discussion you mention (the 44 message-long "mlx4_core probe error after applying Yinghai's patch" discussion), I might be able to figure out what it is.

Quoted 6 lines

The result of 44 message-long discussion is: the Mellanox card's failure is due to a bug in its driver instead of this patch.

The patch refines the logic of using prefetchable window, so that 64-bit prefetchable BARs have a chance to be really prefetchable. It does not fix any bugs, instead, it exposes one.

OK, then I'm back to square one, and I need an explanation of why we want to merge this patch.

I think the patch conserves 32-bit address space by putting more things above 4G than we used to. But this comes at the cost of using non-prefetchable windows in some cases where we used to use prefetchable ones.

Somebody has to explain why we want to do this and why the benefit of conserving the 32-bit space is more important losing the prefetchability.

Bjorn

Quoted 11 lines

OK, then I'm back to square one, and I need an explanation of why we want to merge this patch.

I think the patch conserves 32-bit address space by putting more things above 4G than we used to. But this comes at the cost of using non-prefetchable windows in some cases where we used to use prefetchable ones.

Somebody has to explain why we want to do this and why the benefit of conserving the 32-bit space is more important losing the prefetchability.

I want to make sure we are talking about this patch:
PCI: Try best to allocate pref mmio 64bit above 4g
http://patchwork.ozlabs.org/patch/328067/

instead of this one:
PCI: Try to allocate mem64 above 4G at first
http://patchwork.ozlabs.org/patch/303197/

This patch does not intend to conserve 32-bit space at all. It makes better use of prefetchable window.

The problem with the old code is: when both 32-bit and 64-bit prefetchable BARs present, it's in favor of 32-bit one to use prefetchable window. This window is then not supposed to get 4G-above address, and this 32-bit-address-only property would propagates upwards until reaching root bridge. In later assignment phase, 4G-above space is never touched. This is just caused by a single 32-bit prefetchable BAR (say a ROM BAR).

This patch helps by making better decision:

* Keep the old behaviour if only 32-bit or 64-bit prefetchable BARs present

* If both of them present, put 64-bit ones to prefetchable window, 32-bit ones to non-prefetchable window

So 4G-above space can be properly used.

Why does enabling 64-bit space matter?

* 32-bit space is just too small. If larger-than-4G BAR sounds unrealistic (we do have such devices), there is still a chance that total MMIO size under a domain is too large, especially when SR-IOV enabled (we met this situation).

Quoted 12 lines

The problem with the old code is: when both 32-bit and 64-bit prefetchable BARs present, it's in favor of 32-bit one to use prefetchable window. This window is then not supposed to get 4G-above address, and this 32-bit-address-only property would propagates upwards until reaching root bridge. In later assignment phase, 4G-above space is never touched. This is just caused by a single 32-bit prefetchable BAR (say a ROM BAR).

This patch helps by making better decision:

* Keep the old behaviour if only 32-bit or 64-bit prefetchable BARs present

Quoted 2 lines

* If both of them present, put 64-bit ones to prefetchable window, 32-bit ones to non-prefetchable window

I thought the patch was supposed to change the way we allocate bridge windows. But you are talking about the way we assign 32- and 64-bit device resources, i.e., changing the way we decide whether to put them in prefetchable or non-prefetchable windows.

Quoted 8 lines

So 4G-above space can be properly used.

Why does enabling 64-bit space matter?

* 32-bit space is just too small. If larger-than-4G BAR sounds unrealistic (we do have such devices), there is still a chance that total MMIO size under a domain is too large, especially when SR-IOV enabled (we met this situation).

Please give more details about the problem you saw. A complete dmesg log showing a boot failure or a device that doesn't work, and another log with this patch applied, showing things working as desired, would go a long ways toward clarifying this.

This sounds like a specific case that will be fixed by the patch, and if you give more details, maybe I can figure out what's going on.

Quoted 8 lines · Guo Chao

So 4G-above space can be properly used.

Why does enabling 64-bit space matter?

* 32-bit space is just too small. If larger-than-4G BAR sounds unrealistic (we do have such devices), there is still a chance that total MMIO size under a domain is too large, especially when SR-IOV enabled (we met this situation).

Please give more details about the problem you saw. A complete dmesg log showing a boot failure or a device that doesn't work, and another log with this patch applied, showing things working as desired, would go a long ways toward clarifying this.

This sounds like a specific case that will be fixed by the patch, and if you give more details, maybe I can figure out what's going on.

Bjorn

Quoted 15 lines · Bjorn Helgaas

Quoted 8 lines · Guo Chao

So 4G-above space can be properly used.

Why does enabling 64-bit space matter?

* 32-bit space is just too small. If larger-than-4G BAR sounds unrealistic (we do have such devices), there is still a chance that total MMIO size under a domain is too large, especially when SR-IOV enabled (we met this situation).

Please give more details about the problem you saw. A complete dmesg log showing a boot failure or a device that doesn't work, and another log with this patch applied, showing things working as desired, would go a long ways toward clarifying this.

Quoted 3 lines

This sounds like a specific case that will be fixed by the patch, and if you give more details, maybe I can figure out what's going on.

Let's see an example.

| pci 0003:05:00.0: reg 0x10: [mem 0x3d05801000000-0x3d058010fffff 64bit]
| pci 0003:05:00.0: reg 0x18: [mem 0x3d05010000000-0x3d05017ffffff 64bit pref]
| pci 0003:05:00.0: reg 0x30: [mem 0x00000000-0x000fffff pref]
| pci 0003:05:00.0: reg 0x134: [mem 0x3d05018000000-0x3d0501fffffff 64bit pref]

This is printed at enumeration phase. This device has a SRIOV BAR with size of 0x7ffffff (128M). That's the size of a signle VF BAR. The device supports 63 VFs so we need near 8G space in total. Apparanlty we need exploit 64-bit space.

| PCI host bridge to bus 0003:00
| pci_bus 0003:00: root bus resource [mem 0x3d05800000000-0x3d0587ffeffff] (bus address [0x80000000-0xfffeffff])
| pci_bus 0003:00: root bus resource [mem 0x3d05008000000-0x3d057ffffffff 64bit pref]

And we do have a huge (32G) 64-bit prefetchable window supply. We expect everything to work fine, but:

| pci 0003:00:00.0: BAR 15: can't assign mem pref (size 0x206000000)
| pci 0003:00:00.0: BAR 14: assigned [mem 0x3d05800000000-0x3d05802ffffff]
| pci 0003:00:00.0: BAR 13: can't assign io (size 0x4000)

It went wrong at the beginning. Note the error message never considers 64-bit or not, but BAR 15 here has it MEM_64 flag cleared.

It first
tried to find a 32-bit prefetchable window, but we only supply a 64-bit one. So it fall back to (32-bit) non-prefetchable window, but there is no enough room there. At last it went into complicated steps (not show here) of allocating requested resource first, then try best for the optional ones, etc..

Why is BAR 15 (prefetchable) 32 bit instead of 64? Because PCI core favours 32-bit prefetchable BARs and we have some. This is one of them:

| pci 0003:05:00.0: reg 0x30: [mem 0x00000000-0x000fffff pref]

PCI core decides to let them enjoy the benefition of prefetch. They can't bear the risk of getting 4G-above address, so its parent, its parent's parent, its parent's parent's parent, finally the root bridge (00:00.0) must have their MEM_64 flag of prefetchable resource (BAR 15) clear.

In the end nobody
is eligible to use the 64-bit (prefetchable) space even we have huge supply !

Note even the resource is small and successfully fall back into 32-bit non-prefetchable window, that's still not OK for us because we need SRIOV resource be at 64-bit prefetchable space to do platform configuration.

With Yinghai's patch, when 64-bit prefetchable BARs found, they're more favoured than the 32-bit prefetchable ones (if any), so all upstream bridges' prefetchable windows have their MEM_64 flag reserved and the huge 64-bit prefetchable space will be exploited:

| pci 0003:00:00.0: BAR 15: assigned [mem 0x3d05008000000-0x3d0521fffffff 64bit pref]
| pci 0003:00:00.0: BAR 14: assigned [mem 0x3d05800000000-0x3d05802ffffff]
| pci 0003:00:00.0: BAR 13: can't assign io (size 0x4000)

(The IO resource error here is due to we do not provide IO window)

Quoted 2 lines

It went wrong at the beginning. Note the error message never considers 64-bit or not, but BAR 15 here has it MEM_64 flag cleared.

BAR 15 is a bridge window. I think its resource flags should reflect the capability of the *window*, even if we disable the window or we happen to assign addresses that are under 4GB. So I think it's wrong that we clear the MEM_64 flag in pbus_size_mem() and the IO flag in pbus_size_io().

Quoted 8 lines · Bjorn Helgaas

Quoted 2 lines

It went wrong at the beginning. Note the error message never considers 64-bit or not, but BAR 15 here has it MEM_64 flag cleared.

BAR 15 is a bridge window. I think its resource flags should reflect the capability of the *window*, even if we disable the window or we happen to assign addresses that are under 4GB. So I think it's wrong that we clear the MEM_64 flag in pbus_size_mem() and the IO flag in pbus_size_io().

We keep PCI_PREF_RANGE_TYPE_64 in the flag.

We should check that before we touch PCI_PREF_LIMIT_UPPER32 BAR in pci_setup_bridge_mmio_pref()

like following:

Subject: [PATCH] PCI: only touch upper 32bit for pref64bit bridge bar

If the bridge pref mmio bar does not support 64bit, we should touch upper 32bit.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/setup-bus.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================

drivers/pci/setup-bus.c +8 -6

@@ -592,23 +592,23 @@ static void pci_setup_bridge_mmio(struct
 static void pci_setup_bridge_mmio_pref(struct pci_bus *bus)
 {
     struct pci_dev *bridge = bus->self;
-    struct resource *res;
+    struct resource *res = bus->resource[2];
     struct pci_bus_region region;
     u32 l, bu, lu;
 
     /* Clear out the upper 32 bits of PREF limit.
        If PCI_PREF_BASE_UPPER32 was non-zero, this temporarily
        disables PREF range, which is ok. */
-    pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);
+    if (res->flags & PCI_PREF_RANGE_TYPE_64)
+        pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);
 
     /* Set up PREF base/limit. */
     bu = lu = 0;
-    res = bus->resource[2];
     pcibios_resource_to_bus(bridge->bus, &region, res);
     if (res->flags & IORESOURCE_PREFETCH) {
         l = (region.start >> 16) & 0xfff0;
         l |= region.end & 0xfff00000;
-        if (res->flags & IORESOURCE_MEM_64) {
+        if (res->flags & PCI_PREF_RANGE_TYPE_64) {
             bu = upper_32_bits(region.start);
             lu = upper_32_bits(region.end);
         }

@@ -619,8 +619,10 @@ static void pci_setup_bridge_mmio_pref(s
     pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, l);
 
     /* Set the upper 32 bits of PREF base & limit. */
-    pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
-    pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
+    if (res->flags & PCI_PREF_RANGE_TYPE_64) {
+        pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
+        pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
+    }
 }
 
 static void __pci_setup_bridge(struct pci_bus *bus, unsigned long type)

I think we should stop putting PCI_PREF_RANGE_TYPE_64 in res->flags. I don't think we ever test for PCI_PREF_RANGE_TYPE_64 today, except for the trivial usage in pci_read_bridge_mmio_pref(), which can be easily removed.

Everywhere else, we use IORESOURCE_MEM, IORESOURCE_MEM_64, and IORESOURCE_PREFETCH to keep track of the properties of the hardware underlying the struct resource, i.e., the BAR. Why shouldn't we do the same for bridge windows?

The struct resource flags should tell us the *possible* properties of the window, e.g., what the bridge can support. These properties are fixed by the bridge hardware, regardless of what devices happen to be below the bridge. We discover those properties at enumeration-time, and we shouldn't change them afterwards.

Quoted 9 lines

It first
tried to find a 32-bit prefetchable window, but we only supply a 64-bit one. So it fall back to (32-bit) non-prefetchable window, but there is no enough room there. At last it went into complicated steps (not show here) of allocating requested resource first, then try best for the optional ones, etc..

Why is BAR 15 (prefetchable) 32 bit instead of 64? Because PCI core favours 32-bit prefetchable BARs and we have some. This is one of them:

Quoted 6 lines

| pci 0003:05:00.0: reg 0x30: [mem 0x00000000-0x000fffff pref]

PCI core decides to let them enjoy the benefition of prefetch. They can't bear the risk of getting 4G-above address, so its parent, its parent's parent, its parent's parent's parent, finally the root bridge (00:00.0) must have their MEM_64 flag of prefetchable resource (BAR 15) clear.

It sounds like we're tracking the resource requirements (prefetchability and BAR width) by using the flags on bridge windows. If that's the case, I think it's wrong. We should preserve the bridge window flags, because those express the bridge hardware capabilities, and we should explicitly keep track of what's required by devices below the bridge in some other way.

Quoted 16 lines · Bjorn Helgaas

Quoted 9 lines

It first
tried to find a 32-bit prefetchable window, but we only supply a 64-bit one. So it fall back to (32-bit) non-prefetchable window, but there is no enough room there. At last it went into complicated steps (not show here) of allocating requested resource first, then try best for the optional ones, etc..

Why is BAR 15 (prefetchable) 32 bit instead of 64? Because PCI core favours 32-bit prefetchable BARs and we have some. This is one of them:

Quoted 6 lines

| pci 0003:05:00.0: reg 0x30: [mem 0x00000000-0x000fffff pref]

PCI core decides to let them enjoy the benefition of prefetch. They can't bear the risk of getting 4G-above address, so its parent, its parent's parent, its parent's parent's parent, finally the root bridge (00:00.0) must have their MEM_64 flag of prefetchable resource (BAR 15) clear.

Quoted 6 lines

It sounds like we're tracking the resource requirements (prefetchability and BAR width) by using the flags on bridge windows. If that's the case, I think it's wrong. We should preserve the bridge window flags, because those express the bridge hardware capabilities, and we should explicitly keep track of what's required by devices below the bridge in some other way.

if the BAR support 64bit, then resource could be 64bit. if the BAR does not support 64bit, then resource should be 32bit.

That should be best guess to decide resource flag.

Thanks for the example. Please open a bug report at
http://bugzilla.kernel.org and attach the complete dmesg logs before and after Yinghai's patch.

Having the complete logs helps me answer questions myself without having to bother you, and it also helps me figure out whether we can improve our logging to make it easier to diagnose problems like this.

Quoted 9 lines

| pci 0003:05:00.0: reg 0x10: [mem 0x3d05801000000-0x3d058010fffff 64bit]
| pci 0003:05:00.0: reg 0x18: [mem 0x3d05010000000-0x3d05017ffffff 64bit pref]
| pci 0003:05:00.0: reg 0x30: [mem 0x00000000-0x000fffff pref]
| pci 0003:05:00.0: reg 0x134: [mem 0x3d05018000000-0x3d0501fffffff 64bit pref]

This is printed at enumeration phase. This device has a SRIOV BAR with size of 0x7ffffff (128M). That's the size of a signle VF BAR. The device supports 63 VFs so we need near 8G space in total. Apparanlty we need exploit 64-bit space.

Yes. Do we print a hint anywhere about how many VFs there are? In other words, can you deduce the number "63" from the dmesg, or do you have to figure that out some other way? It'd be nice if that information were somewhere in dmesg.

Quoted 7 lines

| PCI host bridge to bus 0003:00
| pci_bus 0003:00: root bus resource [mem 0x3d05800000000-0x3d0587ffeffff] (bus address [0x80000000-0xfffeffff])
| pci_bus 0003:00: root bus resource [mem 0x3d05008000000-0x3d057ffffffff 64bit pref]

And we do have a huge (32G) 64-bit prefetchable window supply. We expect everything to work fine, but:

Quoted 6 lines

| pci 0003:00:00.0: BAR 15: can't assign mem pref (size 0x206000000)
| pci 0003:00:00.0: BAR 14: assigned [mem 0x3d05800000000-0x3d05802ffffff]
| pci 0003:00:00.0: BAR 13: can't assign io (size 0x4000)

It went wrong at the beginning. Note the error message never considers 64-bit or not, but BAR 15 here has it MEM_64 flag cleared.

BAR 15 is a bridge window. I think its resource flags should reflect the capability of the *window*, even if we disable the window or we happen to assign addresses that are under 4GB. So I think it's wrong that we clear the MEM_64 flag in pbus_size_mem() and the IO flag in pbus_size_io().

Quoted 9 lines

It first
tried to find a 32-bit prefetchable window, but we only supply a 64-bit one. So it fall back to (32-bit) non-prefetchable window, but there is no enough room there. At last it went into complicated steps (not show here) of allocating requested resource first, then try best for the optional ones, etc..

Why is BAR 15 (prefetchable) 32 bit instead of 64? Because PCI core favours 32-bit prefetchable BARs and we have some. This is one of them:

Quoted 6 lines

| pci 0003:05:00.0: reg 0x30: [mem 0x00000000-0x000fffff pref]

PCI core decides to let them enjoy the benefition of prefetch. They can't bear the risk of getting 4G-above address, so its parent, its parent's parent, its parent's parent's parent, finally the root bridge (00:00.0) must have their MEM_64 flag of prefetchable resource (BAR 15) clear.

It sounds like we're tracking the resource requirements (prefetchability and BAR width) by using the flags on bridge windows. If that's the case, I think it's wrong. We should preserve the bridge window flags, because those express the bridge hardware capabilities, and we should explicitly keep track of what's required by devices below the bridge in some other way.

Quoted 14 lines

In the end nobody
is eligible to use the 64-bit (prefetchable) space even we have huge supply !

Note even the resource is small and successfully fall back into 32-bit non-prefetchable window, that's still not OK for us because we need SRIOV resource be at 64-bit prefetchable space to do platform configuration.

With Yinghai's patch, when 64-bit prefetchable BARs found, they're more favoured than the 32-bit prefetchable ones (if any), so all upstream bridges' prefetchable windows have their MEM_64 flag reserved and the huge 64-bit prefetchable space will be exploited:

Quoted 5 lines

| pci 0003:00:00.0: BAR 15: assigned [mem 0x3d05008000000-0x3d0521fffffff 64bit pref]
| pci 0003:00:00.0: BAR 14: assigned [mem 0x3d05800000000-0x3d05802ffffff]
| pci 0003:00:00.0: BAR 13: can't assign io (size 0x4000)

(The IO resource error here is due to we do not provide IO window)

Yes. The lack of I/O space is just a constraint of the platform. It'd be nice if we printed a more meaningful error message in this case. One really has to be a PCI expert to distinguish this from a real problem that we need to fix.

Bjorn

Quoted 5 lines · Bjorn Helgaas

Thanks for the example. Please open a bug report at
http://bugzilla.kernel.org and attach the complete dmesg logs before and after Yinghai's patch.

Quoted 3 lines

Having the complete logs helps me answer questions myself without having to bother you, and it also helps me figure out whether we can improve our logging to make it easier to diagnose problems like this.

Unfortunately, for a *little while* longer (hint !) we can't publish a complete log from a Power8 machine, but we should be able to include everything remotely related to PCI.

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 8 lines · Bjorn Helgaas

Thanks for the example. Please open a bug report at
http://bugzilla.kernel.org and attach the complete dmesg logs before and after Yinghai's patch.

Having the complete logs helps me answer questions myself without having to bother you, and it also helps me figure out whether we can improve our logging to make it easier to diagnose problems like this.

Quoted 3 lines

Unfortunately, for a *little while* longer (hint !) we can't publish a complete log from a Power8 machine, but we should be able to include everything remotely related to PCI.

Hrm, actually, I'm not sure we can disclose the device details just yet, so let's just hide the vendor/device ID to make the lawyers happy and you can print all the rest.

When one of children resources does not support MEM_64, MEM_64 for bridge get reset, so pull down whole pref resource on the bridge under 4G.

If the bridge support pref mem 64, will only allocate that with pref mem64 to children that support it.
For children resources if they only support pref mem 32, will allocate them from non pref mem instead.

If the bridge only support 32bit pref mmio, will still have all children pref mmio under that.

(quoted diff) +0 -4

-v2: Add release bridge res support with bridge mem res for pref_mem children res.
-v3: refresh and make it can be applied early before for_each_dev_res patchset.
-v4: fix non-pref mmio 64bit support found by Guo Chao.
-v7: repost as ibm's powerpc system work again when
    1. petiboot boot kernel have this patch.
    2. or mellanox driver added new .shutdown member.
    discussion could be found at:

	 http://marc.info/?t=138968064500001&r=1&w=2

Signed-off-by Yinghai Lu <[email protected]>

Tested-by Guo Chao <[email protected]>

Tested-by Wei Yang <[email protected]>

---
 drivers/pci/setup-bus.c |  138 ++++++++++++++++++++++++++++++++----------------
 drivers/pci/setup-res.c |   20 ++++++
 2 files changed, 111 insertions(+), 47 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================

drivers/pci/setup-bus.c +4 -4

@@ -713,12 +713,11 @@ static void pci_bridge_check_ranges(stru
    bus resource of a given type. Note: we intentionally skip
    the bus resources which have already been assigned (that is,
    have non-NULL parent resource). */
-static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type)
+static struct resource *find_free_bus_resource(struct pci_bus *bus,
+			 unsigned long type_mask, unsigned long type)
 {
 	int i;
 	struct resource *r;
-	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-				  IORESOURCE_PREFETCH;
 
 	pci_bus_for_each_resource(bus, r, i) {
 		if (r == &ioport_resource || r == &iomem_resource)

@@ -815,7 +814,8 @@ static void pbus_size_io(struct pci_bus
 		resource_size_t add_size, struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
-	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
+	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
+							IORESOURCE_IO);
 	resource_size_t size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
 	resource_size_t min_align, align;

drivers/pci/setup-bus.c +24 -9

 {
 	struct pci_dev *dev;
 	resource_size_t min_align, align, size, size0, size1;
 	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
 	int order, max_order;
-	struct resource *b_res = find_free_bus_resource(bus, type);
+	struct resource *b_res = find_free_bus_resource(bus,
+					 mask | IORESOURCE_PREFETCH, type);
 	unsigned int mem64_mask = 0;
 	resource_size_t children_add_size = 0;
 

@@ -944,7 +946,9 @@ static int pbus_size_mem(struct pci_bus
 			struct resource *r = &dev->resource[i];
 			resource_size_t r_size;
 
-			if (r->parent || (r->flags & mask) != type)
+			if (r->parent || ((r->flags & mask) != type &&
+					  (r->flags & mask) != type2 &&
+					  (r->flags & mask) != type3))
 				continue;
 			r_size = resource_size(r);
 #ifdef CONFIG_PCI_IOV

@@ -1117,8 +1121,9 @@ void __ref __pci_bus_size_bridges(struct
 			struct list_head *realloc_head)
 {
 	struct pci_dev *dev;
-	unsigned long mask, prefmask;
+	unsigned long mask, prefmask, type2 = 0, type3 = 0;
 	resource_size_t additional_mem_size = 0, additional_io_size = 0;
+	struct resource *b_res;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		struct pci_bus *b = dev->subordinate;

@@ -1163,15 +1168,34 @@ void __ref __pci_bus_size_bridges(struct
 		   has already been allocated by arch code, try
 		   non-prefetchable range for both types of PCI memory
 		   resources. */
+		b_res = &bus->self->resource[PCI_BRIDGE_RESOURCES];
 		mask = IORESOURCE_MEM;
 		prefmask = IORESOURCE_MEM | IORESOURCE_PREFETCH;
-		if (pbus_size_mem(bus, prefmask, prefmask,
+		if (b_res[2].flags & IORESOURCE_MEM_64) {
+			prefmask |= IORESOURCE_MEM_64;
+			if (pbus_size_mem(bus, prefmask, prefmask,
+				  prefmask, prefmask,
 				  realloc_head ? 0 : additional_mem_size,
-				  additional_mem_size, realloc_head))
-			mask = prefmask; /* Success, size non-prefetch only. */
-		else
-			additional_mem_size += additional_mem_size;
-		pbus_size_mem(bus, mask, IORESOURCE_MEM,
+				  additional_mem_size, realloc_head)) {
+					/* Success, size non-pref64 only. */
+					mask = prefmask;
+					type2 = prefmask & ~IORESOURCE_MEM_64;
+					type3 = prefmask & ~IORESOURCE_PREFETCH;
+			}
+		}
+		if (!type2) {
+			prefmask &= ~IORESOURCE_MEM_64;
+			if (pbus_size_mem(bus, prefmask, prefmask,
+					 prefmask, prefmask,
+					 realloc_head ? 0 : additional_mem_size,

drivers/pci/setup-bus.c +21 -27

 		break;

@@ -1257,42 +1281,66 @@ static void __ref __pci_bridge_assign_re
 static void pci_bridge_release_resources(struct pci_bus *bus,
 					  unsigned long type)
 {
-	int idx;
-	bool changed = false;
-	struct pci_dev *dev;
+	struct pci_dev *dev = bus->self;
 	struct resource *r;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-				  IORESOURCE_PREFETCH;
+				  IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
+	unsigned old_flags = 0;
+	struct resource *b_res;
+	int idx = 1;
 
-	dev = bus->self;
-	for (idx = PCI_BRIDGE_RESOURCES; idx <= PCI_BRIDGE_RESOURCE_END;
-	     idx++) {
-		r = &dev->resource[idx];
-		if ((r->flags & type_mask) != type)
-			continue;
-		if (!r->parent)
-			continue;
-		/*
-		 * if there are children under that, we should release them
-		 *  all
-		 */
-		release_child_resources(r);
-		if (!release_resource(r)) {
-			dev_printk(KERN_DEBUG, &dev->dev,
-				 "resource %d %pR released\n", idx, r);
-			/* keep the old size */
-			r->end = resource_size(r) - 1;
-			r->start = 0;
-			r->flags = 0;
-			changed = true;
-		}
-	}
+	b_res = &dev->resource[PCI_BRIDGE_RESOURCES];
+
+	/*
+	 *     1. if there is io port assign fail, will release bridge
+	 *	  io port.
+	 *     2. if there is non pref mmio assign fail, release bridge
+	 *	  nonpref mmio.
+	 *     3. if there is 64bit pref mmio assign fail, and bridge pref
+	 *	  is 64bit, release bridge pref mmio.
+	 *     4. if there is pref mmio assign fail, and bridge pref is
+	 *	  32bit mmio, release bridge pref mmio
+	 *     5. if there is pref mmio assign fail, and bridge pref is not
+	 *	  assigned, release bridge nonpref mmio.
+	 */
+	if (type & IORESOURCE_IO)
+		idx = 0;

drivers/pci/setup-bus.c +22 -2

+
+	r = &b_res[idx];
+
+	if (!r->parent)
+		return;
+
+	/*
+	 * if there are children under that, we should release them
+	 *  all
+	 */
+	release_child_resources(r);
+	if (!release_resource(r)) {
+		type = old_flags = r->flags & type_mask;
+		dev_printk(KERN_DEBUG, &dev->dev, "resource %d %pR released\n",
+					PCI_BRIDGE_RESOURCES + idx, r);
+		/* keep the old size */
+		r->end = resource_size(r) - 1;
+		r->start = 0;
+		r->flags = 0;
 
-	if (changed) {
 		/* avoiding touch the one without PREF */
 		if (type & IORESOURCE_PREFETCH)
 			type = IORESOURCE_PREFETCH;
 		__pci_setup_bridge(bus, type);
+		/* for next child res under same bridge */
+		r->flags = old_flags;
 	}
 }
 

@@ -1471,7 +1519,7 @@ void pci_assign_unassigned_root_bus_reso
 	LIST_HEAD(fail_head);
 	struct pci_dev_resource *fail_res;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-				  IORESOURCE_PREFETCH;
+				  IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
 	int pci_try_num = 1;
 	enum enable_type enable_local;
 

Index: linux-2.6/drivers/pci/setup-res.c
===================================================================

drivers/pci/setup-res.c +3 -2

@@ -208,15 +208,31 @@ static int __pci_assign_resource(struct
 
 	/* First, try exact prefetching match.. */
 	ret = pci_bus_alloc_resource(bus, res, size, align, min,
-				     IORESOURCE_PREFETCH,
+				     IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
 				     pcibios_align_resource, dev);
 
-	if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
+	if (ret < 0 &&
+	    (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) ==

drivers/pci/setup-res.c +5 -0

+
+	if (ret < 0 &&
+	    (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))) {
 		/*
 		 * That failed.
 		 *
 		 * But a prefetching area can handle a non-prefetching
 		 * window (it will just not perform as well).
+		 *
+		 * Also can put 64bit under 32bit range. (below 4g).
 		 */
 		ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
 					     pcibios_align_resource, dev);

I spent quite a bit of time trying to understand this code yesterday, but I failed. Comments might help, but more importantly, it needs to be simplified so the code makes sense even without comments.

Quoted 16 lines

drivers/pci/setup-bus.c +16 -0

+     b_res = &dev->resource[PCI_BRIDGE_RESOURCES];
+
+     /*
+      *     1. if there is io port assign fail, will release bridge
+      *        io port.
+      *     2. if there is non pref mmio assign fail, release bridge
+      *        nonpref mmio.
+      *     3. if there is 64bit pref mmio assign fail, and bridge pref
+      *        is 64bit, release bridge pref mmio.
+      *     4. if there is pref mmio assign fail, and bridge pref is
+      *        32bit mmio, release bridge pref mmio
+      *     5. if there is pref mmio assign fail, and bridge pref is not
+      *        assigned, release bridge nonpref mmio.
+      */
+     if (type & IORESOURCE_IO)
+             idx = 0;

drivers/pci/setup-bus.c static void __ref __pci_bridge_assign_re · line 1317

Quoted 10 lines

drivers/pci/setup-bus.c +10 -0

+     else if (!(type & IORESOURCE_PREFETCH))
+             idx = 1;
+     else if ((type & IORESOURCE_MEM_64) &&
+              (b_res[2].flags & IORESOURCE_MEM_64))
+             idx = 2;
+     else if (!(b_res[2].flags & IORESOURCE_MEM_64) &&
+              (b_res[2].flags & IORESOURCE_PREFETCH))
+             idx = 2;
+     else
+             idx = 1;

This is pretty hard to figure out. Is there any way we can just pass in the bridge window identifier directly, e.g., 0 == I/O window, 1 == non-prefetchable MEM window, 2 == prefetchable window? It seems like way up in pci_assign_unassigned_root_bus_resources(), we ought to know which window failed, and we could just pass that in somehow instead of having to derive it here.

Quoted 4 lines

drivers/pci/setup-res.c +2 -1

-     if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
+     if (ret < 0 &&
+         (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) ==

drivers/pci/setup-res.c static int __pci_assign_resource(struct · line 225

Quoted 10 lines

drivers/pci/setup-res.c +10 -0

+          (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
+             /*
+              * That failed.
+              *
+              * Try below 4g pref
+              */
+             ret = pci_bus_alloc_resource(bus, res, size, align, min,
+                                          IORESOURCE_PREFETCH,
+                                          pcibios_align_resource, dev);
+     }

pci_bus_alloc_from_region() already has a mechanism for restricting allocation to certain bus addresses. I don't like this second mechanism of also using IORESOURCE_MEM_64. There's too much implicit state -- I think there's a dependency here on how we manage the IORESOURCE_MEM_64 bit in the bridge window resources. That makes this code hard to understand.

Quoted 15 lines · Bjorn Helgaas

Quoted 14 lines

drivers/pci/setup-res.c +12 -1

-     if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
+     if (ret < 0 &&
+         (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) ==
+          (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
+             /*
+              * That failed.
+              *
+              * Try below 4g pref
+              */
+             ret = pci_bus_alloc_resource(bus, res, size, align, min,
+                                          IORESOURCE_PREFETCH,
+                                          pcibios_align_resource, dev);
+     }

Quoted 6 lines

pci_bus_alloc_from_region() already has a mechanism for restricting allocation to certain bus addresses. I don't like this second mechanism of also using IORESOURCE_MEM_64. There's too much implicit state -- I think there's a dependency here on how we manage the IORESOURCE_MEM_64 bit in the bridge window resources. That makes this code hard to understand.

will drop that.

pbus_size_mem() previously returned 0 for failure and 1 for success. Change it to return -ENOSPC for failure and 0 for success.

No functional change.

Signed-off-by Bjorn Helgaas <[email protected]>

---
drivers/pci/setup-bus.c | 35 +++++++++++++++++++++--------------

1 file changed, 21 insertions(+), 14 deletions(-)

drivers/pci/setup-bus.c +0 -0

index 589f2261ab0e..caa07fe6a23a 100644

Quoted 15 lines · Bjorn Helgaas

pbus_size_mem() previously returned 0 for failure and 1 for success. Change it to return -ENOSPC for failure and 0 for success.

No functional change.

Signed-off-by Bjorn Helgaas <[email protected]>

---
drivers/pci/setup-bus.c | 35 +++++++++++++++++++++--------------

1 file changed, 21 insertions(+), 14 deletions(-)

drivers/pci/setup-bus.c +0 -0

index 589f2261ab0e..caa07fe6a23a 100644

drivers/pci/setup-bus.c static inline resource_size_t calculate_mem_align(resource_size_t *aligns, · line 919

Quoted 5 lines

drivers/pci/setup-bus.c +2 -0

@@ -915,6 +915,10 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
 *
 * Calculate the size of the bus and minimal alignment which
 * guarantees that all child resources fit in this size.
+ *
+ * Returns -ENOSPC 0 if there's no available bus resource of the desired type.

                    ^^^

One small question, the 0 here means?

I'd like to merge these for v3.16. The first is Yinghai's v9 patch unchanged except to resolve minor merge conflicts, write a changelog, and fold in the mem64_mask removal posted previously as [1].

The rest are cleanups and comment changes to try to make the code more readable.

This does fix a real bug -- in some cases we can't assign space to large 64-bit BARs even though there is plenty of 64-bit space available. But I am concerned that we are giving up performance in some cases (32-bit prefetchable resources below a bridge with a 64-bit prefetchable window will no longer be prefetchable). With some additional work, it should be possible to get that performance back.

Comments welcome.

[1] http://lkml.kernel.org/r/CAE9FiQUKYMPvXLsk84o-gH-j0-wiJGL0Wt1aTW2o4FPSm0PvbQ@mail.gmail.com

---

Bjorn Helgaas (3):
PCI: Change pbus_size_mem() return values to be more conventional
PCI: Simplify __pci_assign_resource() coding style
PCI: Add resource allocation comments

Yinghai Lu (1):
PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources

drivers/pci/setup-bus.c | 207 +++++++++++++++++++++++++++++++++--------------
drivers/pci/setup-res.c | 41 +++++++--

2 files changed, 176 insertions(+), 72 deletions(-)

Bjorn,

I tried to apply it on the lastest upstrea, but seems not work.

Which tree should I apply to? This one ?
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git

Quoted 25 lines

I'd like to merge these for v3.16. The first is Yinghai's v9 patch unchanged except to resolve minor merge conflicts, write a changelog, and fold in the mem64_mask removal posted previously as [1].

The rest are cleanups and comment changes to try to make the code more readable.

This does fix a real bug -- in some cases we can't assign space to large 64-bit BARs even though there is plenty of 64-bit space available. But I am concerned that we are giving up performance in some cases (32-bit prefetchable resources below a bridge with a 64-bit prefetchable window will no longer be prefetchable). With some additional work, it should be possible to get that performance back.

Comments welcome.

[1] http://lkml.kernel.org/r/CAE9FiQUKYMPvXLsk84o-gH-j0-wiJGL0Wt1aTW2o4FPSm0PvbQ@mail.gmail.com

---

Bjorn Helgaas (3):
PCI: Change pbus_size_mem() return values to be more conventional
PCI: Simplify __pci_assign_resource() coding style
PCI: Add resource allocation comments

Quoted 7 lines

Yinghai Lu (1):
PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources

drivers/pci/setup-bus.c | 207 +++++++++++++++++++++++++++++++++--------------
drivers/pci/setup-res.c | 41 +++++++--

2 files changed, 176 insertions(+), 72 deletions(-)

--
Richard Yang
Help you, Help me

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html