PCI: Do not touch siblings in pci_assign_unassigned_bridge_resources

pci_assign_unassigned_bridge_resources is used to assign resources below a hotplug bridge. If the first attempt fails it will release some resources and try again. If a resource allocation on the hotplug bridge itself fails then pci_assign_unassigned_bridge_resources will invoke pci_bus_release_bridge_resources on the parent bus of the hotplug bridge. This in turn will release resources assigned to siblings of the hotplug bridge which may already be in use.

This patch checks for this case and prevents pci_bus_release_bridge_resources to be invoked on the parent bus.

The problem can be reproduced by having two sibling hotplug bridges A and B. The problem will occour if the parent of A and B does not have enough resources to satisfy window allocations for B during a hotplug event.

Signed-off-by Andreas Noever <[email protected]>

---

I must admit that I do not fully understand how pci_assign_unassigned_bridge_resources works. Under which scenario would the second allocation attempt be successful?

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

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

index 138bdd6..2e418d6 100644
@@ -1560,6 +1560,12 @@ void __init pci_assign_unassigned_resources(void)
 		pci_assign_unassigned_root_bus_resources(root_bus);
 }
 
+/**
+ * pci_assign_unassigned_bridge_resources - greenfield resource allocation
+ *
+ * Try to assign io and memory resources on and below @bridge. The caller must
+ * ensure that no device below @bridge is active.
+ */
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 {
 	struct pci_bus *parent = bridge->subordinate;

@@ -1567,7 +1573,7 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 					want additional resources */
 	int tried_times = 0;
 	LIST_HEAD(fail_head);
-	struct pci_dev_resource *fail_res;
+	struct pci_dev_resource *fail_res, *tmp;
 	int retval;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
 				  IORESOURCE_PREFETCH;

@@ -1594,10 +1600,21 @@ again:
 	 * Try to release leaf bridge's resources that doesn't fit resource of
 	 * child device under that bridge
 	 */
-	list_for_each_entry(fail_res, &fail_head, list)
+	list_for_each_entry_safe(fail_res, tmp, &fail_head, list) {
+		 /*
+		  * The allocation of the mem/io window of the top level bridge
+		  * can fail.  Without the following check we would release our
+		  * siblings' resources.
+		  */
+		if (fail_res->dev == bridge) {
+			list_del(&fail_res->list);
+			kfree(fail_res);
+			continue;
+		}
 		pci_bus_release_bridge_resources(fail_res->dev->bus,
 						 fail_res->flags & type_mask,
 						 whole_subtree);
+	}
 
 	/* restore size and flags */
 	list_for_each_entry(fail_res, &fail_head, list) {

Signature · 1 line

2.0.0

[+cc Yinghai]

Quoted 4 lines · Andreas Noever

The problem can be reproduced by having two sibling hotplug bridges A and B. The problem will occour if the parent of A and B does not have enough resources to satisfy window allocations for B during a hotplug event.

I don't understand how all this works either. Yinghai?

We definitely don't want to release resources that are already in use. Can you review and ack or nack this?

Quoted 7 lines · Bjorn Helgaas

[+cc Yinghai]

Quoted 4 lines · Andreas Noever

The problem can be reproduced by having two sibling hotplug bridges A and B. The problem will occour if the parent of A and B does not have enough resources to satisfy window allocations for B during a hotplug event.

Quoted 4 lines

I don't understand how all this works either. Yinghai?

We definitely don't want to release resources that are already in use. Can you review and ack or nack this?

Hi Andreas,

Can you check if attached patch fix the problem on your test case?

In some case, if we can not assign pref mmio properly for the bridge, we may need to even clear non-pref mmio for the bridge.

Thanks

Yinghai

On Wed, Jun 18, 2014 at 1:39 AM, Yinghai Lu <[email protected]> wrote:
It seems to fix the testcase (no unwanted resources are released). But why do you reassign bus and not just skip the top level bridge? If one of the allocations below bridge failed then a resource of that device will be in fail_res and bridge->subordinate will get released anyways? Also by not removing fail_res from the list you trigger the code in the next loop for the top level bridge (in particular the res->flags = 0 line looks dangerous).

Could you explain why this function attempts to assign resources two times? In which scenario will a second attempt be successful?

Quoted 8 lines · Andreas Noever

On Wed, Jun 18, 2014 at 1:39 AM, Yinghai Lu <[email protected]> wrote:
It seems to fix the testcase (no unwanted resources are released). But why do you reassign bus and not just skip the top level bridge? If one of the allocations below bridge failed then a resource of that device will be in fail_res and bridge->subordinate will get released anyways? Also by not removing fail_res from the list you trigger the code in the next loop for the top level bridge (in particular the res->flags = 0 line looks dangerous).

Should not be dangerous, just second try.

Quoted 2 lines

Could you explain why this function attempts to assign resources two times? In which scenario will a second attempt be successful?

For example, at first mmio is assigned (by firmware), but pref mmio fails, then before second try, mmio get cleared, then we could separate mmio and mmio pref. So need to try again for pref mmio.

Also I missed one MEM_64 for hotplug path.

So we need two patches.

Thanks

Yinghai

Subject: [PATCH] pci: Don't release sibiling bridge resources

On hotplug case, we should not touch sibling bridges that is out side of the slots.

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

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

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

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

@@ -1676,10 +1676,16 @@ again:
 	 * Try to release leaf bridge's resources that doesn't fit resource of
 	 * child device under that bridge
 	 */
-	list_for_each_entry(fail_res, &fail_head, list)
-		pci_bus_release_bridge_resources(fail_res->dev->bus,
+	list_for_each_entry(fail_res, &fail_head, list) {
+		struct pci_bus *bus = fail_res->dev->bus;
+
+		if (fail_res->dev == bridge)
+			bus = bridge->subordinate;
+
+		pci_bus_release_bridge_resources(bus,
 						 fail_res->flags & type_mask,
 						 whole_subtree);
+	}
 
 	/* restore size and flags */
 	list_for_each_entry(fail_res, &fail_head, list) {

Subject: [PATCH] pci: Add back missing MEM_64 check for hotplug path

We miss that in
| commit 5b28541552ef5eeffc41d6936105f38c2508e566
| PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
for pci hotplug path.

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

---
 drivers/pci/setup-bus.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

@@ -1652,7 +1652,7 @@ void pci_assign_unassigned_bridge_resour
 	struct pci_dev_resource *fail_res;
 	int retval;
 	unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
-				  IORESOURCE_PREFETCH;
+				  IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
 
 again:
 	__pci_bus_size_bridges(parent, &add_list);

Quoted 1 line

Should not be dangerous, just second try.

I still don't understand this. Why do we set "res->flags = 0"? That clears out the resource type. Where do we figure out the type of "res" again?

Quoted 6 lines

Subject: [PATCH] pci: Add back missing MEM_64 check for hotplug path

We miss that in
| commit 5b28541552ef5eeffc41d6936105f38c2508e566
| PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources
for pci hotplug path.

This changelog is useless. I don't have time to spend a few hours figuring out why we want this change.