[PATCH 0/8] PCI: cleanup pci_scan_bridge

[PATCH 0/8] PCI: cleanup pci_scan_bridge

[Andreas Noever]

I have been reading the code around pci_scan_bridge and noticed a few oddities.
These patches try to cleanup some of the these things.

Most of the patches make also sense in isolation, but some make more sense with
the previous ones applied as well. None of the patches fix any observed bugs.
Patch 5 fixes a (probably harmless) dmesg.

The changes related to CardBus probing are completely untested.

Andreas

Andreas Noever (8):
  PCI: Increment max correctly in pci_scan_bridge.
  PCI: Clarify the "scan anyway" comment in pci_scan_bridge.
  PCI: Assign CardBus bus number only during the second pass.
  PCI: Use request_resource_conflict instead of insert_ for bus numbers.
  PCI: Make sure bus number resources stay within their parents bounds.
  PCI: Remove pci_fixup_parent_subordinate_busnr.
  PCI: Check for child busses which use more bus numbers than allocated.
  PCI: Don't scan random busses in pci_scan_bridge.

 drivers/pci/probe.c | 78 +++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 47 deletions(-)

-- 
1.8.5.3

[PATCH 1/8] PCI: Increment max correctly in pci_scan_bridge.

[Andreas Noever]

This patch fixes two small issues:
 - If pci_add_new_bus fails then max must not be incremented. Otherwise
   an incorrect value is returned from pci_scan_bridge().
 - If the bus is already present, then max must be incremented. I think
   that this case should only be hit if we trigger a manual rescan of a
   CardBus bridge.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 04796c0..78caade 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -852,11 +852,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 		 * this case we only re-scan this bus. */
 		child = pci_find_bus(pci_domain_nr(bus), max+1);
 		if (!child) {
-			child = pci_add_new_bus(bus, dev, ++max);
+			child = pci_add_new_bus(bus, dev, max+1);
 			if (!child)
 				goto out;
-			pci_bus_insert_busn_res(child, max, 0xff);
+			pci_bus_insert_busn_res(child, max+1, 0xff);
 		}
+		max++;
 		buses = (buses & 0xff000000)
 		      | ((unsigned int)(child->primary)     <<  0)
 		      | ((unsigned int)(child->busn_res.start)   <<  8)
-- 
1.8.5.3

[PATCH 2/8] PCI: Clarify the "scan anyway" comment in pci_scan_bridge.

[Andreas Noever]

Initially when we encountered a bus that was already present we skipped
it. Since 74710ded8e16 'PCI: always scan child buses' we continue
scanning in order to allow user triggered rescans of already existing
busses.

The old comment suggested that the reason for continuing the scan is a
bug in the i450NX chipset. This is not the case.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 78caade..cf05b3e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -805,11 +805,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 			goto out;
 
 		/*
-		 * If we already got to this bus through a different bridge,
-		 * don't re-add it. This can happen with the i450NX chipset.
-		 *
-		 * However, we continue to descend down the hierarchy and
-		 * scan remaining child buses.
+		 * The bus might already exist for two reasons: Either we are
+		 * rescanning the bus or the bus is reachable through more than
+		 * one bridge. The second case can happen with the i450NX
+		 * chipset.
 		 */
 		child = pci_find_bus(pci_domain_nr(bus), secondary);
 		if (!child) {
-- 
1.8.5.3

[PATCH 3/8] PCI: Assign CardBus bus number only during the second pass.

[Andreas Noever]

Right now the CardBus code in pci_scan_bridge is executed during both
passes. Since we always allocate the bus number ourselves it makes sense
to put it into the second pass.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cf05b3e..bed1934 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -831,7 +831,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 		 * do in the second pass.
 		 */
 		if (!pass) {
-			if (pcibios_assign_all_busses() || broken)
+			if (pcibios_assign_all_busses() || broken || is_cardbus)
 				/* Temporarily disable forwarding of the
 				   configuration cycles on all bridges in
 				   this bus segment to avoid possible
-- 
1.8.5.3

[PATCH 4/8] PCI: Use request_resource_conflict instead of insert_ for bus numbers.

[Andreas Noever]

If a conflict happens during insert_resource_conflict and all conflicts
fit within the newly inserted resource then they will become children of
the new resource. This is almost certainly not what we want for bus
numbers.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index bed1934..fe6e10e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1852,7 +1852,7 @@ int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
 		res->flags |= IORESOURCE_PCI_FIXED;
 	}
 
-	conflict = insert_resource_conflict(parent_res, res);
+	conflict = request_resource_conflict(parent_res, res);
 
 	if (conflict)
 		dev_printk(KERN_DEBUG, &b->dev,
-- 
1.8.5.3

[PATCH 5/8] PCI: Make sure bus number resources stay within their parents bounds.

[Andreas Noever]

Right now we use 0xff for busn_res.end when probing and later reduce it
to the value that is actually used. This does not work if a parent
bridge has already a lower subordinate value. For example during hotplug of a
new bridge below an already configured bridge the following message is
printed from pci_bus_insert_busn_res:

pci_bus 0000:06: busn_res: can not insert [bus 06-ff] under [bus 05-9b]
(conflicts with (null) [bus 05-9b])

This patch clamps the bus range to that of the parent and also ensures
that we do not exceed the parents range when assigning the final
subordinate value.

We also check that busses configured by the firmware fit into their
parents bounds.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index fe6e10e..42ee0c0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -782,7 +782,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 	/* Check if setup is sensible at all */
 	if (!pass &&
 	    (primary != bus->number || secondary <= bus->number ||
-	     secondary > subordinate)) {
+	     secondary > subordinate || subordinate > bus->busn_res.end)) {
 		dev_info(&dev->dev, "bridge configuration invalid ([bus %02x-%02x]), reconfiguring\n",
 			 secondary, subordinate);
 		broken = 1;
@@ -854,7 +854,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 			child = pci_add_new_bus(bus, dev, max+1);
 			if (!child)
 				goto out;
-			pci_bus_insert_busn_res(child, max+1, 0xff);
+			pci_bus_insert_busn_res(child, max+1,
+						bus->busn_res.end);
 		}
 		max++;
 		buses = (buses & 0xff000000)
@@ -927,6 +928,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 		/*
 		 * Set the subordinate bus number to its real value.
 		 */
+		if (max > bus->busn_res.end) {
+			dev_warn(&dev->dev,
+				"bridge has max busn %02x, but can only accomodate up to %02x\n",
+				max, bus->busn_res.end);
+			max = bus->busn_res.end;
+		}
 		pci_bus_update_busn_res_end(child, max);
 		pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max);
 	}
-- 
1.8.5.3

[PATCH 6/8] PCI: Remove pci_fixup_parent_subordinate_busnr.

[Andreas Noever]

The function has no effect.

If pcibios_assign_all_busses() is not set then the function does nothing.

If it is set then in pci_scan_bridge we are always in the branch where
we assign the bus numbers ourselves and the subordinate values of all
parent busses will be set to 0xff since that is what they inherited from
their parent bus and ultimately from the root bus.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 42ee0c0..90d5c48 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -731,22 +731,6 @@ struct pci_bus *__ref pci_add_new_bus(struct pci_bus *parent, struct pci_dev *de
 	return child;
 }
 
-static void pci_fixup_parent_subordinate_busnr(struct pci_bus *child, int max)
-{
-	struct pci_bus *parent = child->parent;
-
-	/* Attempts to fix that up are really dangerous unless
-	   we're going to re-assign all bus numbers. */
-	if (!pcibios_assign_all_busses())
-		return;
-
-	while (parent->parent && parent->busn_res.end < max) {
-		parent->busn_res.end = max;
-		pci_write_config_byte(parent->self, PCI_SUBORDINATE_BUS, max);
-		parent = parent->parent;
-	}
-}
-
 /*
  * If it's a bridge, configure it and scan the bus behind it.
  * For CardBus bridges, we don't scan behind as the devices will
@@ -879,20 +863,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 
 		if (!is_cardbus) {
 			child->bridge_ctl = bctl;
-			/*
-			 * Adjust subordinate busnr in parent buses.
-			 * We do this before scanning for children because
-			 * some devices may not be detected if the bios
-			 * was lazy.
-			 */
-			pci_fixup_parent_subordinate_busnr(child, max);
-			/* Now we can scan all subordinate buses... */
 			max = pci_scan_child_bus(child);
-			/*
-			 * now fix it up again since we have found
-			 * the real value of max.
-			 */
-			pci_fixup_parent_subordinate_busnr(child, max);
 		} else {
 			/*
 			 * For CardBus bridges, we leave 4 bus numbers
@@ -923,7 +894,6 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 				}
 			}
 			max += i;
-			pci_fixup_parent_subordinate_busnr(child, max);
 		}
 		/*
 		 * Set the subordinate bus number to its real value.
-- 
1.8.5.3

[PATCH 7/8] PCI: Check for child busses which use more bus numbers than allocated.

[Andreas Noever]

pci_scan_child_bus can (potentially) return a bus number higher than the subordinate
value of the child bus. Possible reasons are that bus numbers are reserved for
SR-IOV or for CardBus (SR-IOV is done without checks and the CardBus
checks are sketchy at best).

We clamp the returned value to the actual subordinate value and print a
warning if too many bus numbers are reserved.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 90d5c48..74bc644 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -805,10 +805,13 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 		}
 
 		cmax = pci_scan_child_bus(child);
-		if (cmax > max)
-			max = cmax;
-		if (child->busn_res.end > max)
-			max = child->busn_res.end;
+		if (cmax > subordinate)
+			dev_warn(&dev->dev,
+				"bridge has subordinate %02x but max busn %02x\n",
+				subordinate, cmax);
+		 /* subordinate should equal child->busn_res.end */
+		if (subordinate > max)
+			max = subordinate;
 	} else {
 		/*
 		 * We need to assign a number to this bus which we always
-- 
1.8.5.3

[PATCH 8/8] PCI: Don't scan random busses in pci_scan_bridge.

[Andreas Noever]

When assigning a new bus number in pci_scan_bridge we check whether
max+1 is free by calling pci_find_bus. If it does already exists then we
assume that we are rescanning and that this is the right bus to scan.

This is fragile. If max+1 lies outside of bus->busn_res.end then we will
rescan some random bus from somewhere else in the hierachy. This patch
checks for this case and prints a warning.

Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
---
 drivers/pci/probe.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 74bc644..0a94de2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -830,12 +830,16 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 			goto out;
 		}
 
+		if (max >= bus->busn_res.end) {
+			dev_warn(&dev->dev,
+				"parent bus ran out of bus numbers, cannot allocate child bus\n");
+			goto out;
+		}
+
 		/* Clear errors */
 		pci_write_config_word(dev, PCI_STATUS, 0xffff);
 
-		/* Prevent assigning a bus number that already exists.
-		 * This can happen when a bridge is hot-plugged, so in
-		 * this case we only re-scan this bus. */
+		/* The bus will already exist if we are rescanning */
 		child = pci_find_bus(pci_domain_nr(bus), max+1);
 		if (!child) {
 			child = pci_add_new_bus(bus, dev, max+1);
-- 
1.8.5.3

Re: [PATCH 0/8] PCI: cleanup pci_scan_bridge

[Bjorn Helgaas]

On Thu, Jan 23, 2014 at 09:59:20PM +0100, Andreas Noever wrote:
> I have been reading the code around pci_scan_bridge and noticed a few oddities.
> These patches try to cleanup some of the these things.
> 
> Most of the patches make also sense in isolation, but some make more sense with
> the previous ones applied as well. None of the patches fix any observed bugs.
> Patch 5 fixes a (probably harmless) dmesg.
> 
> The changes related to CardBus probing are completely untested.
> 
> Andreas
> 
> Andreas Noever (8):
>   PCI: Increment max correctly in pci_scan_bridge.
>   PCI: Clarify the "scan anyway" comment in pci_scan_bridge.
>   PCI: Assign CardBus bus number only during the second pass.
>   PCI: Use request_resource_conflict instead of insert_ for bus numbers.
>   PCI: Make sure bus number resources stay within their parents bounds.
>   PCI: Remove pci_fixup_parent_subordinate_busnr.
>   PCI: Check for child busses which use more bus numbers than allocated.
>   PCI: Don't scan random busses in pci_scan_bridge.
> 
>  drivers/pci/probe.c | 78 +++++++++++++++++++++--------------------------------
>  1 file changed, 31 insertions(+), 47 deletions(-)

I applied all these to pci/scan-bridge for v3.15, thanks!

Bjorn