2011-06-20 22:47:34

by Ram Pai

[permalink] [raw]
Subject: [PATCH 0/4] PCI: fix cardbus and sriov regressions

The following patch-set fixes regressions caused by:

the commit "PCI: update bridge resources to get more big ranges when allocating space (again)"
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=da7822e5ad71ec9b745b412639f1e5e0ba795a20

patch 1/4: correctly calculates the additional resource size for hotplug bridges.
patch 2/4: ability to resize assigned pci-resource.
patch 3/4: makes SRIOV BARs a nice-to-have resource, which means resources will
be attempted to assign, but not gauranteed to succeed.
patch 4/4: makes cardbus bridge resources nice-to-have resource.


The regression was caused because on some platforms with limited i/o and mem
resources, the nice-to-have resources were allocated ahead of
absolutely-required resources, thus starving the latter. The patchset will
ensure that all the mandatory resource requirements are satisfied before any
nice-to-have resource requirements are satisfied.


2011-06-20 22:47:46

by Ram Pai

[permalink] [raw]
Subject: [PATCH 1/4] PCI: honor child buses add_size in hot plug configuration

From: Yinghai Lu <[email protected]>

Recent pci_bus_size change will use add_size for minimum resource size for pcie
hotplug bridge. But it does not pass children back to parent bridge.

that will have problem on some setup like:
hot add one chassis with more hot plug slots.
for example: if the chassis have 8 slots, we should allocate 8x2M instead
of one 1x2M for parent bus.

So try to get child add_size and compare the sum with parent bus bridge...

Signed-off-by: Yinghai Lu <[email protected]>
Reviewed-by: Ram Pai <[email protected]>
---
drivers/pci/setup-bus.c | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 1e9e5a5..e42b89a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -536,6 +536,20 @@ static resource_size_t calculate_memsize(resource_size_t size,
return size;
}

+static resource_size_t get_res_add_size(struct resource_list_x *add_head,
+ struct resource *res)
+{
+ struct resource_list_x *list;
+
+ /* check if it is in add_head list */
+ for (list = add_head->next; list && list->res != res;
+ list = list->next);
+ if (list)
+ return list->add_size;
+
+ return 0;
+}
+
/**
* pbus_size_io() - size the io window of a given bus
*
@@ -555,6 +569,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
struct pci_dev *dev;
struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
unsigned long size = 0, size0 = 0, size1 = 0;
+ resource_size_t children_add_size = 0;

if (!b_res)
return;
@@ -575,10 +590,15 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
size += r_size;
else
size1 += r_size;
+
+ if (add_head)
+ children_add_size += get_res_add_size(add_head, r);
}
}
size0 = calculate_iosize(size, min_size, size1,
resource_size(b_res), 4096);
+ if (children_add_size > add_size)
+ add_size = children_add_size;
size1 = (!add_head || (add_head && !add_size)) ? size0 :
calculate_iosize(size, min_size+add_size, size1,
resource_size(b_res), 4096);
@@ -620,6 +640,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
int order, max_order;
struct resource *b_res = find_free_bus_resource(bus, type);
unsigned int mem64_mask = 0;
+ resource_size_t children_add_size = 0;

if (!b_res)
return 0;
@@ -661,6 +682,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
if (order > max_order)
max_order = order;
mem64_mask &= r->flags & IORESOURCE_MEM_64;
+
+ if (add_head)
+ children_add_size += get_res_add_size(add_head, r);
}
}
align = 0;
@@ -677,6 +701,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
align += aligns[order];
}
size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
+ if (children_add_size > add_size)
+ add_size = children_add_size;
size1 = (!add_head || (add_head && !add_size)) ? size0 :
calculate_memsize(size, min_size+add_size, 0,
resource_size(b_res), min_align);
--
1.7.0.4

2011-06-20 22:47:55

by Ram Pai

[permalink] [raw]
Subject: [PATCH 2/4] PCI : ability to resize assigned pci-resource

Currently pci-bridges are allocated enough resources to satisfy their immediate
requirements. Any additional resource-requests fail if additional free space,
contiguous to the one already allocated, is not available. This behavior is not
satisfying when sufficient contiguous resources, that can satisfy the request,
is available in a different location.

This patch provides the ability to expand a allocated resource. This behavior
is particularly useful to satisfy larger size nice-to-have resource requests,
like SRIOV BARs or cardbus bridges.

Signed-off-by: Ram Pai <[email protected]>
---
drivers/pci/setup-bus.c | 30 ++++++---
drivers/pci/setup-res.c | 152 +++++++++++++++++++++++++++++------------------
include/linux/pci.h | 1 +
kernel/resource.c | 98 ++++++++++++++++++++++++++++---
4 files changed, 206 insertions(+), 75 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index e42b89a..c282c48 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -34,6 +34,7 @@ struct resource_list_x {
resource_size_t start;
resource_size_t end;
resource_size_t add_size;
+ resource_size_t min_align;
unsigned long flags;
};

@@ -58,7 +59,7 @@ struct resource_list_x {
*/
static void add_to_list(struct resource_list_x *head,
struct pci_dev *dev, struct resource *res,
- resource_size_t add_size)
+ resource_size_t add_size, resource_size_t min_align)
{
struct resource_list_x *list = head;
struct resource_list_x *ln = list->next;
@@ -77,13 +78,16 @@ static void add_to_list(struct resource_list_x *head,
tmp->end = res->end;
tmp->flags = res->flags;
tmp->add_size = add_size;
+ tmp->min_align = min_align;
list->next = tmp;
}

static void add_to_failed_list(struct resource_list_x *head,
struct pci_dev *dev, struct resource *res)
{
- add_to_list(head, dev, res, 0);
+ add_to_list(head, dev, res,
+ 0 /* dont care */,
+ 0 /* dont care */);
}

static void __dev_sort_resources(struct pci_dev *dev,
@@ -152,13 +156,19 @@ static void adjust_resources_sorted(struct resource_list_x *add_head,

idx = res - &list->dev->resource[0];
add_size=list->add_size;
- if (!resource_size(res) && add_size) {
- res->end = res->start + add_size - 1;
- if(pci_assign_resource(list->dev, idx))
+ if (!resource_size(res)) {
+ res->end = res->start + add_size - 1;
+ if(pci_assign_resource(list->dev, idx))
reset_resource(res);
- } else if (add_size) {
- adjust_resource(res, res->start,
- resource_size(res) + add_size);
+ } else {
+ resource_size_t align = list->min_align;
+ res->flags |= list->flags &
+ (IORESOURCE_STARTALIGN|IORESOURCE_SIZEALIGN);
+ if (pci_reassign_resource(list->dev, idx, add_size,
+ align))
+ dev_printk(KERN_DEBUG, &list->dev->dev,
+ "failed to add optional resources res=%pR\n",
+ res);
}
out:
tmp = list;
@@ -615,7 +625,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
b_res->end = b_res->start + size0 - 1;
b_res->flags |= IORESOURCE_STARTALIGN;
if (size1 > size0 && add_head)
- add_to_list(add_head, bus->self, b_res, size1-size0);
+ add_to_list(add_head, bus->self, b_res, size1-size0, 4096);
}

/**
@@ -718,7 +728,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
b_res->end = size0 + min_align - 1;
b_res->flags |= IORESOURCE_STARTALIGN | mem64_mask;
if (size1 > size0 && add_head)
- add_to_list(add_head, bus->self, b_res, size1-size0);
+ add_to_list(add_head, bus->self, b_res, size1-size0, min_align);
return 1;
}

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index bc0e6ee..e8a94d5 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -129,16 +129,16 @@ void pci_disable_bridge_window(struct pci_dev *dev)
}
#endif /* CONFIG_PCI_QUIRKS */

+
+
static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
- int resno)
+ int resno, resource_size_t size, resource_size_t align)
{
struct resource *res = dev->resource + resno;
- resource_size_t size, min, align;
+ resource_size_t min;
int ret;

- size = resource_size(res);
min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
- align = pci_resource_alignment(dev, res);

/* First, try exact prefetching match.. */
ret = pci_bus_alloc_resource(bus, res, size, align, min,
@@ -155,56 +155,101 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
pcibios_align_resource, dev);
}
+ return ret;
+}

- if (ret < 0 && dev->fw_addr[resno]) {
- struct resource *root, *conflict;
- resource_size_t start, end;
+static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
+ int resno, resource_size_t size)
+{
+ struct resource *root, *conflict;
+ resource_size_t start, end;
+ int ret = 0;

- /*
- * If we failed to assign anything, let's try the address
- * where firmware left it. That at least has a chance of
- * working, which is better than just leaving it disabled.
- */
+ if (res->flags & IORESOURCE_IO)
+ root = &ioport_resource;
+ else
+ root = &iomem_resource;
+
+ start = res->start;
+ end = res->end;
+ res->start = dev->fw_addr[resno];
+ res->end = res->start + size - 1;
+ dev_info(&dev->dev, "BAR %d: trying firmware assignment %pR\n",
+ resno, res);
+ conflict = request_resource_conflict(root, res);
+ if (conflict) {
+ dev_info(&dev->dev,
+ "BAR %d: %pR conflicts with %s %pR\n", resno,
+ res, conflict->name, conflict);
+ res->start = start;
+ res->end = end;
+ ret = 1;
+ }
+ return ret;
+}
+
+static int _pci_assign_resource(struct pci_dev *dev, int resno, int size, resource_size_t min_align)
+{
+ struct resource *res = dev->resource + resno;
+ struct pci_bus *bus;
+ int ret;
+ char *type;

- if (res->flags & IORESOURCE_IO)
- root = &ioport_resource;
+ bus = dev->bus;
+ while ((ret = __pci_assign_resource(bus, dev, resno, size, min_align))) {
+ if (!bus->parent || !bus->self->transparent)
+ break;
+ bus = bus->parent;
+ }
+
+ if (ret) {
+ if (res->flags & IORESOURCE_MEM)
+ if (res->flags & IORESOURCE_PREFETCH)
+ type = "mem pref";
+ else
+ type = "mem";
+ else if (res->flags & IORESOURCE_IO)
+ type = "io";
else
- root = &iomem_resource;
-
- start = res->start;
- end = res->end;
- res->start = dev->fw_addr[resno];
- res->end = res->start + size - 1;
- dev_info(&dev->dev, "BAR %d: trying firmware assignment %pR\n",
- resno, res);
- conflict = request_resource_conflict(root, res);
- if (conflict) {
- dev_info(&dev->dev,
- "BAR %d: %pR conflicts with %s %pR\n", resno,
- res, conflict->name, conflict);
- res->start = start;
- res->end = end;
- } else
- ret = 0;
+ type = "unknown";
+ dev_info(&dev->dev,
+ "BAR %d: can't assign %s (size %#llx)\n",
+ resno, type, (unsigned long long) resource_size(res));
}

+ return ret;
+}
+
+int pci_reassign_resource(struct pci_dev *dev, int resno, resource_size_t addsize,
+ resource_size_t min_align)
+{
+ struct resource *res = dev->resource + resno;
+ resource_size_t new_size;
+ int ret;
+
+ if (!res->parent) {
+ dev_info(&dev->dev, "BAR %d: can't reassign an unassigned resouce %pR "
+ "\n", resno, res);
+ return -EINVAL;
+ }
+
+ new_size = resource_size(res) + addsize + min_align - 1;
+ ret = _pci_assign_resource(dev, resno, new_size, min_align);
if (!ret) {
res->flags &= ~IORESOURCE_STARTALIGN;
dev_info(&dev->dev, "BAR %d: assigned %pR\n", resno, res);
if (resno < PCI_BRIDGE_RESOURCES)
pci_update_resource(dev, resno);
}
-
return ret;
}

int pci_assign_resource(struct pci_dev *dev, int resno)
{
struct resource *res = dev->resource + resno;
- resource_size_t align;
+ resource_size_t align, size;
struct pci_bus *bus;
int ret;
- char *type;

align = pci_resource_alignment(dev, res);
if (!align) {
@@ -214,34 +259,27 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
}

bus = dev->bus;
- while ((ret = __pci_assign_resource(bus, dev, resno))) {
- if (bus->parent && bus->self->transparent)
- bus = bus->parent;
- else
- bus = NULL;
- if (bus)
- continue;
- break;
- }
+ size = resource_size(res);
+ ret = _pci_assign_resource(dev, resno, size, align);

- if (ret) {
- if (res->flags & IORESOURCE_MEM)
- if (res->flags & IORESOURCE_PREFETCH)
- type = "mem pref";
- else
- type = "mem";
- else if (res->flags & IORESOURCE_IO)
- type = "io";
- else
- type = "unknown";
- dev_info(&dev->dev,
- "BAR %d: can't assign %s (size %#llx)\n",
- resno, type, (unsigned long long) resource_size(res));
- }
+ /*
+ * If we failed to assign anything, let's try the address
+ * where firmware left it. That at least has a chance of
+ * working, which is better than just leaving it disabled.
+ */
+ if (ret < 0 && dev->fw_addr[resno])
+ ret = pci_revert_fw_address(res, dev, resno, size);

+ if (!ret) {
+ res->flags &= ~IORESOURCE_STARTALIGN;
+ dev_info(&dev->dev, "BAR %d: assigned %pR\n", resno, res);
+ if (resno < PCI_BRIDGE_RESOURCES)
+ pci_update_resource(dev, resno);
+ }
return ret;
}

+
/* Sort resources by alignment */
void pdev_sort_resources(struct pci_dev *dev, struct resource_list *head)
{
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c446b5c..f39d894 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -800,6 +800,7 @@ int __pci_reset_function(struct pci_dev *dev);
int pci_reset_function(struct pci_dev *dev);
void pci_update_resource(struct pci_dev *dev, int resno);
int __must_check pci_assign_resource(struct pci_dev *dev, int i);
+int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
int pci_select_bars(struct pci_dev *dev, unsigned long flags);

/* ROM control related routines */
diff --git a/kernel/resource.c b/kernel/resource.c
index 798e2fa..ff3629c 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -386,7 +386,8 @@ static bool resource_contains(struct resource *res1, struct resource *res2)
/*
* Find empty slot in the resource tree given range and alignment.
*/
-static int find_resource(struct resource *root, struct resource *new,
+static int __find_resource(struct resource *root, struct resource *old,
+ struct resource *new,
resource_size_t size, resource_size_t min,
resource_size_t max, resource_size_t align,
resource_size_t (*alignf)(void *,
@@ -404,14 +405,22 @@ static int find_resource(struct resource *root, struct resource *new,
* Skip past an allocated resource that starts at 0, since the assignment
* of this->start - 1 to tmp->end below would cause an underflow.
*/
- if (this && this->start == 0) {
- tmp.start = this->end + 1;
- this = this->sibling;
+ if (this && this->start == root->start) {
+ if (this == old) {
+ tmp.start = old->start;
+ } else {
+ tmp.start = this->end + 1;
+ }
+ this = this->sibling;
}
for(;;) {
- if (this)
- tmp.end = this->start - 1;
- else
+ if (this) {
+ if (this == old) {
+ tmp.end = this->end;
+ } else {
+ tmp.end = this->start - 1;
+ }
+ } else
tmp.end = root->end;

resource_clip(&tmp, min, max);
@@ -432,12 +441,82 @@ static int find_resource(struct resource *root, struct resource *new,
}
if (!this)
break;
- tmp.start = this->end + 1;
+ if (this != old)
+ tmp.start = this->end + 1;
this = this->sibling;
}
return -EBUSY;
}

+/*
+ * Find empty slot in the resource tree given range and alignment.
+ */
+static int find_resource(struct resource *root, struct resource *new,
+ resource_size_t size, resource_size_t min,
+ resource_size_t max, resource_size_t align,
+ resource_size_t (*alignf)(void *,
+ const struct resource *,
+ resource_size_t,
+ resource_size_t),
+ void *alignf_data)
+{
+ return __find_resource(root, NULL, new, size, min, max, align, alignf, alignf_data);
+}
+
+/**
+ * reallocate_resource - allocate empty slot in the resource tree given range & alignment
+ * @root: root resource descriptor
+ * @new: resource descriptor desired by caller
+ * @size: requested resource region size
+ * @min: minimum size to allocate
+ * @max: maximum size to allocate
+ * @align: alignment requested, in bytes
+ * @alignf: alignment function, optional, called if not NULL
+ * @alignf_data: arbitrary data to pass to the @alignf function
+ */
+int reallocate_resource(struct resource *root, struct resource *old,
+ resource_size_t newsize, resource_size_t min,
+ resource_size_t max, resource_size_t align,
+ resource_size_t (*alignf)(void *,
+ const struct resource *,
+ resource_size_t,
+ resource_size_t),
+ void *alignf_data)
+{
+ int err=0;
+ struct resource new = *old;
+
+ write_lock(&resource_lock);
+
+ if ((err = __find_resource(root, old, &new, newsize, min, max, align,
+ alignf, alignf_data)))
+ goto out;
+
+ if (resource_contains(&new, old)) {
+ old->start = new.start;
+ old->end = new.end;
+ goto out;
+ }
+
+ if (old->child) {
+ err = -EBUSY;
+ goto out;
+ }
+
+ if (resource_contains(old, &new)) {
+ old->start = new.start;
+ old->end = new.end;
+ } else {
+ __release_resource(old);
+ *old = new;
+ __request_resource(root, old);
+ }
+out:
+ write_unlock(&resource_lock);
+ return err;
+}
+
+
/**
* allocate_resource - allocate empty slot in the resource tree given range & alignment
* @root: root resource descriptor
@@ -463,6 +542,9 @@ int allocate_resource(struct resource *root, struct resource *new,
if (!alignf)
alignf = simple_align_resource;

+ if ( new->parent )
+ return reallocate_resource(root, new, size, min, max, align, alignf, alignf_data);
+
write_lock(&resource_lock);
err = find_resource(root, new, size, min, max, align, alignf, alignf_data);
if (err >= 0 && __request_resource(root, new))
--
1.7.0.4

2011-06-20 22:47:57

by Ram Pai

[permalink] [raw]
Subject: [PATCH 3/4] PCI: make SRIOV resources nice-to-have

From: Yinghai Lu <[email protected]>

Allocate resources to SRIOV BARs only after all other genuine resources
requests are satisfied. Dont retry if resource allocation for SRIOV BARs fail.

Signed-off-by: Ram Pai <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
---
drivers/pci/setup-bus.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index c282c48..4f8873e 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -672,6 +672,16 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
if (r->parent || (r->flags & mask) != type)
continue;
r_size = resource_size(r);
+#ifdef CONFIG_PCI_IOV
+ /* add SRIOV BARs to nice-to-have list */
+ if (add_head && i >= PCI_IOV_RESOURCES &&
+ i <= PCI_IOV_RESOURCE_END) {
+ r->end = r->start - 1;
+ add_to_list(add_head, dev, r, r_size, 1);
+ children_add_size += r_size;
+ continue;
+ }
+#endif
/* For bridges size != alignment */
align = pci_resource_alignment(dev, r);
order = __ffs(align) - 20;
--
1.7.0.4

2011-06-20 22:48:09

by Ram Pai

[permalink] [raw]
Subject: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

Allocate resources to cardbus bridge only after all other genuine
resources requests are satisfied. Dont retry if resource allocation
for cardbus-bridge fails.

Tested-by: Oliver Hartkopp <[email protected]>
Signed-off-by: Ram Pai <[email protected]>
---
drivers/pci/setup-bus.c | 20 +++++++++++++-------
1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 4f8873e..023fc9c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -742,7 +742,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
return 1;
}

-static void pci_bus_size_cardbus(struct pci_bus *bus)
+static void pci_bus_size_cardbus(struct pci_bus *bus,
+ struct resource_list_x *add_head)
{
struct pci_dev *bridge = bus->self;
struct resource *b_res = &bridge->resource[PCI_BRIDGE_RESOURCES];
@@ -753,12 +754,14 @@ static void pci_bus_size_cardbus(struct pci_bus *bus)
* a fixed amount of bus space for CardBus bridges.
*/
b_res[0].start = 0;
- b_res[0].end = pci_cardbus_io_size - 1;
+ b_res[0].end = 0;
b_res[0].flags |= IORESOURCE_IO | IORESOURCE_SIZEALIGN;
+ add_to_list(add_head, bridge, b_res, pci_cardbus_io_size - 1, 1);

b_res[1].start = 0;
- b_res[1].end = pci_cardbus_io_size - 1;
+ b_res[1].end = 0;
b_res[1].flags |= IORESOURCE_IO | IORESOURCE_SIZEALIGN;
+ add_to_list(add_head, bridge, b_res+1, pci_cardbus_io_size - 1, 1);

/*
* Check whether prefetchable memory is supported
@@ -778,16 +781,19 @@ static void pci_bus_size_cardbus(struct pci_bus *bus)
*/
if (ctrl & PCI_CB_BRIDGE_CTL_PREFETCH_MEM0) {
b_res[2].start = 0;
- b_res[2].end = pci_cardbus_mem_size - 1;
+ b_res[2].end = 0;
b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_SIZEALIGN;
+ add_to_list(add_head, bridge, b_res+2, pci_cardbus_mem_size - 1, 1);

b_res[3].start = 0;
- b_res[3].end = pci_cardbus_mem_size - 1;
+ b_res[3].end = 0;
b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_SIZEALIGN;
+ add_to_list(add_head, bridge, b_res+3, pci_cardbus_mem_size - 1, 1);
} else {
b_res[3].start = 0;
- b_res[3].end = pci_cardbus_mem_size * 2 - 1;
+ b_res[3].end = 0;
b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_SIZEALIGN;
+ add_to_list(add_head, bridge, b_res+3, pci_cardbus_mem_size * 2 - 1, 1);
}
}

@@ -805,7 +811,7 @@ void __ref __pci_bus_size_bridges(struct pci_bus *bus,

switch (dev->class >> 8) {
case PCI_CLASS_BRIDGE_CARDBUS:
- pci_bus_size_cardbus(b);
+ pci_bus_size_cardbus(b, add_head);
break;

case PCI_CLASS_BRIDGE_PCI:
--
1.7.0.4

2011-06-21 09:20:19

by Dominik Brodowski

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Mon, Jun 20, 2011 at 03:47:17PM -0700, Ram Pai wrote:
> Allocate resources to cardbus bridge only after all other genuine
> resources requests are satisfied. Dont retry if resource allocation
> for cardbus-bridge fails.

Well, for those who use cardbus cards, cardbus resources aren't "nice to
have", they are absolutely required. Of course, not all cardbus cards need
as many resources as are currently assigned, so I wouldn't oppose a patch
which marks _some_ of the currently assigned resources as "nice to have".
But this approach -- 0 required, all "nice to have" -- seems wrong to me.

Best,
Dominik

2011-06-21 16:23:40

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Tue, Jun 21, 2011 at 09:57:00AM +0200, Dominik Brodowski wrote:
> On Mon, Jun 20, 2011 at 03:47:17PM -0700, Ram Pai wrote:
> > Allocate resources to cardbus bridge only after all other genuine
> > resources requests are satisfied. Dont retry if resource allocation
> > for cardbus-bridge fails.
>
> Well, for those who use cardbus cards, cardbus resources aren't "nice to
> have", they are absolutely required. Of course, not all cardbus cards need
> as many resources as are currently assigned, so I wouldn't oppose a patch
> which marks _some_ of the currently assigned resources as "nice to have".
> But this approach -- 0 required, all "nice to have" -- seems wrong to me.

Do you know how much minimal resource is good enough? The value, before
this patch, was 256 for IO ports and 64M for memory.

BTW: If the BIOS has already assigned enough resources for all the devices on
the system, no devices will be starved including the cardbus. The OS intervenes
and is forced to make this hard choice only when it sees unassigned resources to
some devices along with resource contention.

RP

2011-06-21 18:50:57

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Tue, 21 Jun 2011 09:23:21 -0700
Ram Pai <[email protected]> wrote:

> On Tue, Jun 21, 2011 at 09:57:00AM +0200, Dominik Brodowski wrote:
> > On Mon, Jun 20, 2011 at 03:47:17PM -0700, Ram Pai wrote:
> > > Allocate resources to cardbus bridge only after all other genuine
> > > resources requests are satisfied. Dont retry if resource allocation
> > > for cardbus-bridge fails.
> >
> > Well, for those who use cardbus cards, cardbus resources aren't "nice to
> > have", they are absolutely required. Of course, not all cardbus cards need
> > as many resources as are currently assigned, so I wouldn't oppose a patch
> > which marks _some_ of the currently assigned resources as "nice to have".
> > But this approach -- 0 required, all "nice to have" -- seems wrong to me.
>
> Do you know how much minimal resource is good enough? The value, before
> this patch, was 256 for IO ports and 64M for memory.
>
> BTW: If the BIOS has already assigned enough resources for all the devices on
> the system, no devices will be starved including the cardbus. The OS intervenes
> and is forced to make this hard choice only when it sees unassigned resources to
> some devices along with resource contention.

I just know this is going to trigger regressions, so I think Dominik's
concern is valid. We'll have some existing machine with a device whose
resource was never assigned, but we didn't care because it was unused.
Now this code will try to give it some address space at the expense of
something that *is* being used.

But OTOH this will at least try to allocate *some* space to cardbus, we
just won't try as hard as with some other resources. I'd mainly like
to avoid the situation Dominik pointed out, where we have perfectly
good cardbus resources assigned (unlike in Oliver's case) but they're
stolen for a bridge that may get a hotplugged device or some other
device that didn't have a BIOS assignment.

--
Jesse Barnes, Intel Open Source Technology Center

2011-06-21 21:36:34

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Tue, 21 Jun 2011 09:23:21 -0700
Ram Pai <[email protected]> wrote:

> On Tue, Jun 21, 2011 at 09:57:00AM +0200, Dominik Brodowski wrote:
> > On Mon, Jun 20, 2011 at 03:47:17PM -0700, Ram Pai wrote:
> > > Allocate resources to cardbus bridge only after all other genuine
> > > resources requests are satisfied. Dont retry if resource allocation
> > > for cardbus-bridge fails.
> >
> > Well, for those who use cardbus cards, cardbus resources aren't "nice to
> > have", they are absolutely required. Of course, not all cardbus cards need
> > as many resources as are currently assigned, so I wouldn't oppose a patch
> > which marks _some_ of the currently assigned resources as "nice to have".
> > But this approach -- 0 required, all "nice to have" -- seems wrong to me.
>
> Do you know how much minimal resource is good enough? The value, before
> this patch, was 256 for IO ports and 64M for memory.
>
> BTW: If the BIOS has already assigned enough resources for all the devices on
> the system, no devices will be starved including the cardbus. The OS intervenes
> and is forced to make this hard choice only when it sees unassigned resources to
> some devices along with resource contention.

Dominik, presumably you have a few good cardbus test machines; can you
give Ram's patches a try? If we know they break existing
configurations, I'm afraid we'll just have to revert the whole
re-allocation patch yet again. If your stuff survives, I'll ping Linus
to see what he thinks, though he'll probably want to revert in any
case...

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center

2011-06-21 22:13:07

by Dominik Brodowski

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

Hey,

On Tue, Jun 21, 2011 at 02:36:22PM -0700, Jesse Barnes wrote:
> On Tue, 21 Jun 2011 09:23:21 -0700
> Ram Pai <[email protected]> wrote:
>
> > On Tue, Jun 21, 2011 at 09:57:00AM +0200, Dominik Brodowski wrote:
> > > On Mon, Jun 20, 2011 at 03:47:17PM -0700, Ram Pai wrote:
> > > > Allocate resources to cardbus bridge only after all other genuine
> > > > resources requests are satisfied. Dont retry if resource allocation
> > > > for cardbus-bridge fails.
> > >
> > > Well, for those who use cardbus cards, cardbus resources aren't "nice to
> > > have", they are absolutely required. Of course, not all cardbus cards need
> > > as many resources as are currently assigned, so I wouldn't oppose a patch
> > > which marks _some_ of the currently assigned resources as "nice to have".
> > > But this approach -- 0 required, all "nice to have" -- seems wrong to me.
> >
> > Do you know how much minimal resource is good enough? The value, before
> > this patch, was 256 for IO ports and 64M for memory.
> >
> > BTW: If the BIOS has already assigned enough resources for all the devices on
> > the system, no devices will be starved including the cardbus. The OS intervenes
> > and is forced to make this hard choice only when it sees unassigned resources to
> > some devices along with resource contention.
>
> Dominik, presumably you have a few good cardbus test machines; can you
> give Ram's patches a try? If we know they break existing
> configurations, I'm afraid we'll just have to revert the whole
> re-allocation patch yet again. If your stuff survives, I'll ping Linus
> to see what he thinks, though he'll probably want to revert in any
> case...

Actually, I only have one cardbus-capable test machine, which does work in
very most cases, and also I do care much more about the PCMCIA side of
things than the PCI/CardBus side... Therefore, all I could do is some more
or less informed guessing about how much minimal resource we should try to
allocate...

Best,
Dominik

2011-06-22 00:48:23

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Wed, Jun 22, 2011 at 12:13:01AM +0200, Dominik Brodowski wrote:
> Hey,
>
> On Tue, Jun 21, 2011 at 02:36:22PM -0700, Jesse Barnes wrote:
> > On Tue, 21 Jun 2011 09:23:21 -0700
> > Ram Pai <[email protected]> wrote:
> >
> > > On Tue, Jun 21, 2011 at 09:57:00AM +0200, Dominik Brodowski wrote:
> > > > On Mon, Jun 20, 2011 at 03:47:17PM -0700, Ram Pai wrote:
> > > > > Allocate resources to cardbus bridge only after all other genuine
> > > > > resources requests are satisfied. Dont retry if resource allocation
> > > > > for cardbus-bridge fails.
> > > >
> > > > Well, for those who use cardbus cards, cardbus resources aren't "nice to
> > > > have", they are absolutely required. Of course, not all cardbus cards need
> > > > as many resources as are currently assigned, so I wouldn't oppose a patch
> > > > which marks _some_ of the currently assigned resources as "nice to have".
> > > > But this approach -- 0 required, all "nice to have" -- seems wrong to me.
> > >
> > > Do you know how much minimal resource is good enough? The value, before
> > > this patch, was 256 for IO ports and 64M for memory.
> > >
> > > BTW: If the BIOS has already assigned enough resources for all the devices on
> > > the system, no devices will be starved including the cardbus. The OS intervenes
> > > and is forced to make this hard choice only when it sees unassigned resources to
> > > some devices along with resource contention.
> >
> > Dominik, presumably you have a few good cardbus test machines; can you
> > give Ram's patches a try? If we know they break existing
> > configurations, I'm afraid we'll just have to revert the whole
> > re-allocation patch yet again. If your stuff survives, I'll ping Linus
> > to see what he thinks, though he'll probably want to revert in any
> > case...
>
> Actually, I only have one cardbus-capable test machine, which does work in
> very most cases, and also I do care much more about the PCMCIA side of
> things than the PCI/CardBus side... Therefore, all I could do is some more
> or less informed guessing about how much minimal resource we should try to
> allocate...

I assume majority of the platforms will have enough resources to satisfy all
the resource requests, and their BIOS would have done a decent job.

Even if the BIOS has not done a decent job, and there are enough resources
available we should not see a regression.

The only platforms that would expose a regression is when resources are under
contention and the BIOS has assigned enough resource to the cardbus bridge but
not to some other device. It will be hard to find such a platform, but I am
sure there is one out somewhere there.

I am sure we will see; some day, reports of regression because that platform
would have the exact right characteristics to expose the issue. But then that
platform is a highly constrained platform in the first place. Its debatable if
that should be characterised as a regression, or a platform that was riding on
good luck till now.

Even Oliver's platform is a highly constrained platform, and we probably can
treat his platform as 'riding on good luck till now'.

We won't be able to satisfy all the platforms with resource constraints. I
think our probable choice is to select a solution that breaks least number of
platforms and special case those broken platforms through kernel command line
parameters.

RP

2011-06-23 14:55:10

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH 0/4] PCI: fix cardbus and sriov regressions

Hello RP,

unfortunately i noticed the discussion on linux-kernel ML a bit late, as i did
not subscribe it due to the traffic.

Did you get further with the unusual alignment?

Even if i did not see any problems (i did not test the CardBus slot but only
the sdhci_pci), i wonder if the register layout of some adapters would get
into problems with this alignment shown below?!?

Regards,
Oliver

ps. pls put me in CC in the next patch servies. tnx

On 21.06.2011 00:00, Oliver Hartkopp wrote:

>>>
>>> When comparing the logs with mgdiff, i found
>>>
>>> in revered:
>>>
>>> pci_bus 0000:04: resource 0 [io 0x2000-0x20ff]
>>> pci_bus 0000:04: resource 1 [io 0x2400-0x24ff]
>>>
>>> in the fixed version (4 patches)
>>>
>>> pci_bus 0000:04: resource 0 [io 0x2400-0x24ff]
>>> pci_bus 0000:04: resource 1 [io 0x2001-0x2100]
>>>
>>> Don't you think that the ressource 0x2001-0x2100 looks a bit weird?
>>>
>>> off-by-one problem?
>>
>> The resource location has slid by 1. That is because a reassign of
>> resource unfortunately does not let me realign it at the same location.
>>
>> ALIGN() macro is causing that weirdness.
>>
>> However is that slide by 1 causing you any problem?
>
> No. I don't have a PCMCIA card here to test but the SD-Card is working
> properly. I just wondered about the unusual alignment ...

2011-06-23 20:31:48

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Tue, 21 Jun 2011 17:48:16 -0700
Ram Pai <[email protected]> wrote:
> I assume majority of the platforms will have enough resources to satisfy all
> the resource requests, and their BIOS would have done a decent job.
>
> Even if the BIOS has not done a decent job, and there are enough resources
> available we should not see a regression.
>
> The only platforms that would expose a regression is when resources are under
> contention and the BIOS has assigned enough resource to the cardbus bridge but
> not to some other device. It will be hard to find such a platform, but I am
> sure there is one out somewhere there.
>
> I am sure we will see; some day, reports of regression because that platform
> would have the exact right characteristics to expose the issue. But then that
> platform is a highly constrained platform in the first place. Its debatable if
> that should be characterised as a regression, or a platform that was riding on
> good luck till now.
>
> Even Oliver's platform is a highly constrained platform, and we probably can
> treat his platform as 'riding on good luck till now'.
>
> We won't be able to satisfy all the platforms with resource constraints. I
> think our probable choice is to select a solution that breaks least number of
> platforms and special case those broken platforms through kernel command line
> parameters.

Another option is to hide the new allocation behavior behind a kernel
parameter. I know Bjorn has opposed this in the past because really
this sort of thing should "just work". But so far it hasn't, and we've
had to revert both Bjorn's resource tracking changes as well as the
re-allocation code.

Hiding it behind a boot option would at least let us improve things
over time and potentially switch over to new resource code in the
future...

Thoughts?

--
Jesse Barnes, Intel Open Source Technology Center

2011-06-23 20:41:49

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Thursday, June 23, 2011, Jesse Barnes wrote:
> On Tue, 21 Jun 2011 17:48:16 -0700
> Ram Pai <[email protected]> wrote:
> > I assume majority of the platforms will have enough resources to satisfy all
> > the resource requests, and their BIOS would have done a decent job.
> >
> > Even if the BIOS has not done a decent job, and there are enough resources
> > available we should not see a regression.
> >
> > The only platforms that would expose a regression is when resources are under
> > contention and the BIOS has assigned enough resource to the cardbus bridge but
> > not to some other device. It will be hard to find such a platform, but I am
> > sure there is one out somewhere there.
> >
> > I am sure we will see; some day, reports of regression because that platform
> > would have the exact right characteristics to expose the issue. But then that
> > platform is a highly constrained platform in the first place. Its debatable if
> > that should be characterised as a regression, or a platform that was riding on
> > good luck till now.
> >
> > Even Oliver's platform is a highly constrained platform, and we probably can
> > treat his platform as 'riding on good luck till now'.
> >
> > We won't be able to satisfy all the platforms with resource constraints. I
> > think our probable choice is to select a solution that breaks least number of
> > platforms and special case those broken platforms through kernel command line
> > parameters.
>
> Another option is to hide the new allocation behavior behind a kernel
> parameter. I know Bjorn has opposed this in the past because really
> this sort of thing should "just work". But so far it hasn't, and we've
> had to revert both Bjorn's resource tracking changes as well as the
> re-allocation code.
>
> Hiding it behind a boot option would at least let us improve things
> over time and potentially switch over to new resource code in the
> future...
>
> Thoughts?

Do I understand correctly that at the moment we have two set of systems,
one of which works with the new code and doesn't work with the old code
and the other one conversely?

Rafael

2011-06-24 16:29:32

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Thu, Jun 23, 2011 at 10:42:29PM +0200, Rafael J. Wysocki wrote:
> On Thursday, June 23, 2011, Jesse Barnes wrote:
> > On Tue, 21 Jun 2011 17:48:16 -0700
> > Ram Pai <[email protected]> wrote:
> > > I assume majority of the platforms will have enough resources to satisfy all
> > > the resource requests, and their BIOS would have done a decent job.
> > >
> > > Even if the BIOS has not done a decent job, and there are enough resources
> > > available we should not see a regression.
> > >
> > > The only platforms that would expose a regression is when resources are under
> > > contention and the BIOS has assigned enough resource to the cardbus bridge but
> > > not to some other device. It will be hard to find such a platform, but I am
> > > sure there is one out somewhere there.
> > >
> > > I am sure we will see; some day, reports of regression because that platform
> > > would have the exact right characteristics to expose the issue. But then that
> > > platform is a highly constrained platform in the first place. Its debatable if
> > > that should be characterised as a regression, or a platform that was riding on
> > > good luck till now.
> > >
> > > Even Oliver's platform is a highly constrained platform, and we probably can
> > > treat his platform as 'riding on good luck till now'.
> > >
> > > We won't be able to satisfy all the platforms with resource constraints. I
> > > think our probable choice is to select a solution that breaks least number of
> > > platforms and special case those broken platforms through kernel command line
> > > parameters.
> >
> > Another option is to hide the new allocation behavior behind a kernel
> > parameter. I know Bjorn has opposed this in the past because really
> > this sort of thing should "just work". But so far it hasn't, and we've
> > had to revert both Bjorn's resource tracking changes as well as the
> > re-allocation code.
> >
> > Hiding it behind a boot option would at least let us improve things
> > over time and potentially switch over to new resource code in the
> > future...
> >
> > Thoughts?
>
> Do I understand correctly that at the moment we have two set of systems,
> one of which works with the new code and doesn't work with the old code
> and the other one conversely?

Here is the current state:

(a) As of 2.6.39, for platforms whose BIOS have not allocated enough resources to its
devices, those devices will **continue to not work**. An example of such a platform is
the one whose BIOS has not allocated enough resources to SRIOV BARs.

(b) With Yinghai's patch
the commit "PCI: update bridge resources to get more big ranges when allocating space (again)"
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=da7822e5ad71ec9b745b412639f1e5e0ba795a20
Most of the platforms that were not working in (a) will start working, but will break a few platforms, that
have resource constraints and whose BIOS has not allocated enough resources to some of its devices.
Oliver's and Ben Hutching's platform are two of the known platforms; as of now.

(c) with my patch all the above platforms will start working. But the 4th patch in the series
raises a genuine concern that it might break resource-constrained platforms with cardbus bridges.

The question is which one of these is a lesser-evil :)

Personally I think we should merge all the patches except the 4th patch, and support
Oliver's platform through kernel command line parameter. And I think we should
revert Yinghai's patch for now and merge it with all other patches in the 3.0.1 timeframe
after thorough testing.

RP

2011-06-24 19:29:20

by Ram Pai

[permalink] [raw]
Subject: Re: [PATCH 0/4] PCI: fix cardbus and sriov regressions

On Thu, Jun 23, 2011 at 04:55:06PM +0200, Oliver Hartkopp wrote:
> Hello RP,
>
> unfortunately i noticed the discussion on linux-kernel ML a bit late, as i did
> not subscribe it due to the traffic.

sorry, did not see this mail earlier.


> >>> When comparing the logs with mgdiff, i found
> >>>
> >>> in revered:
> >>>
> >>> pci_bus 0000:04: resource 0 [io 0x2000-0x20ff]
> >>> pci_bus 0000:04: resource 1 [io 0x2400-0x24ff]
> >>>
> >>> in the fixed version (4 patches)
> >>>
> >>> pci_bus 0000:04: resource 0 [io 0x2400-0x24ff]
> >>> pci_bus 0000:04: resource 1 [io 0x2001-0x2100]
>
> Did you get further with the unusual alignment?

No. i was thinking this alignment should be ok, since that cardbus resource
has to be SIZE aligned and not START aligned.

However if it is not acceptable, i will figure out a way
to get it aligned on the right boundaries.

RP

2011-06-24 23:45:15

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 4/4] PCI: make cardbus-bridge resources nice-to-have

On Friday, June 24, 2011, Ram Pai wrote:
> On Thu, Jun 23, 2011 at 10:42:29PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, June 23, 2011, Jesse Barnes wrote:
> > > On Tue, 21 Jun 2011 17:48:16 -0700
> > > Ram Pai <[email protected]> wrote:
> > > > I assume majority of the platforms will have enough resources to satisfy all
> > > > the resource requests, and their BIOS would have done a decent job.
> > > >
> > > > Even if the BIOS has not done a decent job, and there are enough resources
> > > > available we should not see a regression.
> > > >
> > > > The only platforms that would expose a regression is when resources are under
> > > > contention and the BIOS has assigned enough resource to the cardbus bridge but
> > > > not to some other device. It will be hard to find such a platform, but I am
> > > > sure there is one out somewhere there.
> > > >
> > > > I am sure we will see; some day, reports of regression because that platform
> > > > would have the exact right characteristics to expose the issue. But then that
> > > > platform is a highly constrained platform in the first place. Its debatable if
> > > > that should be characterised as a regression, or a platform that was riding on
> > > > good luck till now.
> > > >
> > > > Even Oliver's platform is a highly constrained platform, and we probably can
> > > > treat his platform as 'riding on good luck till now'.
> > > >
> > > > We won't be able to satisfy all the platforms with resource constraints. I
> > > > think our probable choice is to select a solution that breaks least number of
> > > > platforms and special case those broken platforms through kernel command line
> > > > parameters.
> > >
> > > Another option is to hide the new allocation behavior behind a kernel
> > > parameter. I know Bjorn has opposed this in the past because really
> > > this sort of thing should "just work". But so far it hasn't, and we've
> > > had to revert both Bjorn's resource tracking changes as well as the
> > > re-allocation code.
> > >
> > > Hiding it behind a boot option would at least let us improve things
> > > over time and potentially switch over to new resource code in the
> > > future...
> > >
> > > Thoughts?
> >
> > Do I understand correctly that at the moment we have two set of systems,
> > one of which works with the new code and doesn't work with the old code
> > and the other one conversely?
>
> Here is the current state:
>
> (a) As of 2.6.39, for platforms whose BIOS have not allocated enough resources to its
> devices, those devices will **continue to not work**. An example of such a platform is
> the one whose BIOS has not allocated enough resources to SRIOV BARs.
>
> (b) With Yinghai's patch
> the commit "PCI: update bridge resources to get more big ranges when allocating space (again)"
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=da7822e5ad71ec9b745b412639f1e5e0ba795a20
> Most of the platforms that were not working in (a) will start working, but will break a few platforms, that
> have resource constraints and whose BIOS has not allocated enough resources to some of its devices.
> Oliver's and Ben Hutching's platform are two of the known platforms; as of now.
>
> (c) with my patch all the above platforms will start working. But the 4th patch in the series
> raises a genuine concern that it might break resource-constrained platforms with cardbus bridges.
>
> The question is which one of these is a lesser-evil :)
>
> Personally I think we should merge all the patches except the 4th patch, and support
> Oliver's platform through kernel command line parameter. And I think we should
> revert Yinghai's patch for now and merge it with all other patches in the 3.0.1
> timeframe after thorough testing.

Well, I think in that case the default behavior should be as for 2.6.39
and there may be a command line switch turning on some new behavior for
whoever needs that.

This way we'll avoid introducing regressions on systems that don't use the
new command line switch and we'' allow systems that don't work without it to
be handled.

What the new behavior should be is to be determined I guess.

Thanks,
Rafael

2011-06-25 11:35:39

by Oliver Hartkopp

[permalink] [raw]
Subject: Re: [PATCH 0/4] PCI: fix cardbus and sriov regressions

On 24.06.2011 21:29, Ram Pai wrote:
> On Thu, Jun 23, 2011 at 04:55:06PM +0200, Oliver Hartkopp wrote:
>> Hello RP,
>>
>> unfortunately i noticed the discussion on linux-kernel ML a bit late, as i did
>> not subscribe it due to the traffic.
>
> sorry, did not see this mail earlier.
>
>
>>>>> When comparing the logs with mgdiff, i found
>>>>>
>>>>> in revered:
>>>>>
>>>>> pci_bus 0000:04: resource 0 [io 0x2000-0x20ff]
>>>>> pci_bus 0000:04: resource 1 [io 0x2400-0x24ff]
>>>>>
>>>>> in the fixed version (4 patches)
>>>>>
>>>>> pci_bus 0000:04: resource 0 [io 0x2400-0x24ff]
>>>>> pci_bus 0000:04: resource 1 [io 0x2001-0x2100]
>>
>> Did you get further with the unusual alignment?
>
> No. i was thinking this alignment should be ok, since that cardbus resource
> has to be SIZE aligned and not START aligned.
>
> However if it is not acceptable, i will figure out a way
> to get it aligned on the right boundaries.

I'm not sure how drivers expect to access the register layouts of their
specific hardware. E.g. if you have a 32-bit register at the beginning of the
io resource and the driver accesses this with a 32-bit write operation, the
misalignment would be handled (by the CPU) very expensive at runtime, which is
definitely a big drawback in performance. I know the networking people
re-arranging structs to get less cycles in accessing data structures ...

I tried to understand the reason for the original commit in 3.0.0-rc1 that
broke my system - very ambitious :-)

But IMHO you should meet analogue starting boundaries as we had before to make
sure that you don't brake things on machines that did not show up yet.

Best regards,
Oliver

2011-06-27 17:34:46

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH 0/4] PCI: fix cardbus and sriov regressions

[+linux-pci again]

On Fri, Jun 24, 2011 at 1:29 PM, Ram Pai <[email protected]> wrote:
> On Thu, Jun 23, 2011 at 04:55:06PM +0200, Oliver Hartkopp wrote:
>> Hello RP,
>>
>> unfortunately i noticed the discussion on linux-kernel ML a bit late, as i did
>> not subscribe it due to the traffic.
>
> sorry, did not see this mail earlier.
>
>
>> >>> When comparing the logs with mgdiff, i found
>> >>>
>> >>> in revered:
>> >>>
>> >>> pci_bus 0000:04: resource 0 [io ?0x2000-0x20ff]
>> >>> pci_bus 0000:04: resource 1 [io ?0x2400-0x24ff]
>> >>>
>> >>> in the fixed version (4 patches)
>> >>>
>> >>> pci_bus 0000:04: resource 0 [io ?0x2400-0x24ff]
>> >>> pci_bus 0000:04: resource 1 [io ?0x2001-0x2100]
>>
>> Did you get further with the unusual alignment?
>
> No. i was thinking this alignment ?should be ok, since that cardbus resource
> has to be SIZE aligned and not START aligned.
>
> However if it is not acceptable, i will figure out a way
> to get it aligned on the right boundaries.

Does the bridge leading to bus 04 really support a window starting at 0x2001?

I assume this is a CardBus bridge because it has two I/O windows. My
interpretation of the MindShare CardBus book (I don't have the PC Card
Standard) is that the low two bits of the I/O base address register
tell you whether 16- or 32-bit addressing is supported (00b means
16-bit decoding is used, 01b means 32-bit decoding is used), and that
the actual window starts on a four-byte aligned address.

If that's correct, an I/O base register containing 0x2001 would mean a
window starting at 0x2000 and supporting 32-bit decoding, and it would
be impossible to have a window starting at 0x2001, i.e., one that
contains 0x2001 but not 0x2000.

Bjorn