pci: fix pci_find_bus().

PATCH Superseded Isaku Yamahata 3 participants 6 messages Apr 9, 2010 – Apr 20, 2010

lore

Share & tools

lore

Isaku Yamahata

author

2010-04-09 10:16 UTC #0 lore raw

PATCH 1/1 pci: fix pci_find_bus().

When looking down child bus, it looked parent bridge's bus number.
It should look child bridge's.

Cc Blue Swirl <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

---
 hw/pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

hw/pci.c +2 -2

index 0dbca17..2f6907b 100644
@@ -1557,9 +1557,9 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 
     /* try child bus */
     QLIST_FOREACH(sec, &bus->child, sibling) {
-        if (!bus->parent_dev /* pci host bridge */
+        if (!sec->parent_dev /* pci host bridge */
             || (pci_bus_num(sec) <= bus_num &&
-                bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
+                bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
             ret = pci_find_bus(sec, bus_num);
             if (ret) {
                 return ret;

Signature · 1 line

1.6.6.1

Isaku Yamahata

author

2010-04-09 23:51 UTC #1 lore raw

Oh I sent out the wrong one. This is the correct one. Sorry for noise.

Quoted 1 line

From 370c23c837070f78ca5715f032eacbf6747d42fb Mon Sep 17 00:00:00 2001

Message-Id: <370c23c837070f78ca5715f032eacbf6747d42fb.1270856742.git.yamahata@valinux.co.jp>
In-Reply-To: <[email protected]>
References: <[email protected]>
From: Isaku Yamahata <[email protected]>
Date: Fri, 9 Apr 2010 19:09:35 +0900
Subject: [PATCH] pci: fix pci_find_bus().

When looking down child bus, it should look parent bridge's bus number, not child bus's.

Cc Blue Swirl <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

---
 hw/pci.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

hw/pci.c +4 -4

index 0dbca17..b6e6bbe 100644
@@ -1556,10 +1556,10 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
     }
 
     /* try child bus */
-    QLIST_FOREACH(sec, &bus->child, sibling) {
-        if (!bus->parent_dev /* pci host bridge */
-            || (pci_bus_num(sec) <= bus_num &&
-                bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
+    if (!bus->parent_dev /* pci host bridge */
+        || (pci_bus_num(bus) <= bus_num &&
+            bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
+        QLIST_FOREACH(sec, &bus->child, sibling) {
             ret = pci_find_bus(sec, bus_num);
             if (ret) {
                 return ret;

Signature · 5 lines

1.6.6.1


-- 
yamahata

Michael S. Tsirkin

2010-04-11 10:55 UTC #2 lore raw

Quoted 26 lines · Isaku Yamahata

Oh I sent out the wrong one. This is the correct one. Sorry for noise.
Quoted 1 line
From 370c23c837070f78ca5715f032eacbf6747d42fb Mon Sep 17 00:00:00 2001
Message-Id: <370c23c837070f78ca5715f032eacbf6747d42fb.1270856742.git.yamahata@valinux.co.jp>
In-Reply-To: <[email protected]>
References: <[email protected]>
From: Isaku Yamahata <[email protected]>
Date: Fri, 9 Apr 2010 19:09:35 +0900
Subject: [PATCH] pci: fix pci_find_bus().
When looking down child bus, it should look parent bridge's bus number, not child bus's.
Cc Blue Swirl <[email protected]>
Cc "Michael S. Tsirkin" <[email protected]>
Signed-off-by Isaku Yamahata <[email protected]>
---
 hw/pci.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
hw/pci.c +0 -0
index 0dbca17..b6e6bbe 100644

hw/pci.c PCIBus *pci_find_bus(PCIBus *bus, int bus_num) · line 1560

Quoted 9 lines

hw/pci.c +2 -4

@@ -1556,10 +1556,10 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
     }
 
     /* try child bus */
-    QLIST_FOREACH(sec, &bus->child, sibling) {
-        if (!bus->parent_dev /* pci host bridge */
-            || (pci_bus_num(sec) <= bus_num &&
-                bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
+    if (!bus->parent_dev /* pci host bridge */
+        || (pci_bus_num(bus) <= bus_num &&

Better use PCI_SECONDARY_BUS here directly, we know parent_dev is non-NULL.

hw/pci.c PCIBus *pci_find_bus(PCIBus *bus, int bus_num) · line 1565

Quoted 5 lines

hw/pci.c +2 -0

+            bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
+        QLIST_FOREACH(sec, &bus->child, sibling) {
             ret = pci_find_bus(sec, bus_num);
             if (ret) {
                 return ret;

What do you think about converting code to loop
as I suggested earlier?

Quoted 6 lines

Signature · 5 lines
1.6.6.1


-- 
yamahata

Isaku Yamahata

author

2010-04-12 03:01 UTC #3 lore raw

PATCH v2 1/1 pci: fix pci_find_bus().

When looking down child bus, it should look parent bridge's bus number, not child bus's.
Optimized tail recursion and style fix.

Cc Blue Swirl <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

---
 hw/pci.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

hw/pci.c +16 -9

index 2355232..6c0cc7b 100644
@@ -1546,23 +1546,30 @@ static void pci_bridge_write_config(PCIDevice *d,
 
 PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 {
-    PCIBus *sec, *ret;
+    PCIBus *sec;
 
-    if (!bus)
+    if (!bus) {
         return NULL;
+    }
 
     if (pci_bus_num(bus) == bus_num) {
         return bus;
     }
 
     /* try child bus */
-    QLIST_FOREACH(sec, &bus->child, sibling) {
-        if (!bus->parent_dev /* pci host bridge */
-            || (pci_bus_num(sec) <= bus_num &&
-                bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
-            ret = pci_find_bus(sec, bus_num);
-            if (ret) {
-                return ret;
+    if (!bus->parent_dev /* host pci bridge */ ||
+        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
+         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
+        for (; bus; bus = sec) {
+            QLIST_FOREACH(sec, &bus->child, sibling) {
+                assert(sec->parent_dev);
+                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
+                    return sec;
+                }
+                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
+                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
+                    break;
+                }
             }
         }
     }

Signature · 1 line

1.6.6.1

Blue Swirl

Applied

2010-04-20 17:19 UTC #4 lore raw

Thanks, applied.

Quoted 79 lines · Isaku Yamahata

When looking down child bus, it should look parent bridge's
bus number, not child bus's.

Optimized tail recursion and style fix.

Cc Blue Swirl <[email protected]>

Cc "Michael S. Tsirkin" <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

---

 hw/pci.c |   25 ++++++++++++++++---------
  1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c

index 2355232..6c0cc7b 100644

--- a/hw/pci.c
+++ b/hw/pci.c

(quoted diff) +5 -3

@@ -1546,23 +1546,30 @@ static void pci_bridge_write_config(PCIDevice *d,
 
 
  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 
 {
 -    PCIBus *sec, *ret;
 +    PCIBus *sec;
 
 
-    if (!bus)
 +    if (!bus) {
         return NULL;
 +    }
 
 
     if (pci_bus_num(bus) == bus_num) {
         return bus;
 
     }
 
     /* try child bus */
 -    QLIST_FOREACH(sec, &bus->child, sibling) {
 -        if (!bus->parent_dev /* pci host bridge */
 -            || (pci_bus_num(sec) <= bus_num &&
 -                bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
 
-            ret = pci_find_bus(sec, bus_num);
 -            if (ret) {
 -                return ret;
 +    if (!bus->parent_dev /* host pci bridge */ ||
 +        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
 
+         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
 
+        for (; bus; bus = sec) {
 
+            QLIST_FOREACH(sec, &bus->child, sibling) {
 
+                assert(sec->parent_dev);
 +                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
 +                    return sec;
 +                }
 +                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
 
+                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
 +                    break;
 +                }
             }
         }
     }
 
--
 1.6.6.1

Michael S. Tsirkin

2010-04-11 10:50 UTC #5 lore raw

In reply to Isaku Yamahata

Quoted 16 lines · Isaku Yamahata

When looking down child bus, it looked parent bridge's bus number.
It should look child bridge's.
Cc Blue Swirl <[email protected]>
Cc "Michael S. Tsirkin" <[email protected]>
Signed-off-by Isaku Yamahata <[email protected]>
---
 hw/pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
hw/pci.c +0 -0
index 0dbca17..2f6907b 100644

hw/pci.c PCIBus *pci_find_bus(PCIBus *bus, int bus_num) · line 1560

Quoted 5 lines

hw/pci.c +1 -1

@@ -1557,9 +1557,9 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 
     /* try child bus */
     QLIST_FOREACH(sec, &bus->child, sibling) {
-        if (!bus->parent_dev /* pci host bridge */
+        if (!sec->parent_dev /* pci host bridge */

I don't understand this first test. As far as I can tell secondary bus must always have a device, as set by pci_register_secondary_bus. Should this
rather be assert(sec->parent_dev)?

hw/pci.c PCIBus *pci_find_bus(PCIBus *bus, int bus_num) · line 1561

Quoted 1 line

hw/pci.c +0 -0

             || (pci_bus_num(sec) <= bus_num &&

And so the above should just use sec->parent_dev->config[PCI_SECONDARY_BUS] instead of a wrapper that tests sec->parent_dev.

Quoted 6 lines

hw/pci.c +1 -1

-                bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
+                bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
 
 
             ret = pci_find_bus(sec, bus_num);

             if (ret) {

I think that in the above, we can just as well do return pci_find_bus() - if multiple children claim the same bus range, on real hardware only one of them will claim the transaction.

Quoted 1 line

                 return ret;

I find the use of recursion here confusing. Since as pointed out above this can be a tail recursion, it can easily be converted to a loop.
How does the below look, for example?
Note: completely untested, likely broken:

hw/pci.c +17 ~~-10~~

index b6abd67..e9d6def 100644
@@ -1546,26 +1546,33 @@ static void pci_bridge_write_config(PCIDevice *d,
 
 PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 {
-    PCIBus *sec, *ret;
+    PCIBus *sec;
+    bool found;
 
-    if (!bus)
+    if (!bus) {
         return NULL;
+    }
 
     if (pci_bus_num(bus) == bus_num) {
         return bus;
     }
 
     /* try child bus */
-    QLIST_FOREACH(sec, &bus->child, sibling) {
-        if (!bus->parent_dev /* pci host bridge */
-            || (pci_bus_num(sec) <= bus_num &&
-                bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS]) ) {
-            ret = pci_find_bus(sec, bus_num);
-            if (ret) {
-                return ret;
+    do {
+	 found = false;
+        QLIST_FOREACH(sec, &bus->child, sibling) {
+            assert(sec->parent_dev);
+            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
+                return sec;
+            }
+            if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
+                bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
+                bus = sec;
+                found = true;
+                break;
             }
         }
-    }
+    } while (found);
 
     return NULL;
 }

Quoted 3 lines

Signature · 1 line
1.6.6.1