[PATCH v2 0/8] PCI: Improvements for native PCIe hotplug

[PATCH v2 0/8] PCI: Improvements for native PCIe hotplug

[Mika Westerberg]

Currently when plugging PCIe device using native PCIe hotplug Linux PCI
core tries to allocate bus space and resources so that the newly enumerated
topology barely fits there. Now, if the PCIe topology that was just plugged
in has more PCIe hotplug ports we will run out of bus space and resources
pretty quickly. There is a workaround for this by passing pci=hpbussize=N
in the kernel command line but it runs to the same situation after next
hotplug.

A good example where this is a problem is Thunderbolt where each
Thunderbolt device includes PCIe switch and the topology can be extended up
to 6 chained devices.

Future platforms will move from BIOS assisted (ACPI) hotplug from native
PCIe hotplug because then it is possible to power down PCIe hotplug ports
without confusing the SMI handler which is not necessary when native PCIe
hotplug is used. Also Apple Macs have been using native PCIe hotplug from
the beginning. Windows already knows how to handle these complex PCIe
topologies and we will reuse the same knowledge in Linux with this patch
series.

The idea is to distribute available bus space and resources to hotplug PCIe
downstream ports so that the PCIe chain can be extended from those points.
The initial available space is configured by the BIOS to the root port in
question.

This series first teaches the Linux PCI core to distribute available bus
space and resources if PCIe hotplug is used. Then following are fixes for
issues found while testing.

With this series applied I can connect the maximum length Thunderbolt PCIe
chain (6 devices) to a Macbook Pro as can be seen from the 'lspci -t'
output below:

-[0000:00]-+-00.0
           +-02.0
           +-14.0
           +-16.0
           +-19.0
           +-1c.0-[01]----00.0
           +-1c.4-[03-78]----00.0-[04-78]--+-00.0-[05]----00.0
           |                               +-01.0-[07-3f]----00.0-[08-3f]--+-01.0-[09]----00.0
           |                               |                               \-04.0-[0a-3f]----00.0-[0b-3f]--+-01.0-[0c]----00.0
           |                               |                                                               \-04.0-[0d-3f]----00.0-[0e-3f]--+-01.0-[0f]--
           |                               |                                                                                               \-04.0-[10-3f]----00.0-[11-3f]--+-01.0-[12]----00.0
           |                               |                                                                                                                               \-04.0-[13-3f]----00.0-[14-3f]--+-01.0-[15]----00.0
           |                               |                                                                                                                                                               \-04.0-[16-3f]----00.0-[17-3f]----00.0-[18-3f]----00.0
           |                               +-02.0-[06]----00.0
           |                               \-04.0-[40-78]--
           +-1d.0-[79-ee]----00.0-[7a-ee]--+-00.0-[7b]----00.0
           |                               +-01.0-[7d-b5]--
           |                               +-02.0-[7c]----00.0
           |                               \-04.0-[b6-ee]--
           +-1d.3-[02]----00.0
           +-1e.0
           +-1e.1
           +-1e.3
           +-1f.0
           +-1f.2
           +-1f.3
           \-1f.4

The previous version (v1) of the patch series can be found here:

  https://www.spinics.net/lists/linux-pci/msg64841.html

Changes from v1:

  * Rebase on top of Andy's for_each_pci_bridge() patch
  * Move pci_hp_add_bridge() to drivers/pci/probe.c
  * Split open-coding of the two pass scan loop into a separate patch
  * Drop [PATCH 2/7] PCI: Introduce pcie_upstream_port()
  * Keep pci_scan_bridge_extend() and pci_scan_child_bus_extend() internal
    to drivers/pci/probe.c
  * Add another separate loop to count number of hotplug and normal bridges
  * Reword comment about two scans
  * Generalize patches 4 and 5 so that they could work also on conventional
    PCI by dropping tests for PCIe.

Mika Westerberg (8):
  PCI: Move pci_hp_add_bridge() to drivers/pci/probe.c
  PCI: Open-code the two pass loop when scanning bridges
  PCI: Do not allocate more buses than available in parent
  PCI: Distribute available buses to hotplug capable bridges
  PCI: Distribute available resources to hotplug capable bridges
  PCI: pciehp: Fix race condition handling surprise link down
  PCI: pciehp: Do not clear Presence Detect Changed during initialization
  PCI: pciehp: Check that the device is really present before touching it

 drivers/pci/Makefile              |   3 -
 drivers/pci/hotplug-pci.c         |  29 ------
 drivers/pci/hotplug/pciehp_ctrl.c |   7 +-
 drivers/pci/hotplug/pciehp_hpc.c  |  13 ++-
 drivers/pci/hotplug/pciehp_pci.c  |  18 +++-
 drivers/pci/probe.c               | 188 +++++++++++++++++++++++++++++++++++---
 drivers/pci/setup-bus.c           | 177 +++++++++++++++++++++++++++++++++++
 7 files changed, 379 insertions(+), 56 deletions(-)
 delete mode 100644 drivers/pci/hotplug-pci.c

-- 
2.14.2

[PATCH v2 1/8] PCI: Move pci_hp_add_bridge() to drivers/pci/probe.c

[Mika Westerberg]

There is not much point of having a file with a single function in it.
Instead we can just move pci_hp_add_bridge() to drivers/pci/probe.c and
make it available always when PCI core is enabled.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/Makefile      |  3 ---
 drivers/pci/hotplug-pci.c | 29 -----------------------------
 drivers/pci/probe.c       | 24 ++++++++++++++++++++++++
 3 files changed, 24 insertions(+), 32 deletions(-)
 delete mode 100644 drivers/pci/hotplug-pci.c

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 66a21acad952..fa56267fa2c0 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -16,9 +16,6 @@ obj-$(CONFIG_PCIEPORTBUS) += pcie/
 
 # Build the PCI Hotplug drivers if we were asked to
 obj-$(CONFIG_HOTPLUG_PCI) += hotplug/
-ifdef CONFIG_HOTPLUG_PCI
-obj-y += hotplug-pci.o
-endif
 
 # Build the PCI MSI interrupt support
 obj-$(CONFIG_PCI_MSI) += msi.o
diff --git a/drivers/pci/hotplug-pci.c b/drivers/pci/hotplug-pci.c
deleted file mode 100644
index c68366cee6b7..000000000000
--- a/drivers/pci/hotplug-pci.c
+++ /dev/null
@@ -1,29 +0,0 @@
-/* Core PCI functionality used only by PCI hotplug */
-
-#include <linux/pci.h>
-#include <linux/export.h>
-#include "pci.h"
-
-int pci_hp_add_bridge(struct pci_dev *dev)
-{
-	struct pci_bus *parent = dev->bus;
-	int pass, busnr, start = parent->busn_res.start;
-	int end = parent->busn_res.end;
-
-	for (busnr = start; busnr <= end; busnr++) {
-		if (!pci_find_bus(pci_domain_nr(parent), busnr))
-			break;
-	}
-	if (busnr-- > end) {
-		printk(KERN_ERR "No bus number available for hot-added bridge %s\n",
-				pci_name(dev));
-		return -1;
-	}
-	for (pass = 0; pass < 2; pass++)
-		busnr = pci_scan_bridge(parent, dev, busnr, pass);
-	if (!dev->subordinate)
-		return -1;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pci_hp_add_bridge);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cdc2f83c11c5..7302cef51d3f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2735,3 +2735,27 @@ void __init pci_sort_breadthfirst(void)
 {
 	bus_sort_breadthfirst(&pci_bus_type, &pci_sort_bf_cmp);
 }
+
+int pci_hp_add_bridge(struct pci_dev *dev)
+{
+	struct pci_bus *parent = dev->bus;
+	int pass, busnr, start = parent->busn_res.start;
+	int end = parent->busn_res.end;
+
+	for (busnr = start; busnr <= end; busnr++) {
+		if (!pci_find_bus(pci_domain_nr(parent), busnr))
+			break;
+	}
+	if (busnr-- > end) {
+		printk(KERN_ERR "No bus number available for hot-added bridge %s\n",
+				pci_name(dev));
+		return -1;
+	}
+	for (pass = 0; pass < 2; pass++)
+		busnr = pci_scan_bridge(parent, dev, busnr, pass);
+	if (!dev->subordinate)
+		return -1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_hp_add_bridge);
-- 
2.14.2

[PATCH v2 2/8] PCI: Open-code the two pass loop when scanning bridges

[Mika Westerberg]

The current scanning code is really hard to understand because it calls
the same function in a loop where pass value is changed without any
comments explaining it:

  for (pass = 0; pass < 2; pass++)
  	for_each_pci_bridge(dev, bus)
  		max = pci_scan_bridge(bus, dev, max, pass);

Unfamiliar reader cannot tell easily what is the purpose of this loop
without looking at internals of pci_scan_bridge().

In order to make this bit easier to understand, open-code the loop in
pci_scan_child_bus() and pci_hp_add_bridge() with added comments.

No functional changes intended.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/probe.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7302cef51d3f..107052f1dad9 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2398,7 +2398,7 @@ void __weak pcibios_fixup_bus(struct pci_bus *bus)
 
 unsigned int pci_scan_child_bus(struct pci_bus *bus)
 {
-	unsigned int devfn, pass, max = bus->busn_res.start;
+	unsigned int devfn, max = bus->busn_res.start;
 	struct pci_dev *dev;
 
 	dev_dbg(&bus->dev, "scanning bus\n");
@@ -2420,9 +2420,17 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
 		bus->is_added = 1;
 	}
 
-	for (pass = 0; pass < 2; pass++)
-		for_each_pci_bridge(dev, bus)
-			max = pci_scan_bridge(bus, dev, max, pass);
+	/*
+	 * Scan bridges that are already configured. We don't touch them
+	 * unless they are misconfigured (which will be done in the second
+	 * scan below).
+	 */
+	for_each_pci_bridge(dev, bus)
+		max = pci_scan_bridge(bus, dev, max, 0);
+
+	/* Scan bridges that need to be reconfigured */
+	for_each_pci_bridge(dev, bus)
+		max = pci_scan_bridge(bus, dev, max, 1);
 
 	/*
 	 * Make sure a hotplug bridge has at least the minimum requested
@@ -2739,7 +2747,7 @@ void __init pci_sort_breadthfirst(void)
 int pci_hp_add_bridge(struct pci_dev *dev)
 {
 	struct pci_bus *parent = dev->bus;
-	int pass, busnr, start = parent->busn_res.start;
+	int busnr, start = parent->busn_res.start;
 	int end = parent->busn_res.end;
 
 	for (busnr = start; busnr <= end; busnr++) {
@@ -2751,8 +2759,13 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 				pci_name(dev));
 		return -1;
 	}
-	for (pass = 0; pass < 2; pass++)
-		busnr = pci_scan_bridge(parent, dev, busnr, pass);
+
+	/* Scan bridges that are already configured */
+	busnr = pci_scan_bridge(parent, dev, busnr, 0);
+
+	/* Scan bridges that need to be reconfigured */
+	pci_scan_bridge(parent, dev, busnr, 1);
+
 	if (!dev->subordinate)
 		return -1;
 
-- 
2.14.2

[PATCH v2 3/8] PCI: Do not allocate more buses than available in parent

[Mika Westerberg]

One can ask more buses to be reserved for hotplug bridges by passing
pci=hpbussize=N in the kernel command line. However, if the parent bus
does not have enough bus space available we still create child bus with
the requested number of subordinate buses.

In the example below hpbussize is set to one more than we have available
buses in the root port:

  pci 0000:07:00.0: [8086:1578] type 01 class 0x060400
  pci 0000:07:00.0: calling pci_fixup_transparent_bridge+0x0/0x20
  pci 0000:07:00.0: supports D1 D2
  pci 0000:07:00.0: PME# supported from D0 D1 D2 D3hot D3cold
  pci 0000:07:00.0: PME# disabled
  pci 0000:07:00.0: scanning [bus 00-00] behind bridge, pass 0
  pci 0000:07:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
  pci 0000:07:00.0: scanning [bus 00-00] behind bridge, pass 1
  pci_bus 0000:08: busn_res: can not insert [bus 08-ff] under [bus 07-3f] (conflicts with (null) [bus 07-3f])
  pci_bus 0000:08: scanning bus
  ...
  pci_bus 0000:0a: bus scan returning with max=40
  pci_bus 0000:0a: busn_res: [bus 0a-ff] end is updated to 40
  pci_bus 0000:0a: [bus 0a-40] partially hidden behind bridge 0000:07 [bus 07-3f]
  pci_bus 0000:08: bus scan returning with max=40
  pci_bus 0000:08: busn_res: [bus 08-ff] end is updated to 40

Instead of allowing this we limit the subordinate number to be less than
or equal the maximum subordinate number allocated for the parent bus (if
it has any).

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/probe.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 107052f1dad9..32921f6f21b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1076,7 +1076,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)
@@ -2439,6 +2440,10 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
 	if (bus->self && bus->self->is_hotplug_bridge && pci_hotplug_bus_size) {
 		if (max - bus->busn_res.start < pci_hotplug_bus_size - 1)
 			max = bus->busn_res.start + pci_hotplug_bus_size - 1;
+
+		/* Do not allocate more buses than we have room left */
+		if (max > bus->busn_res.end)
+			max = bus->busn_res.end;
 	}
 
 	/*
-- 
2.14.2

[PATCH v2 4/8] PCI: Distribute available buses to hotplug capable bridges

[Mika Westerberg]

System BIOS sometimes allocates extra bus space for hotplug capable PCIe
root/downstream ports. This space is needed if the device plugged to the
port will have more hotplug capable downstream ports. A good example of
this is Thunderbolt. Each Thunderbolt device contains a PCIe switch and
one or more hotplug capable PCIe downstream ports where the daisy chain
can be extended.

Currently Linux only allocates minimal bus space to make sure all the
enumerated devices barely fit there. The BIOS reserved extra space is
not taken into consideration at all. Because of this we run out of bus
space pretty quickly when more PCIe devices are attached to hotplug
downstream ports in order to extend the chain.

This modifies PCI core so that we distribute the available BIOS
allocated bus space equally between hotplug capable bridges to make sure
there is enough bus space for extending the hierarchy later on.

While there update kernel docs of the affected functions.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/probe.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 138 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 32921f6f21b6..4a1ac6c55aee 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -959,7 +959,21 @@ static void pci_enable_crs(struct pci_dev *pdev)
 					 PCI_EXP_RTCTL_CRSSVE);
 }
 
+static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
+					      unsigned int available_buses);
+
 /*
+ * pci_scan_bridge_extend() - Scan buses behind a bridge
+ * @bus: Parent bus the bridge is on
+ * @dev: Bridge itself
+ * @max: Starting subordinate number of buses behind this bridge
+ * @available_buses: Total number of buses available for this bridge and
+ *		     the devices below. After the minimal bus space has
+ *		     been allocated the remaining buses will be
+ *		     distributed equally between hotplug capable bridges.
+ * @pass: Either %0 (scan already configured bridges) or %1 (scan bridges
+ *        that need to be reconfigured.
+ *
  * 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
  * be handled by the bridge driver itself.
@@ -969,7 +983,9 @@ static void pci_enable_crs(struct pci_dev *pdev)
  * them, we proceed to assigning numbers to the remaining buses in
  * order to avoid overlaps between old and new bus numbers.
  */
-int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
+static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
+				  int max, unsigned int available_buses,
+				  int pass)
 {
 	struct pci_bus *child;
 	int is_cardbus = (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS);
@@ -1080,6 +1096,9 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 						bus->busn_res.end);
 		}
 		max++;
+		if (available_buses)
+			available_buses--;
+
 		buses = (buses & 0xff000000)
 		      | ((unsigned int)(child->primary)     <<  0)
 		      | ((unsigned int)(child->busn_res.start)   <<  8)
@@ -1101,7 +1120,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 
 		if (!is_cardbus) {
 			child->bridge_ctl = bctl;
-			max = pci_scan_child_bus(child);
+			max = pci_scan_child_bus_extend(child, available_buses);
 		} else {
 			/*
 			 * For CardBus bridges, we leave 4 bus numbers
@@ -1169,6 +1188,28 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 
 	return max;
 }
+
+/*
+ * pci_scan_bridge() - Scan buses behind a bridge
+ * @bus: Parent bus the bridge is on
+ * @dev: Bridge itself
+ * @max: Starting subordinate number of buses behind this bridge
+ * @pass: Either %0 (scan already configured bridges) or %1 (scan bridges
+ *        that need to be reconfigured.
+ *
+ * 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
+ * be handled by the bridge driver itself.
+ *
+ * We need to process bridges in two passes -- first we scan those
+ * already configured by the BIOS and after we are done with all of
+ * them, we proceed to assigning numbers to the remaining buses in
+ * order to avoid overlaps between old and new bus numbers.
+ */
+int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
+{
+	return pci_scan_bridge_extend(bus, dev, max, 0, pass);
+}
 EXPORT_SYMBOL(pci_scan_bridge);
 
 /*
@@ -2397,9 +2438,24 @@ void __weak pcibios_fixup_bus(struct pci_bus *bus)
        /* nothing to do, expected to be removed in the future */
 }
 
-unsigned int pci_scan_child_bus(struct pci_bus *bus)
+/**
+ * pci_scan_child_bus_extend() - Scan devices below a bus
+ * @bus: Bus to scan for devices
+ * @available_buses: Total number of buses available (%0 does not try to
+ *		     extend beyond the minimal)
+ *
+ * Scans devices below @bus including subordinate buses. Returns new
+ * subordinate number including all the found devices. Passing
+ * @available_buses causes the remaining bus space to be distributed
+ * equally between hotplug capable bridges to allow future extension of the
+ * hierarchy.
+ */
+static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
+					      unsigned int available_buses)
 {
-	unsigned int devfn, max = bus->busn_res.start;
+	unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0;
+	unsigned int start = bus->busn_res.start;
+	unsigned int devfn, cmax, max = start;
 	struct pci_dev *dev;
 
 	dev_dbg(&bus->dev, "scanning bus\n");
@@ -2409,7 +2465,8 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
 		pci_scan_slot(bus, devfn);
 
 	/* Reserve buses for SR-IOV capability. */
-	max += pci_iov_bus_range(bus);
+	used_buses = pci_iov_bus_range(bus);
+	max += used_buses;
 
 	/*
 	 * After performing arch-dependent fixup of the bus, look behind
@@ -2421,29 +2478,73 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
 		bus->is_added = 1;
 	}
 
+	/*
+	 * Calculate how many hotplug bridges and normal bridges there
+	 * are on this bus. We will distribute the additional available
+	 * buses between hotplug bridges.
+	 */
+	for_each_pci_bridge(dev, bus) {
+		if (dev->is_hotplug_bridge)
+			hotplug_bridges++;
+		else
+			normal_bridges++;
+	}
+
 	/*
 	 * Scan bridges that are already configured. We don't touch them
 	 * unless they are misconfigured (which will be done in the second
 	 * scan below).
 	 */
-	for_each_pci_bridge(dev, bus)
-		max = pci_scan_bridge(bus, dev, max, 0);
+	for_each_pci_bridge(dev, bus) {
+		cmax = max;
+		max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
+		used_buses += cmax - max;
+	}
 
 	/* Scan bridges that need to be reconfigured */
-	for_each_pci_bridge(dev, bus)
-		max = pci_scan_bridge(bus, dev, max, 1);
+	for_each_pci_bridge(dev, bus) {
+		unsigned int buses = 0;
+
+		if (!hotplug_bridges && normal_bridges == 1) {
+			/*
+			 * There is only one bridge on the bus (upstream
+			 * port) so it gets all available buses which it
+			 * can then distribute to the possible hotplug
+			 * bridges below.
+			 */
+			buses = available_buses;
+		} else if (dev->is_hotplug_bridge) {
+			/*
+			 * Distribute the extra buses between hotplug
+			 * bridges if any.
+			 */
+			buses = available_buses / hotplug_bridges;
+			buses = min(buses, available_buses - used_buses);
+		}
+
+		cmax = max;
+		max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
+		used_buses += max - cmax;
+	}
 
 	/*
 	 * Make sure a hotplug bridge has at least the minimum requested
-	 * number of buses.
+	 * number of buses but allow it to grow up to the maximum available
+	 * bus number of there is room.
 	 */
-	if (bus->self && bus->self->is_hotplug_bridge && pci_hotplug_bus_size) {
-		if (max - bus->busn_res.start < pci_hotplug_bus_size - 1)
-			max = bus->busn_res.start + pci_hotplug_bus_size - 1;
-
-		/* Do not allocate more buses than we have room left */
-		if (max > bus->busn_res.end)
-			max = bus->busn_res.end;
+	if (bus->self && bus->self->is_hotplug_bridge) {
+		used_buses = max_t(unsigned int, available_buses,
+				   pci_hotplug_bus_size - 1);
+		if (max - start < used_buses) {
+			max = start + used_buses;
+
+			/* Do not allocate more buses than we have room left */
+			if (max > bus->busn_res.end)
+				max = bus->busn_res.end;
+
+			dev_dbg(&bus->dev, "%pR extended by %#02x\n",
+				&bus->busn_res, max - start);
+		}
 	}
 
 	/*
@@ -2456,6 +2557,18 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
 	dev_dbg(&bus->dev, "bus scan returning with max=%02x\n", max);
 	return max;
 }
+
+/**
+ * pci_scan_child_bus() - Scan devices below a bus
+ * @bus: Bus to scan for devices
+ *
+ * Scans devices below @bus including subordinate buses. Returns new
+ * subordinate number including all the found devices.
+ */
+unsigned int pci_scan_child_bus(struct pci_bus *bus)
+{
+	return pci_scan_child_bus_extend(bus, 0);
+}
 EXPORT_SYMBOL_GPL(pci_scan_child_bus);
 
 /**
@@ -2753,6 +2866,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 {
 	struct pci_bus *parent = dev->bus;
 	int busnr, start = parent->busn_res.start;
+	unsigned int available_buses = 0;
 	int end = parent->busn_res.end;
 
 	for (busnr = start; busnr <= end; busnr++) {
@@ -2768,8 +2882,14 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 	/* Scan bridges that are already configured */
 	busnr = pci_scan_bridge(parent, dev, busnr, 0);
 
+	/*
+	 * Distribute the available bus numbers between hotplug capable
+	 * bridges to make extending the chain later possible.
+	 */
+	available_buses = end - busnr;
+
 	/* Scan bridges that need to be reconfigured */
-	pci_scan_bridge(parent, dev, busnr, 1);
+	pci_scan_bridge_extend(parent, dev, busnr, available_buses, 1);
 
 	if (!dev->subordinate)
 		return -1;
-- 
2.14.2

[PATCH v2 5/8] PCI: Distribute available resources to hotplug capable bridges

[Mika Westerberg]

The same problem that we have with bus space, applies to other resources
as well. Linux only allocates the minimal amount of resources so that
the devices currently present barely fit there. This prevents extending
the chain later on because the resource windows allocated for hotplug
downstream ports are too small.

Here we follow what we already did for bus number and assign all
available extra resources to hotplug capable bridges. This makes it
possible to extend the hierarchy later.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/setup-bus.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 177 insertions(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 7ca03407404c..7b85342ad3c3 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1853,6 +1853,175 @@ void __init pci_assign_unassigned_resources(void)
 	}
 }
 
+static void extend_bridge_window(struct pci_dev *bridge, struct resource *res,
+			struct list_head *add_list, resource_size_t available)
+{
+	struct pci_dev_resource *dev_res;
+
+	if (res->parent)
+		return;
+
+	if (resource_size(res) >= available)
+		return;
+
+	dev_res = res_to_dev_res(add_list, res);
+	if (!dev_res)
+		return;
+
+	/* Is there room to extend the window */
+	if (available - resource_size(res) <= dev_res->add_size)
+		return;
+
+	dev_res->add_size = available - resource_size(res);
+	dev_dbg(&bridge->dev, "bridge window %pR extended by %pa\n", res,
+		&dev_res->add_size);
+}
+
+static void pci_bus_distribute_available_resources(struct pci_bus *bus,
+	struct list_head *add_list, resource_size_t available_io,
+	resource_size_t available_mmio, resource_size_t available_mmio_pref)
+{
+	resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+	unsigned int normal_bridges = 0, hotplug_bridges = 0;
+	struct resource *io_res, *mmio_res, *mmio_pref_res;
+	struct pci_dev *dev, *bridge = bus->self;
+
+	io_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 0];
+	mmio_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 1];
+	mmio_pref_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 2];
+
+	/*
+	 * Update additional resource list (add_list) to fill all the
+	 * extra resource space available for this port except the space
+	 * calculated in __pci_bus_size_bridges() which covers all the
+	 * devices currently connected to the port and below.
+	 */
+	extend_bridge_window(bridge, io_res, add_list, available_io);
+	extend_bridge_window(bridge, mmio_res, add_list, available_mmio);
+	extend_bridge_window(bridge, mmio_pref_res, add_list,
+			     available_mmio_pref);
+
+	/*
+	 * Calculate the total amount of extra resource space we can
+	 * pass the to bridges below this one. This is basically the
+	 * extra space substracted by the minimal required space for the
+	 * non-hotplug bridges.
+	 */
+	remaining_io = available_io;
+	remaining_mmio = available_mmio;
+	remaining_mmio_pref = available_mmio_pref;
+
+	/*
+	 * Calculate how many hotplug bridges and normal bridges there
+	 * are on this bus. We will distribute the additional available
+	 * resources between hotplug bridges.
+	 */
+	for_each_pci_bridge(dev, bus) {
+		if (dev->is_hotplug_bridge)
+			hotplug_bridges++;
+		else
+			normal_bridges++;
+	}
+
+	for_each_pci_bridge(dev, bus) {
+		const struct resource *res;
+
+		if (dev->is_hotplug_bridge)
+			continue;
+
+		/*
+		 * Reduce the available resource space by what the
+		 * bridge and devices below it occupy.
+		 */
+		res = &dev->resource[PCI_BRIDGE_RESOURCES + 0];
+		if (!res->parent && available_io > resource_size(res))
+			remaining_io -= resource_size(res);
+
+		res = &dev->resource[PCI_BRIDGE_RESOURCES + 1];
+		if (!res->parent && available_mmio > resource_size(res))
+			remaining_mmio -= resource_size(res);
+
+		res = &dev->resource[PCI_BRIDGE_RESOURCES + 2];
+		if (!res->parent && available_mmio_pref > resource_size(res))
+			remaining_mmio_pref -= resource_size(res);
+	}
+
+	/*
+	 * Go over devices on this bus and distribute the remaining
+	 * resource space between hotplug bridges.
+	 */
+	for_each_pci_bridge(dev, bus) {
+		struct pci_bus *b;
+
+		b = dev->subordinate;
+		if (!b)
+			continue;
+
+		if (!hotplug_bridges && normal_bridges == 1) {
+			/*
+			 * There is only one bridge on the bus (upstream
+			 * port) so it gets all available resources
+			 * which it can then distribute to the possible
+			 * hotplug bridges below.
+			 */
+			pci_bus_distribute_available_resources(b, add_list,
+				available_io, available_mmio,
+				available_mmio_pref);
+		} else if (dev->is_hotplug_bridge) {
+			resource_size_t align, io, mmio, mmio_pref;
+
+			/*
+			 * Distribute available extra resources equally
+			 * between hotplug capable downstream ports
+			 * taking alignment into account.
+			 *
+			 * Here hotplug_bridges is always != 0.
+			 */
+			align = pci_resource_alignment(bridge, io_res);
+			io = div64_ul(available_io, hotplug_bridges);
+			io = min(ALIGN(io, align), remaining_io);
+			remaining_io -= io;
+
+			align = pci_resource_alignment(bridge, mmio_res);
+			mmio = div64_ul(available_mmio, hotplug_bridges);
+			mmio = min(ALIGN(mmio, align), remaining_mmio);
+			remaining_mmio -= mmio;
+
+			align = pci_resource_alignment(bridge, mmio_pref_res);
+			mmio_pref = div64_ul(available_mmio_pref,
+					     hotplug_bridges);
+			mmio_pref = min(ALIGN(mmio_pref, align),
+					remaining_mmio_pref);
+			remaining_mmio_pref -= mmio_pref;
+
+			pci_bus_distribute_available_resources(b, add_list, io,
+							       mmio, mmio_pref);
+		}
+	}
+}
+
+static void
+pci_bridge_distribute_available_resources(struct pci_dev *bridge,
+					  struct list_head *add_list)
+{
+	resource_size_t available_io, available_mmio, available_mmio_pref;
+	const struct resource *res;
+
+	if (!bridge->is_hotplug_bridge)
+		return;
+
+	/* Take the initial extra resources from the hotplug port */
+	res = &bridge->resource[PCI_BRIDGE_RESOURCES + 0];
+	available_io = resource_size(res);
+	res = &bridge->resource[PCI_BRIDGE_RESOURCES + 1];
+	available_mmio = resource_size(res);
+	res = &bridge->resource[PCI_BRIDGE_RESOURCES + 2];
+	available_mmio_pref = resource_size(res);
+
+	pci_bus_distribute_available_resources(bridge->subordinate,
+		add_list, available_io, available_mmio, available_mmio_pref);
+}
+
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 {
 	struct pci_bus *parent = bridge->subordinate;
@@ -1867,6 +2036,14 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 
 again:
 	__pci_bus_size_bridges(parent, &add_list);
+
+	/*
+	 * Distribute remaining resources (if any) equally between
+	 * hotplug bridges below. This makes it possible to extend the
+	 * hierarchy later without running out of resources.
+	 */
+	pci_bridge_distribute_available_resources(bridge, &add_list);
+
 	__pci_bridge_assign_resources(bridge, &add_list, &fail_head);
 	BUG_ON(!list_empty(&add_list));
 	tried_times++;
-- 
2.14.2

[PATCH v2 6/8] PCI: pciehp: Fix race condition handling surprise link down

[Mika Westerberg]

A surprise link down may retrain very quickly causing the same slot
generate a link up event before handling the link down event completes.

Since the link is active, the power off work queued from the first link
down will cause a second down event when power is disabled. However, the
link up event sets the slot state to POWERON_STATE before the event to
handle this is enqueued, making the second down event believe it needs
to do something.

This creates constant link up and down event cycle.

To prevent this it is better to handle each event at the time in order
it occurred, so change the driver to use ordered workqueue instead.

A normal device hotplug triggers two events (presense detect and link
up) that are already handled properly in the driver but we currently log
an error if we find an existing device in the slot. Since this is not an
error change the log level to be debug instead to avoid scaring users.

This is based on the original work by Ashok Raj.

Link: https://patchwork.kernel.org/patch/9469023
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 7 ++++---
 drivers/pci/hotplug/pciehp_hpc.c  | 2 +-
 drivers/pci/hotplug/pciehp_pci.c  | 6 +++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index ec0b4c11ccd9..83f3d4af3677 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -113,10 +113,11 @@ static int board_added(struct slot *p_slot)
 
 	retval = pciehp_configure_device(p_slot);
 	if (retval) {
-		ctrl_err(ctrl, "Cannot add device at %04x:%02x:00\n",
-			 pci_domain_nr(parent), parent->number);
-		if (retval != -EEXIST)
+		if (retval != -EEXIST) {
+			ctrl_err(ctrl, "Cannot add device at %04x:%02x:00\n",
+				 pci_domain_nr(parent), parent->number);
 			goto err_exit;
+		}
 	}
 
 	pciehp_green_led_on(p_slot);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index e5d5ce9e3010..83c93f9da65a 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -795,7 +795,7 @@ static int pcie_init_slot(struct controller *ctrl)
 	if (!slot)
 		return -ENOMEM;
 
-	slot->wq = alloc_workqueue("pciehp-%u", 0, 0, PSN(ctrl));
+	slot->wq = alloc_ordered_workqueue("pciehp-%u", 0, PSN(ctrl));
 	if (!slot->wq)
 		goto abort;
 
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index c3af027ee1a6..2a1ca020cf5a 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -46,7 +46,11 @@ int pciehp_configure_device(struct slot *p_slot)
 
 	dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
 	if (dev) {
-		ctrl_err(ctrl, "Device %s already exists at %04x:%02x:00, cannot hot-add\n",
+		/*
+		 * The device is already there. Either configured by the
+		 * boot firmware or a previous hotplug event.
+		 */
+		ctrl_dbg(ctrl, "Device %s already exists at %04x:%02x:00, skipping hot-add\n",
 			 pci_name(dev), pci_domain_nr(parent), parent->number);
 		pci_dev_put(dev);
 		ret = -EEXIST;
-- 
2.14.2

[PATCH v2 7/8] PCI: pciehp: Do not clear Presence Detect Changed during initialization

[Mika Westerberg]

It is possible that the hotplug event has already happened before the
driver is attached to a PCIe hotplug downstream port. If we just clear
the status we never get the hotplug interrupt and thus the event will be
missed.

To make sure that does not happen, we leave Presence Detect Changed bit
untouched during initialization. Then once the event is unmasked we get
an interrupt and handle the hotplug event properly.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 83c93f9da65a..bc1622aa7a05 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -862,11 +862,16 @@ struct controller *pcie_init(struct pcie_device *dev)
 	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
 		ctrl->link_active_reporting = 1;
 
-	/* Clear all remaining event bits in Slot Status register */
+	/*
+	 * Clear all remaining event bits in Slot Status register except
+	 * Presence Detect Changed. We want to make sure possible
+	 * hotplug event is triggered when the interrupt is unmasked so
+	 * that we don't lose that event.
+	 */
 	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
 		PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
-		PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
-		PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
+		PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC |
+		PCI_EXP_SLTSTA_DLLSC);
 
 	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c\n",
 		(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
-- 
2.14.2

[PATCH v2 8/8] PCI: pciehp: Check that the device is really present before touching it

[Mika Westerberg]

During surprise hot-unplug the device is not there anymore. When that
happens we read 0xffffffff from the registers and pciehp_unconfigure_device()
inadvertently thinks the device is a display device because bridge
control register returns 0xff refusing to remove it:

  pciehp 0000:00:1c.0:pcie004: Slot(0): Link Down
  pciehp 0000:00:1c.0:pcie004: Slot(0): Card present
  pciehp 0000:00:1c.0:pcie004: Cannot remove display device 0000:01:00.0

This causes the hotplug functionality to leave the hierarcy untouched
preventing further hotplug operations.

To fix this verify presence of a device by calling pci_device_is_present()
for it before we touch it any further.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_pci.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 2a1ca020cf5a..fb4333168e23 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -100,8 +100,14 @@ int pciehp_unconfigure_device(struct slot *p_slot)
 	 */
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
 					 bus_list) {
+		bool present;
+
 		pci_dev_get(dev);
-		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE && presence) {
+
+		/* Check if the device is really there anymore */
+		present = presence ? pci_device_is_present(dev) : false;
+
+		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE && present) {
 			pci_read_config_byte(dev, PCI_BRIDGE_CONTROL, &bctl);
 			if (bctl & PCI_BRIDGE_CTL_VGA) {
 				ctrl_err(ctrl,
@@ -112,7 +118,7 @@ int pciehp_unconfigure_device(struct slot *p_slot)
 				break;
 			}
 		}
-		if (!presence) {
+		if (!present) {
 			pci_dev_set_disconnected(dev, NULL);
 			if (pci_has_subordinate(dev))
 				pci_walk_bus(dev->subordinate,
@@ -123,7 +129,7 @@ int pciehp_unconfigure_device(struct slot *p_slot)
 		 * Ensure that no new Requests will be generated from
 		 * the device.
 		 */
-		if (presence) {
+		if (present) {
 			pci_read_config_word(dev, PCI_COMMAND, &command);
 			command &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
 			command |= PCI_COMMAND_INTX_DISABLE;
-- 
2.14.2

Re: [PATCH v2 8/8] PCI: pciehp: Check that the device is really present before touching it

[Bjorn Helgaas]

On Fri, Oct 13, 2017 at 09:35:48PM +0300, Mika Westerberg wrote:
> During surprise hot-unplug the device is not there anymore. When that
> happens we read 0xffffffff from the registers and pciehp_unconfigure_device()
> inadvertently thinks the device is a display device because bridge
> control register returns 0xff refusing to remove it:
> 
>   pciehp 0000:00:1c.0:pcie004: Slot(0): Link Down
>   pciehp 0000:00:1c.0:pcie004: Slot(0): Card present
>   pciehp 0000:00:1c.0:pcie004: Cannot remove display device 0000:01:00.0
> 
> This causes the hotplug functionality to leave the hierarcy untouched
> preventing further hotplug operations.
> 
> To fix this verify presence of a device by calling pci_device_is_present()
> for it before we touch it any further.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/hotplug/pciehp_pci.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index 2a1ca020cf5a..fb4333168e23 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -100,8 +100,14 @@ int pciehp_unconfigure_device(struct slot *p_slot)
>  	 */
>  	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>  					 bus_list) {
> +		bool present;
> +
>  		pci_dev_get(dev);
> -		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE && presence) {
> +
> +		/* Check if the device is really there anymore */
> +		present = presence ? pci_device_is_present(dev) : false;
> +
> +		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE && present) {
>  			pci_read_config_byte(dev, PCI_BRIDGE_CONTROL, &bctl);

I don't like this fix because it's still racy.  We always have to be deal
with a config read that returns 0xffffffff, even if we previously checked
pci_device_is_present().  The device might have disappeared in the interim.

>  			if (bctl & PCI_BRIDGE_CTL_VGA) {
>  				ctrl_err(ctrl,
> @@ -112,7 +118,7 @@ int pciehp_unconfigure_device(struct slot *p_slot)
>  				break;
>  			}
>  		}
> -		if (!presence) {
> +		if (!present) {
>  			pci_dev_set_disconnected(dev, NULL);
>  			if (pci_has_subordinate(dev))
>  				pci_walk_bus(dev->subordinate,
> @@ -123,7 +129,7 @@ int pciehp_unconfigure_device(struct slot *p_slot)
>  		 * Ensure that no new Requests will be generated from
>  		 * the device.
>  		 */
> -		if (presence) {
> +		if (present) {
>  			pci_read_config_word(dev, PCI_COMMAND, &command);
>  			command &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
>  			command |= PCI_COMMAND_INTX_DISABLE;
> -- 
> 2.14.2
> 

Re: [PATCH v2 0/8] PCI: Improvements for native PCIe hotplug

[Bjorn Helgaas]

On Fri, Oct 13, 2017 at 09:35:40PM +0300, Mika Westerberg wrote:
> Currently when plugging PCIe device using native PCIe hotplug Linux PCI
> core tries to allocate bus space and resources so that the newly enumerated
> topology barely fits there. Now, if the PCIe topology that was just plugged
> in has more PCIe hotplug ports we will run out of bus space and resources
> pretty quickly. There is a workaround for this by passing pci=hpbussize=N
> in the kernel command line but it runs to the same situation after next
> hotplug.
> 
> A good example where this is a problem is Thunderbolt where each
> Thunderbolt device includes PCIe switch and the topology can be extended up
> to 6 chained devices.
> 
> Future platforms will move from BIOS assisted (ACPI) hotplug from native
> PCIe hotplug because then it is possible to power down PCIe hotplug ports
> without confusing the SMI handler which is not necessary when native PCIe
> hotplug is used. Also Apple Macs have been using native PCIe hotplug from
> the beginning. Windows already knows how to handle these complex PCIe
> topologies and we will reuse the same knowledge in Linux with this patch
> series.
> 
> The idea is to distribute available bus space and resources to hotplug PCIe
> downstream ports so that the PCIe chain can be extended from those points.
> The initial available space is configured by the BIOS to the root port in
> question.
> 
> This series first teaches the Linux PCI core to distribute available bus
> space and resources if PCIe hotplug is used. Then following are fixes for
> issues found while testing.
> 
> With this series applied I can connect the maximum length Thunderbolt PCIe
> chain (6 devices) to a Macbook Pro as can be seen from the 'lspci -t'
> output below:
> 
> -[0000:00]-+-00.0
>            +-02.0
>            +-14.0
>            +-16.0
>            +-19.0
>            +-1c.0-[01]----00.0
>            +-1c.4-[03-78]----00.0-[04-78]--+-00.0-[05]----00.0
>            |                               +-01.0-[07-3f]----00.0-[08-3f]--+-01.0-[09]----00.0
>            |                               |                               \-04.0-[0a-3f]----00.0-[0b-3f]--+-01.0-[0c]----00.0
>            |                               |                                                               \-04.0-[0d-3f]----00.0-[0e-3f]--+-01.0-[0f]--
>            |                               |                                                                                               \-04.0-[10-3f]----00.0-[11-3f]--+-01.0-[12]----00.0
>            |                               |                                                                                                                               \-04.0-[13-3f]----00.0-[14-3f]--+-01.0-[15]----00.0
>            |                               |                                                                                                                                                               \-04.0-[16-3f]----00.0-[17-3f]----00.0-[18-3f]----00.0
>            |                               +-02.0-[06]----00.0
>            |                               \-04.0-[40-78]--
>            +-1d.0-[79-ee]----00.0-[7a-ee]--+-00.0-[7b]----00.0
>            |                               +-01.0-[7d-b5]--
>            |                               +-02.0-[7c]----00.0
>            |                               \-04.0-[b6-ee]--
>            +-1d.3-[02]----00.0
>            +-1e.0
>            +-1e.1
>            +-1e.3
>            +-1f.0
>            +-1f.2
>            +-1f.3
>            \-1f.4
> 
> The previous version (v1) of the patch series can be found here:
> 
>   https://www.spinics.net/lists/linux-pci/msg64841.html
> 
> Changes from v1:
> 
>   * Rebase on top of Andy's for_each_pci_bridge() patch
>   * Move pci_hp_add_bridge() to drivers/pci/probe.c
>   * Split open-coding of the two pass scan loop into a separate patch
>   * Drop [PATCH 2/7] PCI: Introduce pcie_upstream_port()
>   * Keep pci_scan_bridge_extend() and pci_scan_child_bus_extend() internal
>     to drivers/pci/probe.c
>   * Add another separate loop to count number of hotplug and normal bridges
>   * Reword comment about two scans
>   * Generalize patches 4 and 5 so that they could work also on conventional
>     PCI by dropping tests for PCIe.
> 
> Mika Westerberg (8):
>   PCI: Move pci_hp_add_bridge() to drivers/pci/probe.c
>   PCI: Open-code the two pass loop when scanning bridges
>   PCI: Do not allocate more buses than available in parent
>   PCI: Distribute available buses to hotplug capable bridges
>   PCI: Distribute available resources to hotplug capable bridges
>   PCI: pciehp: Fix race condition handling surprise link down
>   PCI: pciehp: Do not clear Presence Detect Changed during initialization
>   PCI: pciehp: Check that the device is really present before touching it
> 
>  drivers/pci/Makefile              |   3 -
>  drivers/pci/hotplug-pci.c         |  29 ------
>  drivers/pci/hotplug/pciehp_ctrl.c |   7 +-
>  drivers/pci/hotplug/pciehp_hpc.c  |  13 ++-
>  drivers/pci/hotplug/pciehp_pci.c  |  18 +++-
>  drivers/pci/probe.c               | 188 +++++++++++++++++++++++++++++++++++---
>  drivers/pci/setup-bus.c           | 177 +++++++++++++++++++++++++++++++++++
>  7 files changed, 379 insertions(+), 56 deletions(-)
>  delete mode 100644 drivers/pci/hotplug-pci.c

Looks very nice!

I applied all but the last patch ("Check that the device is really
present") on pci/hotplug for v4.15, thanks!

Re: [PATCH v2 8/8] PCI: pciehp: Check that the device is really present before touching it

[Mika Westerberg]

On Fri, Oct 20, 2017 at 04:15:02PM -0500, Bjorn Helgaas wrote:
> > +
> > +		/* Check if the device is really there anymore */
> > +		present = presence ? pci_device_is_present(dev) : false;
> > +
> > +		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE && present) {
> >  			pci_read_config_byte(dev, PCI_BRIDGE_CONTROL, &bctl);
> 
> I don't like this fix because it's still racy.  We always have to be deal
> with a config read that returns 0xffffffff, even if we previously checked
> pci_device_is_present().  The device might have disappeared in the interim.

That's a fair point. I guess it is better just to check if bctl holds
0xffff before we decide it is a display device.

I'll rework this patch and send an updated version separately.