[PATCH] pci: Check bridge resources after resource allocation.

[PATCH] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

During pci remove/rescan testing found:

[  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
[  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
[  541.199202] pci 0000:c0:03.0: enabling bus mastering
[  541.199209] pci 0000:c0:03.0: setting latency timer to 64
[  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22

This bug was uncovered by commit
| commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
| Author: Ram Pai <linuxram@us.ibm.com>
| Date:   Mon Feb 14 17:43:20 2011 -0800
|
|    PCI: pre-allocate additional resources to devices only after successful allo
cation of essential resources.

After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size. So it will not get into failed list, and will not be reset there.

The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
bridge, and later if children does not need to IO resource. those bridge
resources will not need to be allocated. but flags still there.

Add pci_bridge_check_resources() to close the loop.

after patch, will get right result:
[  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
[  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  621.247012] pci 0000:c0:03.0: enabling bus mastering
[  621.247275] pci 0000:c0:03.0: setting latency timer to 64
[  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
[  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
[  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
[  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
[  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
[  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
[  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:

Signed-off-by: Yinghai Lu <yinghai@kernel.org>


---
 drivers/pci/setup-bus.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -811,6 +811,27 @@ void __ref pci_bus_size_bridges(struct p
 }
 EXPORT_SYMBOL(pci_bus_size_bridges);
 
+static void __ref pci_bridge_check_resources(struct pci_dev *bridge)
+{
+	struct resource *b_res;
+	int i;
+
+	b_res = &bridge->resource[PCI_BRIDGE_RESOURCES];
+
+	/*
+	 * We set resource flag in pci_bridge_check_ranges() for further
+	 *  allocation for children devices.
+	 * Then if children do not need some kind of resource (like IO), and
+	 *  bridge resource will not be allocated.
+	 * At last, those flags could be left set, that will confuse
+	 *  pci_setup_bridge() and pci_enable_bridge()
+	 * So use reset_resource to clear them.
+	 */
+	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++)
+		if (!resource_size(&b_res[i]))
+			reset_resource(&b_res[i]);
+}
+
 static void __ref __pci_bus_assign_resources(const struct pci_bus *bus,
 					 struct resource_list_x *add_head,
 					 struct resource_list_x *fail_head)
@@ -820,6 +841,9 @@ static void __ref __pci_bus_assign_resou
 
 	pbus_assign_resources_sorted(bus, add_head, fail_head);
 
+	if (bus->self)
+		pci_bridge_check_resources(bus->self);
+
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		b = dev->subordinate;
 		if (!b)
@@ -862,6 +886,8 @@ static void __ref __pci_bridge_assign_re
 	if (!b)
 		return;
 
+	pci_bridge_check_resources((struct pci_dev *)bridge);
+
 	__pci_bus_assign_resources(b, NULL, fail_head);
 
 	switch (bridge->class >> 8) {

Re: [PATCH] pci: Check bridge resources after resource allocation.

[Ram Pai]

On Thu, May 05, 2011 at 12:24:22AM -0700, Yinghai Lu wrote:
> 
> During pci remove/rescan testing found:
> 
> [  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
> [  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
> [  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
> [  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
> [  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> [  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
> [  541.199202] pci 0000:c0:03.0: enabling bus mastering
> [  541.199209] pci 0000:c0:03.0: setting latency timer to 64
> [  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> [  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22
> 
> This bug was uncovered by commit
> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
> | Author: Ram Pai <linuxram@us.ibm.com>
> | Date:   Mon Feb 14 17:43:20 2011 -0800
> |
> |    PCI: pre-allocate additional resources to devices only after successful allo
> cation of essential resources.
> 
> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size. So it will not get into failed list, and will not be reset there.
> 
> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
> bridge, and later if children does not need to IO resource. those bridge
> resources will not need to be allocated. but flags still there.
> 
> Add pci_bridge_check_resources() to close the loop.

How about resetting the resource in  adjust_resources_sorted() if  call to
adjust_resource() fails, and the resource is left with zero size?

Something like this:

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8e73abf..77e8454 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -157,8 +157,11 @@ static void adjust_resources_sorted(struct resource_list_x *add_head,
                         if(pci_assign_resource(list->dev, idx))
                                reset_resource(res);
                } else if (add_size) {
-                       adjust_resource(res, res->start,
-                               resource_size(res) + add_size);
+                       if (adjust_resource(res, res->start,
+                               resource_size(res) + add_size) && 
+                               !resource_size(res)) {
+                               reset_resource(res);
+                       }
                }
 out:
                tmp = list;


RP

Re: [PATCH] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

On 05/06/2011 01:12 AM, Ram Pai wrote:
> On Thu, May 05, 2011 at 12:24:22AM -0700, Yinghai Lu wrote:
>>
>> During pci remove/rescan testing found:
>>
>> [  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
>> [  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
>> [  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
>> [  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
>> [  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
>> [  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
>> [  541.199202] pci 0000:c0:03.0: enabling bus mastering
>> [  541.199209] pci 0000:c0:03.0: setting latency timer to 64
>> [  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
>> [  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22
>>
>> This bug was uncovered by commit
>> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
>> | Author: Ram Pai <linuxram@us.ibm.com>
>> | Date:   Mon Feb 14 17:43:20 2011 -0800
>> |
>> |    PCI: pre-allocate additional resources to devices only after successful allo
>> cation of essential resources.
>>
>> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size. So it will not get into failed list, and will not be reset there.
>>
>> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
>> bridge, and later if children does not need to IO resource. those bridge
>> resources will not need to be allocated. but flags still there.
>>
>> Add pci_bridge_check_resources() to close the loop.
> 
> How about resetting the resource in  adjust_resources_sorted() if  call to
> adjust_resource() fails, and the resource is left with zero size?
> 
> Something like this:
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8e73abf..77e8454 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -157,8 +157,11 @@ static void adjust_resources_sorted(struct resource_list_x *add_head,
>                          if(pci_assign_resource(list->dev, idx))
>                                 reset_resource(res);
>                 } else if (add_size) {
> -                       adjust_resource(res, res->start,
> -                               resource_size(res) + add_size);
> +                       if (adjust_resource(res, res->start,
> +                               resource_size(res) + add_size) && 
> +                               !resource_size(res)) {
> +                               reset_resource(res);
> +                       }
>                 }
>  out:
>                 tmp = list;
> 
> 

no, adjust_resource will not change resource size to zero.

also pci_assign_unassigned_bridge_resources ==> pci_bus_size_bridges, it will not take add_list. 

following patch works too.

[PATCH] pci: Using add_list in pcie hotplug path.

During pci remove/rescan testing found:

[  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
[  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
[  541.199202] pci 0000:c0:03.0: enabling bus mastering
[  541.199209] pci 0000:c0:03.0: setting latency timer to 64
[  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22

This bug was uncovered by commit
| commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
| Author: Ram Pai <linuxram@us.ibm.com>
| Date:   Mon Feb 14 17:43:20 2011 -0800
|
|    PCI: pre-allocate additional resources to devices only after successful allo
cation of essential resources.

After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size.
So it will not get into failed list, and will not be reset there.

It turns out we need to add_head for those calling.
So those resource get reset properly.

after patch, will get right result:
[  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
[  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  621.247012] pci 0000:c0:03.0: enabling bus mastering
[  621.247275] pci 0000:c0:03.0: setting latency timer to 64
[  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
[  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
[  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
[  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
[  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
[  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
[  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

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

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -217,13 +217,14 @@ static void __assign_resources_sorted(st
 }
 
 static void pdev_assign_resources_sorted(struct pci_dev *dev,
+				 struct resource_list_x *add_head,
 				 struct resource_list_x *fail_head)
 {
 	struct resource_list head;
 
 	head.next = NULL;
 	__dev_sort_resources(dev, &head);
-	__assign_resources_sorted(&head, NULL, fail_head);
+	__assign_resources_sorted(&head, add_head, fail_head);
 
 }
 
@@ -852,17 +853,19 @@ void __ref pci_bus_assign_resources(cons
 EXPORT_SYMBOL(pci_bus_assign_resources);
 
 static void __ref __pci_bridge_assign_resources(const struct pci_dev *bridge,
+					 struct resource_list_x *add_head,
 					 struct resource_list_x *fail_head)
 {
 	struct pci_bus *b;
 
-	pdev_assign_resources_sorted((struct pci_dev *)bridge, fail_head);
+	pdev_assign_resources_sorted((struct pci_dev *)bridge,
+					 add_head, fail_head);
 
 	b = bridge->subordinate;
 	if (!b)
 		return;
 
-	__pci_bus_assign_resources(b, NULL, fail_head);
+	__pci_bus_assign_resources(b, add_head, fail_head);
 
 	switch (bridge->class >> 8) {
 	case PCI_CLASS_BRIDGE_PCI:
@@ -1129,6 +1132,8 @@ enable_and_dump:
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 {
 	struct pci_bus *parent = bridge->subordinate;
+	struct resource_list_x add_list; /* list of resources that
+					want additional resources */
 	int tried_times = 0;
 	struct resource_list_x head, *list;
 	int retval;
@@ -1136,11 +1141,12 @@ void pci_assign_unassigned_bridge_resour
 				  IORESOURCE_PREFETCH;
 
 	head.next = NULL;
+	add_list.next = NULL;
 
 again:
-	pci_bus_size_bridges(parent);
-	__pci_bridge_assign_resources(bridge, &head);
-
+	__pci_bus_size_bridges(parent, &add_list);
+	__pci_bridge_assign_resources(bridge, &add_list, &head);
+	BUG_ON(add_list.next);
 	tried_times++;
 
 	if (!head.next)

[PATCH 1/2] pci: Using add_list in pcie hotplug path.

[Yinghai Lu]

During pci remove/rescan testing found:

[  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
[  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
[  541.199202] pci 0000:c0:03.0: enabling bus mastering
[  541.199209] pci 0000:c0:03.0: setting latency timer to 64
[  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22

This bug was uncovered by commit
| commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
| Author: Ram Pai <linuxram@us.ibm.com>
| Date:   Mon Feb 14 17:43:20 2011 -0800
|
|    PCI: pre-allocate additional resources to devices only after successful allo
cation of essential resources.

After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size.
So it will not get into failed list, and will not be reset there.

It turns out we need to add_head for those calling.
So those resource get reset properly.

after patch, will get right result:
[  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
[  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  621.247012] pci 0000:c0:03.0: enabling bus mastering
[  621.247275] pci 0000:c0:03.0: setting latency timer to 64
[  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
[  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
[  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
[  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
[  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
[  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
[  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:

We also need this patch when pluging in hotplug chassis without cards.

-v2: update a little bit descriptions.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

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

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -217,13 +217,14 @@ static void __assign_resources_sorted(st
 }
 
 static void pdev_assign_resources_sorted(struct pci_dev *dev,
+				 struct resource_list_x *add_head,
 				 struct resource_list_x *fail_head)
 {
 	struct resource_list head;
 
 	head.next = NULL;
 	__dev_sort_resources(dev, &head);
-	__assign_resources_sorted(&head, NULL, fail_head);
+	__assign_resources_sorted(&head, add_head, fail_head);
 
 }
 
@@ -852,17 +853,19 @@ void __ref pci_bus_assign_resources(cons
 EXPORT_SYMBOL(pci_bus_assign_resources);
 
 static void __ref __pci_bridge_assign_resources(const struct pci_dev *bridge,
+					 struct resource_list_x *add_head,
 					 struct resource_list_x *fail_head)
 {
 	struct pci_bus *b;
 
-	pdev_assign_resources_sorted((struct pci_dev *)bridge, fail_head);
+	pdev_assign_resources_sorted((struct pci_dev *)bridge,
+					 add_head, fail_head);
 
 	b = bridge->subordinate;
 	if (!b)
 		return;
 
-	__pci_bus_assign_resources(b, NULL, fail_head);
+	__pci_bus_assign_resources(b, add_head, fail_head);
 
 	switch (bridge->class >> 8) {
 	case PCI_CLASS_BRIDGE_PCI:
@@ -1129,6 +1132,8 @@ enable_and_dump:
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 {
 	struct pci_bus *parent = bridge->subordinate;
+	struct resource_list_x add_list; /* list of resources that
+					want additional resources */
 	int tried_times = 0;
 	struct resource_list_x head, *list;
 	int retval;
@@ -1136,11 +1141,12 @@ void pci_assign_unassigned_bridge_resour
 				  IORESOURCE_PREFETCH;
 
 	head.next = NULL;
+	add_list.next = NULL;
 
 again:
-	pci_bus_size_bridges(parent);
-	__pci_bridge_assign_resources(bridge, &head);
-
+	__pci_bus_size_bridges(parent, &add_list);
+	__pci_bridge_assign_resources(bridge, &add_list, &head);
+	BUG_ON(add_list.next);
 	tried_times++;
 
 	if (!head.next)

[PATCH 2/2] pci: honor child buses add_size in hot plug configuration

[Yinghai Lu]

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 <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -537,6 +537,20 @@ static resource_size_t calculate_memsize
 	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
  *
@@ -556,6 +570,7 @@ static void pbus_size_io(struct pci_bus
 	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;
@@ -576,10 +591,15 @@ static void pbus_size_io(struct pci_bus
 				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_size? size0:
 		calculate_iosize(size, min_size+add_size, size1,
 			resource_size(b_res), 4096);
@@ -621,6 +641,7 @@ static int pbus_size_mem(struct pci_bus
 	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;
@@ -662,6 +683,9 @@ static int pbus_size_mem(struct pci_bus
 			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;
@@ -678,6 +702,8 @@ static int pbus_size_mem(struct pci_bus
 		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_size ? size :
 		calculate_memsize(size, min_size+add_size, 0,
 				resource_size(b_res), min_align);

Re: [PATCH] pci: Check bridge resources after resource allocation.

[Ram Pai]

On Fri, May 06, 2011 at 01:22:04PM -0700, Yinghai Lu wrote:
> On 05/06/2011 01:12 AM, Ram Pai wrote:
> > On Thu, May 05, 2011 at 12:24:22AM -0700, Yinghai Lu wrote:
> >>
> >> During pci remove/rescan testing found:
> >>
> >> [  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
> >> [  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
> >> [  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
> >> [  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
> >> [  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> >> [  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
> >> [  541.199202] pci 0000:c0:03.0: enabling bus mastering
> >> [  541.199209] pci 0000:c0:03.0: setting latency timer to 64
> >> [  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> >> [  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22
> >>
> >> This bug was uncovered by commit
> >> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
> >> | Author: Ram Pai <linuxram@us.ibm.com>
> >> | Date:   Mon Feb 14 17:43:20 2011 -0800
> >> |
> >> |    PCI: pre-allocate additional resources to devices only after successful allo
> >> cation of essential resources.
> >>
> >> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size. So it will not get into failed list, and will not be reset there.
> >>
> >> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
> >> bridge, and later if children does not need to IO resource. those bridge
> >> resources will not need to be allocated. but flags still there.
> >>
> >> Add pci_bridge_check_resources() to close the loop.
> > 
> > How about resetting the resource in  adjust_resources_sorted() if  call to
> > adjust_resource() fails, and the resource is left with zero size?
> > 
> > Something like this:
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 8e73abf..77e8454 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -157,8 +157,11 @@ static void adjust_resources_sorted(struct resource_list_x *add_head,
> >                          if(pci_assign_resource(list->dev, idx))
> >                                 reset_resource(res);
> >                 } else if (add_size) {
> > -                       adjust_resource(res, res->start,
> > -                               resource_size(res) + add_size);
> > +                       if (adjust_resource(res, res->start,
> > +                               resource_size(res) + add_size) && 
> > +                               !resource_size(res)) {
> > +                               reset_resource(res);
> > +                       }
> >                 }
> >  out:
> >                 tmp = list;
> > 
> > 
> 
> no, adjust_resource will not change resource size to zero.

Ok. There are four scenarios that adjust_resources_sorted() gets called, three
of which are handled properly and one case is not handled at all.  I think you
are running against the one that is not handled.

So here are the 4 scenarios:
a. non-hotplug resource having children resources
b. non-hotplug resource without children resources
c. hotplug resource with children resources
d. hotplug resource without children resources.

case (a):  resource is in head but not in add_list --
		adjust_resource_sorted() has nothing to do.
		Currently this case is handled properly.

case (b):  resource is not in head and not in add_list --
		adjust_resource_sorted() has nothing to do.
		Currently this case is handled properly.

case (c):  resource is in head and in add_list --
		adjust_resource_sorted() extends the size
		of the resource. And if it fails
		to do so, the failure is ignored. 
		Currently this case is handled properly.
	
case (d):  resource is not in head but in add_list --
		adjust_resource_sorted()  is not handling this
		case properly. 

		If the reason the resource is not in head is because its
		allocation failed, then it must be in the failed list. 
		Since it is in the failed list, adjust_resource_sorted() 
		has nothing to do.

		However if the resource is not in the head because
		it had no child resources to begin with, then
		adjust_resource_sorted() has to increase the size
		of the resource, in order to satisfy
		any future hotplug requests. If it unable 
		to increase the size then it has to reset the 
		resource, which means reset its flags too.

		We need code that handles this case, which should
		solve your problem.

		Here is the code, I think, should take care of your problem.
		Let me know if it works.


diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8e73abf..56ec6f7 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -118,19 +118,18 @@ static inline void reset_resource(struct resource *res)
  *
  * @add_head : head of the list tracking requests requiring additional
  *             resources
- * @head     : head of the list tracking requests with allocated
- *             resources
+ * @fail_head: head of the list tracking requests with failed 
+ *             resource allocation
  *
  * Walk through each element of the add_head and try to procure
  * additional resources for the element, provided the element
- * is in the head list.
+ * is not in the failed list.
  */
 static void adjust_resources_sorted(struct resource_list_x *add_head,
-		struct resource_list *head)
+		struct resource_list_x *fail_head)
 {
 	struct resource *res;
-	struct resource_list_x *list, *tmp, *prev;
-	struct resource_list *hlist;
+	struct resource_list_x *list, *tmp, *prev, *f_list;
 	resource_size_t add_size;
 	int idx;
 
@@ -141,10 +140,10 @@ static void adjust_resources_sorted(struct resource_list_x *add_head,
 		if (!res->flags)
 			goto out;
 
-		/* skip this resource if not found in head list */
-		for (hlist = head->next; hlist && hlist->res != res;
-				hlist = hlist->next);
-		if (!hlist) { /* just skip */
+		/* skip this resource if found in failed list */
+		for (f_list = fail_head->next; f_list && f_list->res != res; 
+				f_list = f_list->next);
+		if (f_list) {
 			prev = list;
 			list = list->next;
 			continue;
@@ -212,7 +211,7 @@ static void __assign_resources_sorted(struct resource_list *head,
 	/* Try to satisfy any additional nice-to-have resource
 		requests */
 	if (add_head)
-		adjust_resources_sorted(add_head, head);
+		adjust_resources_sorted(add_head, fail_head);
 	free_list(resource_list, head);
 }
 

RP

Re: [PATCH] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

On 05/06/2011 06:52 PM, Ram Pai wrote:
> On Fri, May 06, 2011 at 01:22:04PM -0700, Yinghai Lu wrote:
>> On 05/06/2011 01:12 AM, Ram Pai wrote:
>>> On Thu, May 05, 2011 at 12:24:22AM -0700, Yinghai Lu wrote:
>>>>
>>>> During pci remove/rescan testing found:
>>>>
>>>> [  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
>>>> [  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
>>>> [  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
>>>> [  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
>>>> [  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
>>>> [  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
>>>> [  541.199202] pci 0000:c0:03.0: enabling bus mastering
>>>> [  541.199209] pci 0000:c0:03.0: setting latency timer to 64
>>>> [  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
>>>> [  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22
>>>>
>>>> This bug was uncovered by commit
>>>> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
>>>> | Author: Ram Pai <linuxram@us.ibm.com>
>>>> | Date:   Mon Feb 14 17:43:20 2011 -0800
>>>> |
>>>> |    PCI: pre-allocate additional resources to devices only after successful allo
>>>> cation of essential resources.
>>>>
>>>> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size. So it will not get into failed list, and will not be reset there.
>>>>
>>>> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
>>>> bridge, and later if children does not need to IO resource. those bridge
>>>> resources will not need to be allocated. but flags still there.
>>>>
>>>> Add pci_bridge_check_resources() to close the loop.
>>>
>>> How about resetting the resource in  adjust_resources_sorted() if  call to
>>> adjust_resource() fails, and the resource is left with zero size?
>>>
>>> Something like this:
>>>
>>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>> index 8e73abf..77e8454 100644
>>> --- a/drivers/pci/setup-bus.c
>>> +++ b/drivers/pci/setup-bus.c
>>> @@ -157,8 +157,11 @@ static void adjust_resources_sorted(struct resource_list_x *add_head,
>>>                          if(pci_assign_resource(list->dev, idx))
>>>                                 reset_resource(res);
>>>                 } else if (add_size) {
>>> -                       adjust_resource(res, res->start,
>>> -                               resource_size(res) + add_size);
>>> +                       if (adjust_resource(res, res->start,
>>> +                               resource_size(res) + add_size) && 
>>> +                               !resource_size(res)) {
>>> +                               reset_resource(res);
>>> +                       }
>>>                 }
>>>  out:
>>>                 tmp = list;
>>>
>>>
>>
>> no, adjust_resource will not change resource size to zero.
> 
> Ok. There are four scenarios that adjust_resources_sorted() gets called, three
> of which are handled properly and one case is not handled at all.  I think you
> are running against the one that is not handled.
> 
> So here are the 4 scenarios:
> a. non-hotplug resource having children resources
> b. non-hotplug resource without children resources
> c. hotplug resource with children resources
> d. hotplug resource without children resources.
> 
> case (a):  resource is in head but not in add_list --
> 		adjust_resource_sorted() has nothing to do.
> 		Currently this case is handled properly.
> 
> case (b):  resource is not in head and not in add_list --
> 		adjust_resource_sorted() has nothing to do.
> 		Currently this case is handled properly.
> 
> case (c):  resource is in head and in add_list --
> 		adjust_resource_sorted() extends the size
> 		of the resource. And if it fails
> 		to do so, the failure is ignored. 
> 		Currently this case is handled properly.
> 	
> case (d):  resource is not in head but in add_list --
> 		adjust_resource_sorted()  is not handling this
> 		case properly. 
> 
> 		If the reason the resource is not in head is because its
> 		allocation failed, then it must be in the failed list. 
> 		Since it is in the failed list, adjust_resource_sorted() 
> 		has nothing to do.
> 
> 		However if the resource is not in the head because
> 		it had no child resources to begin with, then
> 		adjust_resource_sorted() has to increase the size
> 		of the resource, in order to satisfy
> 		any future hotplug requests. If it unable 
> 		to increase the size then it has to reset the 
> 		resource, which means reset its flags too.
> 
> 		We need code that handles this case, which should
> 		solve your problem.
> 
> 		Here is the code, I think, should take care of your problem.
> 		Let me know if it works.
> 
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 8e73abf..56ec6f7 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -118,19 +118,18 @@ static inline void reset_resource(struct resource *res)
>   *
>   * @add_head : head of the list tracking requests requiring additional
>   *             resources
> - * @head     : head of the list tracking requests with allocated
> - *             resources
> + * @fail_head: head of the list tracking requests with failed 
> + *             resource allocation
>   *
>   * Walk through each element of the add_head and try to procure
>   * additional resources for the element, provided the element
> - * is in the head list.
> + * is not in the failed list.
>   */
>  static void adjust_resources_sorted(struct resource_list_x *add_head,
> -		struct resource_list *head)
> +		struct resource_list_x *fail_head)
>  {
>  	struct resource *res;
> -	struct resource_list_x *list, *tmp, *prev;
> -	struct resource_list *hlist;
> +	struct resource_list_x *list, *tmp, *prev, *f_list;
>  	resource_size_t add_size;
>  	int idx;
>  
> @@ -141,10 +140,10 @@ static void adjust_resources_sorted(struct resource_list_x *add_head,
>  		if (!res->flags)
>  			goto out;

No, should not change to fail_head here.

if one resource is in fail_head, that means that it is get reset already.

so !res->flags will be meet and get out ...

Thanks

Yinghai

>  
> -		/* skip this resource if not found in head list */
> -		for (hlist = head->next; hlist && hlist->res != res;
> -				hlist = hlist->next);
> -		if (!hlist) { /* just skip */
> +		/* skip this resource if found in failed list */
> +		for (f_list = fail_head->next; f_list && f_list->res != res; 
> +				f_list = f_list->next);
> +		if (f_list) {
>  			prev = list;
>  			list = list->next;
>  			continue;
> @@ -212,7 +211,7 @@ static void __assign_resources_sorted(struct resource_list *head,
>  	/* Try to satisfy any additional nice-to-have resource
>  		requests */
>  	if (add_head)
> -		adjust_resources_sorted(add_head, head);
> +		adjust_resources_sorted(add_head, fail_head);
>  	free_list(resource_list, head);
>  }
>  
> 
> RP

[PATCH -v2] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

During pci remove/rescan testing found:

[  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
[  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
[  541.199202] pci 0000:c0:03.0: enabling bus mastering
[  541.199209] pci 0000:c0:03.0: setting latency timer to 64
[  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22

This bug was uncovered by commit
| commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
| Author: Ram Pai <linuxram@us.ibm.com>
| Date:   Mon Feb 14 17:43:20 2011 -0800
|
|    PCI: pre-allocate additional resources to devices only after successful allocation of essential resources.

After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size.
So it will not go through resource_size(res) != 0 path,  and will not be reset there.

The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
bridge, and later if children do not need IO resource. those bridge
resources will not need to be allocated. but flags is still there. that will
confuse the the pci_enable_bridges later.

related code:
| static void assign_requested_resources_sorted(struct resource_list *head,
|                                  struct resource_list_x *fail_head)
| {
|         struct resource *res;
|         struct resource_list *list;
|         int idx;
|
|         for (list = head->next; list; list = list->next) {
|                 res = list->res;
|                 idx = res - &list->dev->resource[0];
|                 if (resource_size(res) && pci_assign_resource(list->dev, idx)) {
| ...
|                         reset_resource(res);
|                 }
|         }
| }

We can not just reset resource there, because will still need to use flag for addition resource allocation afterwards.

At last, We have to add pci_bridge_check_resources() to close the loop.

after patch, will get right result:
[  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
[  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  621.247012] pci 0000:c0:03.0: enabling bus mastering
[  621.247275] pci 0000:c0:03.0: setting latency timer to 64
[  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
[  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
[  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
[  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
[  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
[  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
[  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:

-v2: update description.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>


---
 drivers/pci/setup-bus.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -811,6 +811,27 @@ void __ref pci_bus_size_bridges(struct p
 }
 EXPORT_SYMBOL(pci_bus_size_bridges);
 
+static void __ref pci_bridge_check_resources(struct pci_dev *bridge)
+{
+	struct resource *b_res;
+	int i;
+
+	b_res = &bridge->resource[PCI_BRIDGE_RESOURCES];
+
+	/*
+	 * We set resource flag in pci_bridge_check_ranges() for further
+	 *  allocation for children devices.
+	 * Then if children do not need some kind of resource (like IO), and
+	 *  bridge resource will not be allocated.
+	 * At last, those flags could be left set, that will confuse
+	 *  pci_setup_bridge() and pci_enable_bridge()
+	 * So use reset_resource to clear them.
+	 */
+	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++)
+		if (!resource_size(&b_res[i]))
+			reset_resource(&b_res[i]);
+}
+
 static void __ref __pci_bus_assign_resources(const struct pci_bus *bus,
 					 struct resource_list_x *add_head,
 					 struct resource_list_x *fail_head)
@@ -820,6 +841,9 @@ static void __ref __pci_bus_assign_resou
 
 	pbus_assign_resources_sorted(bus, add_head, fail_head);
 
+	if (bus->self)
+		pci_bridge_check_resources(bus->self);
+
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		b = dev->subordinate;
 		if (!b)
@@ -862,6 +886,8 @@ static void __ref __pci_bridge_assign_re
 	if (!b)
 		return;
 
+	pci_bridge_check_resources((struct pci_dev *)bridge);
+
 	__pci_bus_assign_resources(b, NULL, fail_head);
 
 	switch (bridge->class >> 8) {

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Jesse Barnes]

On Sun, 08 May 2011 00:55:04 -0700
Yinghai Lu <yinghai@kernel.org> wrote:

> 
> During pci remove/rescan testing found:
> 
> [  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
> [  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
> [  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
> [  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
> [  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> [  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
> [  541.199202] pci 0000:c0:03.0: enabling bus mastering
> [  541.199209] pci 0000:c0:03.0: setting latency timer to 64
> [  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> [  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22
> 
> This bug was uncovered by commit
> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
> | Author: Ram Pai <linuxram@us.ibm.com>
> | Date:   Mon Feb 14 17:43:20 2011 -0800
> |
> |    PCI: pre-allocate additional resources to devices only after successful allocation of essential resources.
> 
> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size.
> So it will not go through resource_size(res) != 0 path,  and will not be reset there.
> 
> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
> bridge, and later if children do not need IO resource. those bridge
> resources will not need to be allocated. but flags is still there. that will
> confuse the the pci_enable_bridges later.
> 
> related code:
> | static void assign_requested_resources_sorted(struct resource_list *head,
> |                                  struct resource_list_x *fail_head)
> | {
> |         struct resource *res;
> |         struct resource_list *list;
> |         int idx;
> |
> |         for (list = head->next; list; list = list->next) {
> |                 res = list->res;
> |                 idx = res - &list->dev->resource[0];
> |                 if (resource_size(res) && pci_assign_resource(list->dev, idx)) {
> | ...
> |                         reset_resource(res);
> |                 }
> |         }
> | }
> 
> We can not just reset resource there, because will still need to use flag for addition resource allocation afterwards.
> 
> At last, We have to add pci_bridge_check_resources() to close the loop.
> 
> after patch, will get right result:
> [  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
> [  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
> [  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
> [  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
> [  621.247012] pci 0000:c0:03.0: enabling bus mastering
> [  621.247275] pci 0000:c0:03.0: setting latency timer to 64
> [  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
> [  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
> [  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
> [  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
> [  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
> [  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
> [  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:
> 
> -v2: update description.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Why do I keep getting the feeling that our resource handling code is
made of duct tape and bailing wire?  The fact that we can't just clear
the resource and the flag where we discover the allocation failure
suggests that maybe we should be splitting our bus and device
allocation code a bit more.

Clearly we don't want to allocate resources (especially scarce ones
like I/O space) when we don't actually need them, but I think it's
going to be tough to handle all supported cases without real resource
move support in the core and drivers.

That said, if this doesn't break other cases I guess it's a fairly
minimal fix.

Linus?  Bjorn?  Ram?

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

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Linus Torvalds]

On Mon, May 9, 2011 at 2:20 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
> Linus?  Bjorn?  Ram?

I dunno. The patch really makes me go "that looks *broken*". I really
dislike it. But maybe the crappiness of the patch comes from the
horror that is the current code.

               Linus

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

On 05/09/2011 03:36 PM, Linus Torvalds wrote:
> On Mon, May 9, 2011 at 2:20 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>
>> Linus?  Bjorn?  Ram?
> 
> I dunno. The patch really makes me go "that looks *broken*". I really
> dislike it. But maybe the crappiness of the patch comes from the
> horror that is the current code.

please check if this one is ok.

[PATCH] pci: Clear bridge resource flags if requested size is 0

During pci remove/rescan testing found:

[  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
[  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
[  541.199202] pci 0000:c0:03.0: enabling bus mastering
[  541.199209] pci 0000:c0:03.0: setting latency timer to 64
[  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22

This bug was cause by commit
| commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
| Author: Ram Pai <linuxram@us.ibm.com>
| Date:   Mon Feb 14 17:43:20 2011 -0800
|
|    PCI: pre-allocate additional resources to devices only after successful allocation of essential resources.

After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size.
So it will not go through resource_size(res) != 0 path,  and will not be reset there.

The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
bridge, and later if children do not need IO resource. those bridge
resources will not need to be allocated. but flags is still there. that will
confuse the the pci_enable_bridges later.

related code:
| static void assign_requested_resources_sorted(struct resource_list *head,
|                                  struct resource_list_x *fail_head)
| {
|         struct resource *res;
|         struct resource_list *list;
|         int idx;
|
|         for (list = head->next; list; list = list->next) {
|                 res = list->res;
|                 idx = res - &list->dev->resource[0];
|                 if (resource_size(res) && pci_assign_resource(list->dev, idx)) {
| ...
|                         reset_resource(res);
|                 }
|         }
| }

At last, We have to clear the flags in pbus_size_mem/io and etc.

also need to update adjust_resources_sorted() to handle this special case if
requested_size is 0, but add_size is not 0.

after patch, will get right result:
[  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
[  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  621.247012] pci 0000:c0:03.0: enabling bus mastering
[  621.247275] pci 0000:c0:03.0: setting latency timer to 64
[  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
[  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
[  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
[  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
[  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
[  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
[  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:


Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |   31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -138,9 +138,26 @@ static void adjust_resources_sorted(stru
 	for (list = add_head->next; list;) {
 		res = list->res;
 		/* skip resource that has been reset */
-		if (!res->flags)
+		if (!res->flags && !res->start && !res->end)
 			goto out;
 
+		idx = res - &list->dev->resource[0];
+		add_size = list->add_size;
+		if (!add_size) {
+			dev_warn(&list->dev->dev, "idx %d add_size == 0\n",
+					 idx);
+			goto out;
+		}
+
+		if (!resource_size(res)) {
+			/* need to restore the flag */
+			res->flags = list->flags;
+			res->end = res->start + add_size - 1;
+			if (pci_assign_resource(list->dev, idx))
+				reset_resource(res);
+			goto out;
+		}
+
 		/* skip this resource if not found in head list */
 		for (hlist = head->next; hlist && hlist->res != res;
 				hlist = hlist->next);
@@ -150,16 +167,8 @@ static void adjust_resources_sorted(stru
 			continue;
 		}
 
-		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))
-				reset_resource(res);
-		} else if (add_size) {
-			adjust_resource(res, res->start,
+		adjust_resource(res, res->start,
 				resource_size(res) + add_size);
-		}
 out:
 		tmp = list;
 		prev->next = list = list->next;
@@ -596,6 +605,8 @@ static void pbus_size_io(struct pci_bus
 	b_res->flags |= IORESOURCE_STARTALIGN;
 	if (size1 > size0 && add_head)
 		add_to_list(add_head, bus->self, b_res, size1-size0);
+	if (!size0)
+		b_res->flags = 0;
 }
 
 /**
@@ -693,6 +704,8 @@ static int pbus_size_mem(struct pci_bus
 	b_res->flags |= IORESOURCE_STARTALIGN | mem64_mask;
 	if (size1 > size0 && add_head)
 		add_to_list(add_head, bus->self, b_res, size1-size0);
+	if (!size0)
+		b_res->flags = 0;
 	return 1;
 }
 

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Ram Pai]

On Tue, May 10, 2011 at 06:19:17PM -0700, Yinghai Lu wrote:
> On 05/09/2011 03:36 PM, Linus Torvalds wrote:
> > On Mon, May 9, 2011 at 2:20 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >>
> >> Linus?  Bjorn?  Ram?
> > 
> > I dunno. The patch really makes me go "that looks *broken*". I really
> > dislike it. But maybe the crappiness of the patch comes from the
> > horror that is the current code.
> 
> please check if this one is ok.

I like this approach than the earlier approach because it
closes the subtle bug _introduced_ by my earlier patch,
	commit c8adf9a3e873eddaaec11ac410a99ef6b9656938

However i think the implementation can be made cleaner. Comments below..


> 
> [PATCH] pci: Clear bridge resource flags if requested size is 0
> 
> During pci remove/rescan testing found:
> 
> [  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
> [  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
> [  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
> [  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
> [  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> [  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
> [  541.199202] pci 0000:c0:03.0: enabling bus mastering
> [  541.199209] pci 0000:c0:03.0: setting latency timer to 64
> [  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
> [  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22
> 
> This bug was cause by commit
> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
> | Author: Ram Pai <linuxram@us.ibm.com>
> | Date:   Mon Feb 14 17:43:20 2011 -0800
> |
> |    PCI: pre-allocate additional resources to devices only after successful allocation of essential resources.
> 
> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size.
> So it will not go through resource_size(res) != 0 path,  and will not be reset there.
> 
> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
> bridge, and later if children do not need IO resource. those bridge
> resources will not need to be allocated. but flags is still there. that will
> confuse the the pci_enable_bridges later.
> 
> related code:
> | static void assign_requested_resources_sorted(struct resource_list *head,
> |                                  struct resource_list_x *fail_head)
> | {
> |         struct resource *res;
> |         struct resource_list *list;
> |         int idx;
> |
> |         for (list = head->next; list; list = list->next) {
> |                 res = list->res;
> |                 idx = res - &list->dev->resource[0];
> |                 if (resource_size(res) && pci_assign_resource(list->dev, idx)) {
> | ...
> |                         reset_resource(res);
> |                 }
> |         }
> | }
> 
> At last, We have to clear the flags in pbus_size_mem/io and etc.
> 
> also need to update adjust_resources_sorted() to handle this special case if
> requested_size is 0, but add_size is not 0.
> 
> after patch, will get right result:
> [  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
> [  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
> [  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
> [  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
> [  621.247012] pci 0000:c0:03.0: enabling bus mastering
> [  621.247275] pci 0000:c0:03.0: setting latency timer to 64
> [  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
> [  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
> [  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
> [  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
> [  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
> [  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
> [  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:
> 
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/setup-bus.c |   31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -138,9 +138,26 @@ static void adjust_resources_sorted(stru
>  	for (list = add_head->next; list;) {
>  		res = list->res;
>  		/* skip resource that has been reset */
> -		if (!res->flags)
> +		if (!res->flags && !res->start && !res->end)
>  			goto out;
> 
> +		idx = res - &list->dev->resource[0];


> +		add_size = list->add_size;
> +		if (!add_size) {
> +			dev_warn(&list->dev->dev, "idx %d add_size == 0\n",
> +					 idx);
> +			goto out;
> +		}
This code is not needed. Because add_size will alway be greater than 0. The reason
it is in the add_list means that add_size is greater than 0.


> +
> +		if (!resource_size(res)) {

There is assumption made in various sections of the code that 
->start and ->size cannot be relied upon if ->flags is zero.

Though in this case we know that ->start and ->size are valid
even when ->flags is reset, the fact is not easily
recognizable.


> +			/* need to restore the flag */
> +			res->flags = list->flags;
> +			res->end = res->start + add_size - 1;
> +			if (pci_assign_resource(list->dev, idx))
> +				reset_resource(res);
> +			goto out;
> +		}
> +
>  		/* skip this resource if not found in head list */
>  		for (hlist = head->next; hlist && hlist->res != res;
>  				hlist = hlist->next);
> @@ -150,16 +167,8 @@ static void adjust_resources_sorted(stru
>  			continue;
>  		}
> 
> -		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))
> -				reset_resource(res);
> -		} else if (add_size) {
> -			adjust_resource(res, res->start,
> +		adjust_resource(res, res->start,
>  				resource_size(res) + add_size);
> -		}
>  out:
>  		tmp = list;
>  		prev->next = list = list->next;
> @@ -596,6 +605,8 @@ static void pbus_size_io(struct pci_bus
>  	b_res->flags |= IORESOURCE_STARTALIGN;
>  	if (size1 > size0 && add_head)
>  		add_to_list(add_head, bus->self, b_res, size1-size0);
> +	if (!size0)
> +		b_res->flags = 0;

There is code above which resets the flag in if (!size && !size0) {.. b_res->flags = 0;}

We need to restructure this code to have a single place that resets the flag.

Also I choose to capture the necessary fields of b_res in the resource_list_x structure
and totally reset b_res.

pbus_size_io() function looks very clumsy to begin with. This patch will make it
even more clumsier unless restructured a litte.

>  }
> 
>  /**
> @@ -693,6 +704,8 @@ static int pbus_size_mem(struct pci_bus
>  	b_res->flags |= IORESOURCE_STARTALIGN | mem64_mask;
>  	if (size1 > size0 && add_head)
>  		add_to_list(add_head, bus->self, b_res, size1-size0);
> +	if (!size0)
> +		b_res->flags = 0;

same here..

>  	return 1;
>  }

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Linus Torvalds]

On Thu, May 12, 2011 at 11:06 AM, Ram Pai <linuxram@us.ibm.com> wrote:
>
> I like this approach than the earlier approach because it
> closes the subtle bug _introduced_ by my earlier patch,
>        commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
>
> However i think the implementation can be made cleaner. Comments below..

Ack on all of that. Do we have confirmation that the patch works, though?

                 Linus

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Ram Pai]

On Thu, May 12, 2011 at 11:14:07AM -0700, Linus Torvalds wrote:
> On Thu, May 12, 2011 at 11:06 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> >
> > I like this approach than the earlier approach because it
> > closes the subtle bug _introduced_ by my earlier patch,
> >        commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
> >
> > However i think the implementation can be made cleaner. Comments below..
> 
> Ack on all of that. Do we have confirmation that the patch works, though?

In theory it should work. But I have not tested it yet. I will work with Yinghai and
have a cleaned-up/working/tested patch your way soon.

RP

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

On 05/12/2011 11:14 AM, Linus Torvalds wrote:
> On Thu, May 12, 2011 at 11:06 AM, Ram Pai <linuxram@us.ibm.com> wrote:
>>
>> I like this approach than the earlier approach because it
>> closes the subtle bug _introduced_ by my earlier patch,
>>        commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
>>
>> However i think the implementation can be made cleaner. Comments below..
> 
> Ack on all of that. Do we have confirmation that the patch works, though?
> 

yes. that patch fixes the problem.

Thanks

Yinghai

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

On 05/12/2011 11:06 AM, Ram Pai wrote:
> On Tue, May 10, 2011 at 06:19:17PM -0700, Yinghai Lu wrote:
>> On 05/09/2011 03:36 PM, Linus Torvalds wrote:
>>> On Mon, May 9, 2011 at 2:20 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>>>
>>>> Linus?  Bjorn?  Ram?
>>>
>>> I dunno. The patch really makes me go "that looks *broken*". I really
>>> dislike it. But maybe the crappiness of the patch comes from the
>>> horror that is the current code.
>>
>> please check if this one is ok.
> 
> I like this approach than the earlier approach because it
> closes the subtle bug _introduced_ by my earlier patch,
> 	commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
> 
> However i think the implementation can be made cleaner. Comments below..
> 
> 
>>
>> [PATCH] pci: Clear bridge resource flags if requested size is 0
>>
>> During pci remove/rescan testing found:
>>
>> [  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
>> [  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
>> [  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
>> [  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
>> [  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
>> [  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
>> [  541.199202] pci 0000:c0:03.0: enabling bus mastering
>> [  541.199209] pci 0000:c0:03.0: setting latency timer to 64
>> [  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
>> [  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22
>>
>> This bug was cause by commit
>> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
>> | Author: Ram Pai <linuxram@us.ibm.com>
>> | Date:   Mon Feb 14 17:43:20 2011 -0800
>> |
>> |    PCI: pre-allocate additional resources to devices only after successful allocation of essential resources.
>>
>> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size.
>> So it will not go through resource_size(res) != 0 path,  and will not be reset there.
>>
>> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
>> bridge, and later if children do not need IO resource. those bridge
>> resources will not need to be allocated. but flags is still there. that will
>> confuse the the pci_enable_bridges later.
>>
>> related code:
>> | static void assign_requested_resources_sorted(struct resource_list *head,
>> |                                  struct resource_list_x *fail_head)
>> | {
>> |         struct resource *res;
>> |         struct resource_list *list;
>> |         int idx;
>> |
>> |         for (list = head->next; list; list = list->next) {
>> |                 res = list->res;
>> |                 idx = res - &list->dev->resource[0];
>> |                 if (resource_size(res) && pci_assign_resource(list->dev, idx)) {
>> | ...
>> |                         reset_resource(res);
>> |                 }
>> |         }
>> | }
>>
>> At last, We have to clear the flags in pbus_size_mem/io and etc.
>>
>> also need to update adjust_resources_sorted() to handle this special case if
>> requested_size is 0, but add_size is not 0.
>>
>> after patch, will get right result:
>> [  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
>> [  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
>> [  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
>> [  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
>> [  621.247012] pci 0000:c0:03.0: enabling bus mastering
>> [  621.247275] pci 0000:c0:03.0: setting latency timer to 64
>> [  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
>> [  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
>> [  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
>> [  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
>> [  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
>> [  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
>> [  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:
>>
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  drivers/pci/setup-bus.c |   31 +++++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> Index: linux-2.6/drivers/pci/setup-bus.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/pci/setup-bus.c
>> +++ linux-2.6/drivers/pci/setup-bus.c
>> @@ -138,9 +138,26 @@ static void adjust_resources_sorted(stru
>>  	for (list = add_head->next; list;) {
>>  		res = list->res;
>>  		/* skip resource that has been reset */
>> -		if (!res->flags)
>> +		if (!res->flags && !res->start && !res->end)
>>  			goto out;
>>
>> +		idx = res - &list->dev->resource[0];
> 
> 
>> +		add_size = list->add_size;
>> +		if (!add_size) {
>> +			dev_warn(&list->dev->dev, "idx %d add_size == 0\n",
>> +					 idx);
>> +			goto out;
>> +		}
> This code is not needed. Because add_size will alway be greater than 0. The reason
> it is in the add_list means that add_size is greater than 0.

yes, could be removed.

Just keep there, because the old code checked that.

> 
> 
>> +
>> +		if (!resource_size(res)) {
> 
> There is assumption made in various sections of the code that 
> ->start and ->size cannot be relied upon if ->flags is zero.
> 
> Though in this case we know that ->start and ->size are valid
> even when ->flags is reset, the fact is not easily
> recognizable.
> 
> 
>> +			/* need to restore the flag */
>> +			res->flags = list->flags;
>> +			res->end = res->start + add_size - 1;
>> +			if (pci_assign_resource(list->dev, idx))
>> +				reset_resource(res);
>> +			goto out;
>> +		}
>> +
>>  		/* skip this resource if not found in head list */
>>  		for (hlist = head->next; hlist && hlist->res != res;
>>  				hlist = hlist->next);
>> @@ -150,16 +167,8 @@ static void adjust_resources_sorted(stru
>>  			continue;
>>  		}
>>
>> -		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))
>> -				reset_resource(res);
>> -		} else if (add_size) {
>> -			adjust_resource(res, res->start,
>> +		adjust_resource(res, res->start,
>>  				resource_size(res) + add_size);
>> -		}
>>  out:
>>  		tmp = list;
>>  		prev->next = list = list->next;
>> @@ -596,6 +605,8 @@ static void pbus_size_io(struct pci_bus
>>  	b_res->flags |= IORESOURCE_STARTALIGN;
>>  	if (size1 > size0 && add_head)
>>  		add_to_list(add_head, bus->self, b_res, size1-size0);
>> +	if (!size0)
>> +		b_res->flags = 0;
> 
> There is code above which resets the flag in if (!size && !size0) {.. b_res->flags = 0;}
> 
> We need to restructure this code to have a single place that resets the flag.

should be ok.

> 
> Also I choose to capture the necessary fields of b_res in the resource_list_x structure
> and totally reset b_res.
> 
> pbus_size_io() function looks very clumsy to begin with. This patch will make it
> even more clumsier unless restructured a litte.
> 

maybe later. not in this patch.

try to make this patch touch less lines and path as possible.

Thanks

Yinghai

>>  }
>>
>>  /**
>> @@ -693,6 +704,8 @@ static int pbus_size_mem(struct pci_bus
>>  	b_res->flags |= IORESOURCE_STARTALIGN | mem64_mask;
>>  	if (size1 > size0 && add_head)
>>  		add_to_list(add_head, bus->self, b_res, size1-size0);
>> +	if (!size0)
>> +		b_res->flags = 0;
> 
> same here..
> 
>>  	return 1;
>>  }

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Jesse Barnes]

On Thu, 12 May 2011 11:22:29 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> On Thu, May 12, 2011 at 11:14:07AM -0700, Linus Torvalds wrote:
> > On Thu, May 12, 2011 at 11:06 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> > >
> > > I like this approach than the earlier approach because it
> > > closes the subtle bug _introduced_ by my earlier patch,
> > >        commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
> > >
> > > However i think the implementation can be made cleaner. Comments below..
> > 
> > Ack on all of that. Do we have confirmation that the patch works, though?
> 
> In theory it should work. But I have not tested it yet. I will work with Yinghai and
> have a cleaned-up/working/tested patch your way soon.

Linus, I don't have anything else queued up, so you may as well take
this one directly if you want it in 2.6.39.  It's a regression fix, but
resource changes always make me nervous.  Alternately, I could put it
into 2.6.40 instead, the backport to 2.6.39.x if it survives until
2.6.40-rc2 or so...

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

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Linus Torvalds]

On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
> Linus, I don't have anything else queued up, so you may as well take
> this one directly if you want it in 2.6.39.  It's a regression fix, but
> resource changes always make me nervous.  Alternately, I could put it
> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
> 2.6.40-rc2 or so...

Considering the trouble resource allocation always ends up being, I'd
almost prefer that "mark it for stable and put it in the 2.6.40
queue".

Afaik this problem hasn't actually hit any "normal" users, has it? So ...

                        Linus

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Jesse Barnes]

On Thu, 12 May 2011 12:18:43 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >
> > Linus, I don't have anything else queued up, so you may as well take
> > this one directly if you want it in 2.6.39.  It's a regression fix, but
> > resource changes always make me nervous.  Alternately, I could put it
> > into 2.6.40 instead, the backport to 2.6.39.x if it survives until
> > 2.6.40-rc2 or so...
> 
> Considering the trouble resource allocation always ends up being, I'd
> almost prefer that "mark it for stable and put it in the 2.6.40
> queue".
> 
> Afaik this problem hasn't actually hit any "normal" users, has it? So ...

Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
(at least I don't know of any other reports).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> On Thu, 12 May 2011 12:18:43 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>>
>>> Linus, I don't have anything else queued up, so you may as well take
>>> this one directly if you want it in 2.6.39.  It's a regression fix, but
>>> resource changes always make me nervous.  Alternately, I could put it
>>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
>>> 2.6.40-rc2 or so...
>>
>> Considering the trouble resource allocation always ends up being, I'd
>> almost prefer that "mark it for stable and put it in the 2.6.40
>> queue".
>>
>> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
> 
> Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
> (at least I don't know of any other reports).
> 

please check this one, it should be safe for 2.6.39 ?

Thanks

Yinghai

[PATCH -v3] PCI: Clear bridge resource flags if requested size is 0

During pci remove/rescan testing found:

[  541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  541.141965] pci 0000:c0:03.0:   bridge window [io  0x1000-0x0fff]
[  541.159181] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  541.159540] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  541.179374] pci 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing
[  541.199202] pci 0000:c0:03.0: enabling bus mastering
[  541.199209] pci 0000:c0:03.0: setting latency timer to 64
[  541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io  0x1000-0x0fff])
[  541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22

This bug was caused by commit
| commit c8adf9a3e873eddaaec11ac410a99ef6b9656938
| Author: Ram Pai <linuxram@us.ibm.com>
| Date:   Mon Feb 14 17:43:20 2011 -0800
|
|    PCI: pre-allocate additional resources to devices only after successful allocation of essential resources.

After that commit, pci_hotplug_io_size is changed to additional_io_size from
minium size.
So it will not go through resource_size(res) != 0 path,  and will not be reset.

The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
bridge, and later if children do not need IO resource. those bridge
resources will not need to be allocated. but flags is still there. that will
confuse the the pci_enable_bridges later.

related code:
| static void assign_requested_resources_sorted(struct resource_list *head,
|                                  struct resource_list_x *fail_head)
| {
|         struct resource *res;
|         struct resource_list *list;
|         int idx;
|
|         for (list = head->next; list; list = list->next) {
|                 res = list->res;
|                 idx = res - &list->dev->resource[0];
|                 if (resource_size(res) && pci_assign_resource(list->dev, idx)) {
| ...
|                         reset_resource(res);
|                 }
|         }
| }

At last, We have to clear the flags in pbus_size_mem/io when requested size == 0 and !add_head.
becasue this case it will not go through adjust_resources_sorted().

just make size1 = size0 when !add_head. it will make flags get cleared.

At the same time when requested size == 0, add_size != 0, will still have in head and add_list.
because we do not clear the flags for it.

after patch, will get right result:
[  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
[  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  621.247012] pci 0000:c0:03.0: enabling bus mastering
[  621.247275] pci 0000:c0:03.0: setting latency timer to 64
[  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
[  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
[  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
[  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
[  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
[  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
[  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:

v3: more simple fix. also fix one typo in pbus_size_mem

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -579,7 +579,7 @@ static void pbus_size_io(struct pci_bus
 	}
 	size0 = calculate_iosize(size, min_size, size1,
 			resource_size(b_res), 4096);
-	size1 = !add_size? size0:
+	size1 = (!add_head || (add_head && !add_size)) ? size0 :
 		calculate_iosize(size, min_size+add_size, size1,
 			resource_size(b_res), 4096);
 	if (!size0 && !size1) {
@@ -677,7 +677,7 @@ static int pbus_size_mem(struct pci_bus
 		align += aligns[order];
 	}
 	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
-	size1 = !add_size ? size :
+	size1 = (!add_head || (add_head && !add_size)) ? size0 :
 		calculate_memsize(size, min_size+add_size, 0,
 				resource_size(b_res), min_align);
 	if (!size0 && !size1) {

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Ram Pai]

On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> > On Thu, 12 May 2011 12:18:43 -0700
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> >> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >>>
> >>> Linus, I don't have anything else queued up, so you may as well take
> >>> this one directly if you want it in 2.6.39.  It's a regression fix, but
> >>> resource changes always make me nervous.  Alternately, I could put it
> >>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
> >>> 2.6.40-rc2 or so...
> >>
> >> Considering the trouble resource allocation always ends up being, I'd
> >> almost prefer that "mark it for stable and put it in the 2.6.40
> >> queue".
> >>
> >> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
> > 
> > Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
> > (at least I don't know of any other reports).
> > 
> 
> please check this one, it should be safe for 2.6.39 ?

>  	size0 = calculate_iosize(size, min_size, size1,
>  			resource_size(b_res), 4096);
> -	size1 = !add_size? size0:
> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
>  		calculate_iosize(size, min_size+add_size, size1,
>  			resource_size(b_res), 4096);

This solves the problem you encountered.

But, I think, it still does not fix the following scenario:

adjust_resource() failing to allocate additional resource to a hotplug bridge
that has no children. In this case  ->flags of that 'struct resource'
continues to be set even when no resource is allocated to that hot-plug bridge.

RP

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Yinghai Lu]

On 05/16/2011 12:59 AM, Ram Pai wrote:
> On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
>> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
>>> On Thu, 12 May 2011 12:18:43 -0700
>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>
>>>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>>>>
>>>>> Linus, I don't have anything else queued up, so you may as well take
>>>>> this one directly if you want it in 2.6.39.  It's a regression fix, but
>>>>> resource changes always make me nervous.  Alternately, I could put it
>>>>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
>>>>> 2.6.40-rc2 or so...
>>>>
>>>> Considering the trouble resource allocation always ends up being, I'd
>>>> almost prefer that "mark it for stable and put it in the 2.6.40
>>>> queue".
>>>>
>>>> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
>>>
>>> Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
>>> (at least I don't know of any other reports).
>>>
>>
>> please check this one, it should be safe for 2.6.39 ?
> 
>>  	size0 = calculate_iosize(size, min_size, size1,
>>  			resource_size(b_res), 4096);
>> -	size1 = !add_size? size0:
>> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
>>  		calculate_iosize(size, min_size+add_size, size1,
>>  			resource_size(b_res), 4096);
> 
> This solves the problem you encountered.
> 
> But, I think, it still does not fix the following scenario:
> 
> adjust_resource() failing to allocate additional resource to a hotplug bridge
> that has no children. In this case  ->flags of that 'struct resource'
> continues to be set even when no resource is allocated to that hot-plug bridge.
> 
that case: requested_size will be 0, but add_size will not be zero.

res->flags is not cleared in pbus_size_xx, so it will be put into head.
so it will go through first path.
...
                if (!resource_size(res) && add_size) {
                         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);
                }

and if it fails to get assign, the flags will get clear in reset_resource.

so it should be ok. and testing in one my setup show those flags get clear correctly and does not emit any warning.

Thanks

Yinghai

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Ram Pai]

On Mon, May 16, 2011 at 01:55:38PM -0700, Yinghai Lu wrote:
> On 05/16/2011 12:59 AM, Ram Pai wrote:
> > On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
> >> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> >>> On Thu, 12 May 2011 12:18:43 -0700
> >>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >>>
> >>>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >>>>>
> >>>>> Linus, I don't have anything else queued up, so you may as well take
> >>>>> this one directly if you want it in 2.6.39.  It's a regression fix, but
> >>>>> resource changes always make me nervous.  Alternately, I could put it
> >>>>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
> >>>>> 2.6.40-rc2 or so...
> >>>>
> >>>> Considering the trouble resource allocation always ends up being, I'd
> >>>> almost prefer that "mark it for stable and put it in the 2.6.40
> >>>> queue".
> >>>>
> >>>> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
> >>>
> >>> Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
> >>> (at least I don't know of any other reports).
> >>>
> >>
> >> please check this one, it should be safe for 2.6.39 ?
> > 
> >>  	size0 = calculate_iosize(size, min_size, size1,
> >>  			resource_size(b_res), 4096);
> >> -	size1 = !add_size? size0:
> >> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
> >>  		calculate_iosize(size, min_size+add_size, size1,
> >>  			resource_size(b_res), 4096);
> > 
> > This solves the problem you encountered.
> > 
> > But, I think, it still does not fix the following scenario:
> > 
> > adjust_resource() failing to allocate additional resource to a hotplug bridge
> > that has no children. In this case  ->flags of that 'struct resource'
> > continues to be set even when no resource is allocated to that hot-plug bridge.
> > 
> that case: requested_size will be 0, but add_size will not be zero.
> 
> res->flags is not cleared in pbus_size_xx, so it will be put into head.
> so it will go through first path.
> ...
>                 if (!resource_size(res) && add_size) {
>                          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);
>                 }
> 
> and if it fails to get assign, the flags will get clear in reset_resource.
> 
> so it should be ok. and testing in one my setup show those flags get clear correctly and does not emit any warning.


Ack. You are right.

Linus/Jesse:  can we consider this patch for 2.6.39? It is the simplest fix to the problem.

Reviewed-by: Ram Pai <linuxram@us.ibm.com>

RP

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Jesse Barnes]

On Mon, 16 May 2011 15:36:21 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> On Mon, May 16, 2011 at 01:55:38PM -0700, Yinghai Lu wrote:
> > On 05/16/2011 12:59 AM, Ram Pai wrote:
> > > On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
> > >> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> > >>> On Thu, 12 May 2011 12:18:43 -0700
> > >>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >>>
> > >>>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes
> > >>>> <jbarnes@virtuousgeek.org> wrote:
> > >>>>>
> > >>>>> Linus, I don't have anything else queued up, so you may as
> > >>>>> well take this one directly if you want it in 2.6.39.  It's a
> > >>>>> regression fix, but resource changes always make me nervous.
> > >>>>> Alternately, I could put it into 2.6.40 instead, the backport
> > >>>>> to 2.6.39.x if it survives until 2.6.40-rc2 or so...
> > >>>>
> > >>>> Considering the trouble resource allocation always ends up
> > >>>> being, I'd almost prefer that "mark it for stable and put it
> > >>>> in the 2.6.40 queue".
> > >>>>
> > >>>> Afaik this problem hasn't actually hit any "normal" users, has
> > >>>> it? So ...
> > >>>
> > >>> Sounds good, thanks.  Yeah I don't think it's hit anyone but
> > >>> Yinghai (at least I don't know of any other reports).
> > >>>
> > >>
> > >> please check this one, it should be safe for 2.6.39 ?
> > > 
> > >>  	size0 = calculate_iosize(size, min_size, size1,
> > >>  			resource_size(b_res), 4096);
> > >> -	size1 = !add_size? size0:
> > >> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
> > >>  		calculate_iosize(size, min_size+add_size, size1,
> > >>  			resource_size(b_res), 4096);
> > > 
> > > This solves the problem you encountered.
> > > 
> > > But, I think, it still does not fix the following scenario:
> > > 
> > > adjust_resource() failing to allocate additional resource to a
> > > hotplug bridge that has no children. In this case  ->flags of
> > > that 'struct resource' continues to be set even when no resource
> > > is allocated to that hot-plug bridge.
> > > 
> > that case: requested_size will be 0, but add_size will not be zero.
> > 
> > res->flags is not cleared in pbus_size_xx, so it will be put into
> > head. so it will go through first path.
> > ...
> >                 if (!resource_size(res) && add_size) {
> >                          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);
> >                 }
> > 
> > and if it fails to get assign, the flags will get clear in
> > reset_resource.
> > 
> > so it should be ok. and testing in one my setup show those flags
> > get clear correctly and does not emit any warning.
> 
> 
> Ack. You are right.
> 
> Linus/Jesse:  can we consider this patch for 2.6.39? It is the
> simplest fix to the problem.
> 
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>

I'll queue it up for 2.6.40 with stable cc'd.

Thanks,
Jesse

Re: [PATCH -v2] pci: Check bridge resources after resource allocation.

[Linus Torvalds]

On Mon, May 16, 2011 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
> I'll queue it up for 2.6.40 with stable cc'd.

I ended up taking it after all..

                   Linus