[PATCH v8 0/7] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[PATCH v8 0/7] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Mika Westerberg]

When Thunderbolt host router is configured to be in native enumeration mode
it is only present in the system if there is something connected to the
ports. This pretty much follows how the BIOS assisted mode works.

In native enumeration mode the Thunderbolt host controller (NHI) and USB
host controller (xHCI) are not hot-added using native PCIe hotplug but
instead they will be hotplugged via BIOS triggered ACPI Notify() to the
root port. This is done to preserve resources since NHI and xHCI only need
1 MB of MMIO space and no additional buses. Currently Linux does not
support this very well and ends up failing the hotplug in one way or
another. More detailed explanation is in changelog of patch [4/7].

This series fixes this issue and in addition includes fixes for few other
issues found during testing on a system that has Thunderbolt controller in
native PCIe enumeration mode. However, the fixes here are not in any way
Thunderbolt specific and should be applicable to other systems as well.

The previous versions of the patch series can be found here:

  v7: https://www.spinics.net/lists/linux-pci/msg72558.html
  v6: https://www.spinics.net/lists/linux-pci/msg72267.html
  v5: https://www.spinics.net/lists/linux-pci/msg71339.html
  v4: https://www.spinics.net/lists/linux-pci/msg71006.html
  v3: https://www.spinics.net/lists/linux-acpi/msg80876.html
  v2: https://www.spinics.net/lists/linux-pci/msg69186.html
  v1: https://www.spinics.net/lists/linux-acpi/msg80607.html

Changes from v7:

  - Rebased on top of pci.git pci/hotplug
  - Make acpi_get_hp_hw_control_from_firmware() call shpchp_is_native()
  - Drop !bridge check from shpchp_is_native() and update callers to handle
    possibly NULL pdev
  - Split out hotplug_is_native() into a separate patch

Changes from v6:

  - Use IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) and IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)
    directly instaed of wrapping them in macro
  - Check for !bridge in the same condition than we check whether certain
    native hotplug is enabled
  - Add Rafael's Reviewed-by

Changes from v5:

  - Dropped patch "PCI: Take bridge window alignment into account when
    distributing resources"
  - Rework pciehp_is_native() to be more stricter
  - Make standard PCI hotplug driver (SHPC) builtin
  - Rework the way we request OS control of native PCIe
    hotplug (pciehp) and standard PCI hotplug (SHPC)
  - Make acpiphp to avoid bridges where hotplug_is_native() returns true
    (this is combined from pciehp and SHPC)
  - Updated changelog of patch [7/12]
  - Use hotplug_bridges + normal_bridges == 1 in patch [9/12] and also
    remove one indent level from the loop.

Because patch [7/12] looks quite different now I dropped Reviewed-by and
stable tag (it now depends on reworked pciehp/SHCP support that most
probably is not suitable for stable trees).

I also was not able to test that SHPC really works because I don't have
hardware that supports it.

Changes from v4:

  - Updated message in patch [8/9] following Rafael's suggestion
  - Added Rafael's tag to patches 6, 7, 9.

Changes from v3:

  - Added Andy's tag to patches [1-5]
  - Improved changelog and subject line of patch [1/9] to match better what
    it is trying to solve
  - Improved comment in patch [1/9]
  - Improved changelog of patches [2/9] and [4/9] accordingly to match
    better why they are needed
  - Added cleanup patches [6-9/9]

Changes from v2:

  - Added Rafael's tag to patch [1/5]
  - Updated changelog of patch [1/5] to include more details about how the
    problem can be observed from dmesg and lspci output

Changes from v1:

  - Drop 'cmax - max ?: 1' and use similar construct than we use in second
    pass loop in patch [1/5]
  - Drop unnecessary parentheses in patch [1/5]
  - Added Rafael's tag to patches [2-5/5]

Mika Westerberg (7):
  PCI: Take all bridges into account when calculating bus numbers for extension
  PCI: Introduce shpchp_is_native()
  PCI: Introduce hotplug_is_native()
  ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used
  ACPI / hotplug / PCI: Mark stale PCI devices disconnected
  PCI: Move resource distribution for a single bridge outside of the loop
  PCI: Document return value of pci_scan_bridge() and pci_scan_bridge_extend()

 drivers/pci/hotplug/acpi_pcihp.c   |  4 +-
 drivers/pci/hotplug/acpiphp_glue.c | 80 ++++++++++++++++++++++-------
 drivers/pci/hotplug/shpchp_core.c  |  2 -
 drivers/pci/pci-acpi.c             | 21 ++++++++
 drivers/pci/probe.c                | 19 +++++--
 drivers/pci/setup-bus.c            | 82 +++++++++++++++---------------
 include/linux/pci_hotplug.h        |  7 +++
 7 files changed, 150 insertions(+), 65 deletions(-)

-- 
2.17.0

[PATCH v8 1/7] PCI: Take all bridges into account when calculating bus numbers for extension

[Mika Westerberg]

When distributing extra buses between hotplug bridges for future
extension we currently fail to take into account the fact that there
might be non-hotplug bridges on the bus after the hotplug bridges. In
this particular system we have following topology:

  01:00.0 --+- 02:00.0 -- Thunderbolt host controller
            +- 02:01.0 (HotPlug+)
            \- 02:02.0 -- xHCI host controller

Now, pci_scan_child_bus_extend() is supposed to distribute remaining bus
numbers to the hotplug bridge at 02:01.0 but only after all bridges on
that bus have been been accounted for. Since we don't check upfront that
there will be another non-hotplug bridge after the hotplug bridge
02:01.0 it will inadvertently extend over the bus space of the
non-hotplug bridge:

  pci 0000:01:00.0: PCI bridge to [bus 02-39]
  ...
  pci_bus 0000:04: [bus 04-39] extended by 0x35
  pci_bus 0000:04: bus scan returning with max=39
  pci_bus 0000:04: busn_res: [bus 04-39] end is updated to 39
  pci 0000:02:02.0: scanning [bus 00-00] behind bridge, pass 1
  pci_bus 0000:3a: scanning bus
  pci_bus 0000:3a: bus scan returning with max=3a
  pci_bus 0000:3a: busn_res: [bus 3a] end is updated to 3a
  pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39]
  pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39]
  pci_bus 0000:02: bus scan returning with max=3a
  pci_bus 0000:02: busn_res: [bus 02-39] end can not be updated to 3a

Resulting 'lspci -t' output looks like this:

  +-1b.0-[01-39]----00.0-[02-3a]--+-00.0-[03]----00.0
                             ^^   +-01.0-[04-39]--
                                  \-02.0-[3a]----00.0
                                          ^^
The xHCI host controller (3a:00.0) behind downstream bridge at 02:02.0
is not usable anymore.

To fix this reserve at least one bus for each bridge during scanning of
already configured bridges. Then we use this information in the second
scan to correct the available extra bus space for hotplug bridges.

After this change the 'lspci -t' output is what is expected:

  +-1b.0-[01-39]----00.0-[02-39]--+-00.0-[03]----00.0
                                  +-01.0-[04-38]--
                                  \-02.0-[39]----00.0

Fixes: 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")
Reported-by: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/probe.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index d14e8f827d53..8f384e7ca2c2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2634,7 +2634,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 	for_each_pci_bridge(dev, bus) {
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
-		used_buses += cmax - max;
+		/*
+		 * Reserve one bus for each bridge now to avoid
+		 * extending hotplug bridges too much during the second
+		 * scan below.
+		 */
+		used_buses++;
+		if (cmax - max > 1)
+			used_buses += cmax - max - 1;
 	}
 
 	/* Scan bridges that need to be reconfigured */
@@ -2657,12 +2664,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 			 * bridges if any.
 			 */
 			buses = available_buses / hotplug_bridges;
-			buses = min(buses, available_buses - used_buses);
+			buses = min(buses, available_buses - used_buses + 1);
 		}
 
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
-		used_buses += max - cmax;
+		/* One bus is already accounted so don't add it again */
+		if (max - cmax > 1)
+			used_buses += max - cmax - 1;
 	}
 
 	/*
-- 
2.17.0

[PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Mika Westerberg]

In the same way we do for pciehp, introduce a new function
shpchp_is_native() that returns true whether the bridge should be
handled by the native SHCP driver. Then convert the driver to use this
function.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/acpi_pcihp.c  |  4 ++--
 drivers/pci/hotplug/shpchp_core.c |  2 --
 drivers/pci/pci-acpi.c            | 21 +++++++++++++++++++++
 include/linux/pci_hotplug.h       |  2 ++
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index 597d22aeefc1..3979f89b250a 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
 	 * OSHP within the scope of the hotplug controller and its parents,
 	 * up to the host bridge under which this controller exists.
 	 */
-	host = pci_find_host_bridge(pdev->bus);
-	if (host->native_shpc_hotplug)
+	if (shpchp_is_native(pdev))
 		return 0;
 
 	/* If _OSC exists, we should not evaluate OSHP */
+	host = pci_find_host_bridge(pdev->bus);
 	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
 	if (root->osc_support_set)
 		goto no_control;
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 47decc9b3bb3..0f3711404c2b 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev)
 	if (dev->vendor == PCI_VENDOR_ID_AMD &&
 	    dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
 		return 1;
-	if (!pci_find_capability(dev, PCI_CAP_ID_SHPC))
-		return 0;
 	if (acpi_get_hp_hw_control_from_firmware(dev))
 		return 0;
 	return 1;
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 52b8434d4d6e..bb83be0d0e5b 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge)
 	return host->native_pcie_hotplug;
 }
 
+/**
+ * shpchp_is_native - Check whether a hotplug port is handled by the OS
+ * @bridge: Hotplug port to check
+ *
+ * Returns true if the given @bridge is handled by the native SHPC hotplug
+ * driver.
+ */
+bool shpchp_is_native(struct pci_dev *bridge)
+{
+	const struct pci_host_bridge *host;
+
+	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
+		return false;
+
+	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
+		return false;
+
+	host = pci_find_host_bridge(bridge->bus);
+	return host->native_shpc_hotplug;
+}
+
 /**
  * pci_acpi_wake_bus - Root bus wakeup notification fork function.
  * @context: Device wakeup context.
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 1f5c935eb0de..4c378368215c 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -164,6 +164,7 @@ struct hotplug_params {
 int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp);
 bool pciehp_is_native(struct pci_dev *bridge);
 int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge);
+bool shpchp_is_native(struct pci_dev *bridge);
 int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
 int acpi_pci_detect_ejectable(acpi_handle handle);
 #else
@@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
 	return 0;
 }
 static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
+static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
 #endif
 #endif
-- 
2.17.0

[PATCH v8 3/7] PCI: Introduce hotplug_is_native()

[Mika Westerberg]

This helper function can be used for finding out whether the OS is
supposed to handle native hotplug of a given bridge.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 include/linux/pci_hotplug.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 4c378368215c..cf5e22103f68 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -181,4 +181,9 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
 static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
 static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
 #endif
+
+static inline bool hotplug_is_native(struct pci_dev *bridge)
+{
+	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
+}
 #endif
-- 
2.17.0

[PATCH v8 6/7] PCI: Move resource distribution for a single bridge outside of the loop

[Mika Westerberg]

There is a special case where there is only single bridge on the bus. In
that case we just assign all resources to it. Currently this is done as
a part of the resource distribution loop but it does not have to be
there, and moving it outside actually improves readability because we
can then save one indent level in the loop.

While there we can add hotplug_bridges == 1 && normal_bridges == 0 to
the same block because they are dealt the same way.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/setup-bus.c | 82 ++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 072784f55ea5..79b1824e83b4 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1942,57 +1942,57 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			remaining_mmio_pref -= resource_size(res);
 	}
 
+	/*
+	 * There is only one bridge on the bus so it gets all available
+	 * resources which it can then distribute to the possible
+	 * hotplug bridges below.
+	 */
+	if (hotplug_bridges + normal_bridges == 1) {
+		dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
+		if (dev->subordinate) {
+			pci_bus_distribute_available_resources(dev->subordinate,
+				add_list, available_io, available_mmio,
+				available_mmio_pref);
+		}
+		return;
+	}
+
 	/*
 	 * Go over devices on this bus and distribute the remaining
 	 * resource space between hotplug bridges.
 	 */
 	for_each_pci_bridge(dev, bus) {
+		resource_size_t align, io, mmio, mmio_pref;
 		struct pci_bus *b;
 
 		b = dev->subordinate;
-		if (!b)
+		if (!b || !dev->is_hotplug_bridge)
 			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);
-		}
+		/*
+		 * 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);
 	}
 }
 
-- 
2.17.0

[PATCH v8 7/7] PCI: Document return value of pci_scan_bridge() and pci_scan_bridge_extend()

[Mika Westerberg]

It is not immediately clear what the two functions actually return so
add kernel-doc comment explaining it a bit better.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/probe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8f384e7ca2c2..7dc0b3c49941 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -999,6 +999,8 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
  * 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.
+ *
+ * Return: New subordinate number covering all buses behind this bridge.
  */
 static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
 				  int max, unsigned int available_buses,
@@ -1226,6 +1228,8 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
  * 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.
+ *
+ * Return: New subordinate number covering all buses behind this bridge.
  */
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
 {
-- 
2.17.0

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Rafael J. Wysocki]

On Monday, May 28, 2018 2:47:51 PM CEST Mika Westerberg wrote:
> In the same way we do for pciehp, introduce a new function
> shpchp_is_native() that returns true whether the bridge should be
> handled by the native SHCP driver. Then convert the driver to use this
> function.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/hotplug/acpi_pcihp.c  |  4 ++--
>  drivers/pci/hotplug/shpchp_core.c |  2 --
>  drivers/pci/pci-acpi.c            | 21 +++++++++++++++++++++
>  include/linux/pci_hotplug.h       |  2 ++
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index 597d22aeefc1..3979f89b250a 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
>  	 * OSHP within the scope of the hotplug controller and its parents,
>  	 * up to the host bridge under which this controller exists.
>  	 */
> -	host = pci_find_host_bridge(pdev->bus);
> -	if (host->native_shpc_hotplug)
> +	if (shpchp_is_native(pdev))
>  		return 0;
>  
>  	/* If _OSC exists, we should not evaluate OSHP */
> +	host = pci_find_host_bridge(pdev->bus);
>  	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
>  	if (root->osc_support_set)
>  		goto no_control;
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index 47decc9b3bb3..0f3711404c2b 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev)
>  	if (dev->vendor == PCI_VENDOR_ID_AMD &&
>  	    dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
>  		return 1;
> -	if (!pci_find_capability(dev, PCI_CAP_ID_SHPC))
> -		return 0;
>  	if (acpi_get_hp_hw_control_from_firmware(dev))
>  		return 0;
>  	return 1;
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 52b8434d4d6e..bb83be0d0e5b 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge)
>  	return host->native_pcie_hotplug;
>  }
>  
> +/**
> + * shpchp_is_native - Check whether a hotplug port is handled by the OS
> + * @bridge: Hotplug port to check
> + *
> + * Returns true if the given @bridge is handled by the native SHPC hotplug
> + * driver.
> + */
> +bool shpchp_is_native(struct pci_dev *bridge)
> +{
> +	const struct pci_host_bridge *host;
> +
> +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> +		return false;
> +
> +	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
> +		return false;
> +
> +	host = pci_find_host_bridge(bridge->bus);
> +	return host->native_shpc_hotplug;
> +}
> +
>  /**
>   * pci_acpi_wake_bus - Root bus wakeup notification fork function.
>   * @context: Device wakeup context.
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 1f5c935eb0de..4c378368215c 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -164,6 +164,7 @@ struct hotplug_params {
>  int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp);
>  bool pciehp_is_native(struct pci_dev *bridge);
>  int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge);
> +bool shpchp_is_native(struct pci_dev *bridge);
>  int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
>  int acpi_pci_detect_ejectable(acpi_handle handle);
>  #else
> @@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
>  	return 0;
>  }
>  static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
> +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>  #endif
>  #endif
> 

Re: [PATCH v8 3/7] PCI: Introduce hotplug_is_native()

[Rafael J. Wysocki]

On Monday, May 28, 2018 2:47:52 PM CEST Mika Westerberg wrote:
> This helper function can be used for finding out whether the OS is
> supposed to handle native hotplug of a given bridge.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  include/linux/pci_hotplug.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 4c378368215c..cf5e22103f68 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -181,4 +181,9 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
>  static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
>  static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>  #endif
> +
> +static inline bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> +}
>  #endif
> 

Re: [PATCH v8 0/7] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Bjorn Helgaas]

On Mon, May 28, 2018 at 03:47:49PM +0300, Mika Westerberg wrote:
> When Thunderbolt host router is configured to be in native enumeration mode
> it is only present in the system if there is something connected to the
> ports. This pretty much follows how the BIOS assisted mode works.
> 
> In native enumeration mode the Thunderbolt host controller (NHI) and USB
> host controller (xHCI) are not hot-added using native PCIe hotplug but
> instead they will be hotplugged via BIOS triggered ACPI Notify() to the
> root port. This is done to preserve resources since NHI and xHCI only need
> 1 MB of MMIO space and no additional buses. Currently Linux does not
> support this very well and ends up failing the hotplug in one way or
> another. More detailed explanation is in changelog of patch [4/7].
> 
> This series fixes this issue and in addition includes fixes for few other
> issues found during testing on a system that has Thunderbolt controller in
> native PCIe enumeration mode. However, the fixes here are not in any way
> Thunderbolt specific and should be applicable to other systems as well.
> 
> The previous versions of the patch series can be found here:
> 
>   v7: https://www.spinics.net/lists/linux-pci/msg72558.html
>   v6: https://www.spinics.net/lists/linux-pci/msg72267.html
>   v5: https://www.spinics.net/lists/linux-pci/msg71339.html
>   v4: https://www.spinics.net/lists/linux-pci/msg71006.html
>   v3: https://www.spinics.net/lists/linux-acpi/msg80876.html
>   v2: https://www.spinics.net/lists/linux-pci/msg69186.html
>   v1: https://www.spinics.net/lists/linux-acpi/msg80607.html
> 
> Changes from v7:
> 
>   - Rebased on top of pci.git pci/hotplug
>   - Make acpi_get_hp_hw_control_from_firmware() call shpchp_is_native()
>   - Drop !bridge check from shpchp_is_native() and update callers to handle
>     possibly NULL pdev
>   - Split out hotplug_is_native() into a separate patch
> 
> Changes from v6:
> 
>   - Use IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) and IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC)
>     directly instaed of wrapping them in macro
>   - Check for !bridge in the same condition than we check whether certain
>     native hotplug is enabled
>   - Add Rafael's Reviewed-by
> 
> Changes from v5:
> 
>   - Dropped patch "PCI: Take bridge window alignment into account when
>     distributing resources"
>   - Rework pciehp_is_native() to be more stricter
>   - Make standard PCI hotplug driver (SHPC) builtin
>   - Rework the way we request OS control of native PCIe
>     hotplug (pciehp) and standard PCI hotplug (SHPC)
>   - Make acpiphp to avoid bridges where hotplug_is_native() returns true
>     (this is combined from pciehp and SHPC)
>   - Updated changelog of patch [7/12]
>   - Use hotplug_bridges + normal_bridges == 1 in patch [9/12] and also
>     remove one indent level from the loop.
> 
> Because patch [7/12] looks quite different now I dropped Reviewed-by and
> stable tag (it now depends on reworked pciehp/SHCP support that most
> probably is not suitable for stable trees).
> 
> I also was not able to test that SHPC really works because I don't have
> hardware that supports it.
> 
> Changes from v4:
> 
>   - Updated message in patch [8/9] following Rafael's suggestion
>   - Added Rafael's tag to patches 6, 7, 9.
> 
> Changes from v3:
> 
>   - Added Andy's tag to patches [1-5]
>   - Improved changelog and subject line of patch [1/9] to match better what
>     it is trying to solve
>   - Improved comment in patch [1/9]
>   - Improved changelog of patches [2/9] and [4/9] accordingly to match
>     better why they are needed
>   - Added cleanup patches [6-9/9]
> 
> Changes from v2:
> 
>   - Added Rafael's tag to patch [1/5]
>   - Updated changelog of patch [1/5] to include more details about how the
>     problem can be observed from dmesg and lspci output
> 
> Changes from v1:
> 
>   - Drop 'cmax - max ?: 1' and use similar construct than we use in second
>     pass loop in patch [1/5]
>   - Drop unnecessary parentheses in patch [1/5]
>   - Added Rafael's tag to patches [2-5/5]
> 
> Mika Westerberg (7):
>   PCI: Take all bridges into account when calculating bus numbers for extension
>   PCI: Introduce shpchp_is_native()
>   PCI: Introduce hotplug_is_native()
>   ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used
>   ACPI / hotplug / PCI: Mark stale PCI devices disconnected
>   PCI: Move resource distribution for a single bridge outside of the loop
>   PCI: Document return value of pci_scan_bridge() and pci_scan_bridge_extend()

I only see these on the mailing list and in patchwork:

  [PATCH v8 1/7] PCI: Take all bridges into account when calculating bus numbers for extension
  [PATCH v8 2/7] PCI: Introduce shpchp_is_native()
  [PATCH v8 3/7] PCI: Introduce hotplug_is_native()
  [PATCH v8 6/7] PCI: Move resource distribution for a single bridge outside of the loop
  [PATCH v8 7/7] PCI: Document return value of pci_scan_bridge() and pci_scan_bridge_extend()

Looks like patches 4 and 5 are missing for some reason.

Re: [PATCH v8 0/7] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Mika Westerberg]

On Tue, May 29, 2018 at 08:26:17AM -0500, Bjorn Helgaas wrote:
> I only see these on the mailing list and in patchwork:
> 
>   [PATCH v8 1/7] PCI: Take all bridges into account when calculating bus numbers for extension
>   [PATCH v8 2/7] PCI: Introduce shpchp_is_native()
>   [PATCH v8 3/7] PCI: Introduce hotplug_is_native()
>   [PATCH v8 6/7] PCI: Move resource distribution for a single bridge outside of the loop
>   [PATCH v8 7/7] PCI: Document return value of pci_scan_bridge() and pci_scan_bridge_extend()
> 
> Looks like patches 4 and 5 are missing for some reason.

Hmm, weird. I'll resend those in a moment.

[PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Mika Westerberg]

When a system is using native PCIe hotplug for Thunderbolt it will be
only present in the system when there is a device connected. This pretty
much follows the BIOS assisted hotplug behaviour.

Thunderbolt host router integrated PCIe switch has two additional PCIe
downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
(USB 3 host controller) respectively. These downstream bridges are not
marked being hotplug capable. Reason for that is to preserve resources.
Otherwise the OS would distribute remaining resources between all
downstream bridges making these two bridges consume precious resources
of the actual hotplug bridges.

Now, because these two bridges are not marked being hotplug capable the OS
will not enable hotplug interrupt for them either and will not receive
interrupt when devices behind them are hot-added. Solution to this is
that the BIOS sends ACPI Notify() to the root port let the OS know it
needs to rescan for added and/or removed devices.

Here is how the mechanism is supposed to work when a Thunderbolt
endpoint is connected to one of the ports. In case of a standard USB-C
device only the xHCI is hot-added otherwise steps are the same.

1. Initially there is only the PCIe root port that is controlled by
   the pciehp driver

  00:1b.0 (Hotplug+) --

2. Then we get native PCIe hotplug interrupt and once it is handled the
   topology looks as following

  00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 --

3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
   they don't have anything behind them currently. Bridge 02:01.0 is
   hotplug capable and used for extending the topology. At this point
   the required PCIe devices are enabled and ACPI Notify() is sent to
   the root port. The resulting topology is expected to look like

  00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 -- xHCI host controller

However, the current ACPI hotplug implementation scans the whole 00:1b.0
hotplug slot and everything behind it regardless whether native PCIe is
used or not, and it expects that the BIOS has configured bridge
resources upfront. If that's not the case it assigns resources using
minimal allocation (everything currently found just barely fit)
preventing future extension. In addition to that, if there is another
native PCIe hotplug going on we may find the new PCIe switch only
partially ready (all links are not fully trained yet) confusing pciehp
when it finally starts to enumerate for new devices.

To make this work better with the native PCIe (pciehp) and standard PCI
(shpchp) hotplug drivers, we let them handle all slot management and
resource allocation for hotplug bridges and restrict ACPI hotplug to
non-hotplug bridges.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 75 +++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 318b6a6f6341..e2bcd9fc3fd2 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -287,11 +287,12 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
 	/*
 	 * Expose slots to user space for functions that have _EJ0 or _RMV or
 	 * are located in dock stations.  Do not expose them for devices handled
-	 * by the native PCIe hotplug (PCIeHP), becuase that code is supposed to
-	 * expose slots to user space in those cases.
+	 * by the native PCIe hotplug (PCIeHP) or standard PCI hotplug
+	 * (SHPCHP), because that code is supposed to expose slots to user
+	 * space in those cases.
 	 */
 	if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
-	    && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+	    && !(pdev && hotplug_is_native(pdev))) {
 		unsigned long long sun;
 		int retval;
 
@@ -430,6 +431,29 @@ static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
 	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
 }
 
+static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
+{
+	struct pci_bus *bus = bridge->subordinate;
+	struct pci_dev *dev;
+	int max;
+
+	if (!bus)
+		return;
+
+	max = bus->busn_res.start;
+	/* Scan already configured non-hotplug bridges */
+	for_each_pci_bridge(dev, bus) {
+		if (!dev->is_hotplug_bridge)
+			max = pci_scan_bridge(bus, dev, max, 0);
+	}
+
+	/* Scan non-hotplug bridges that need to be reconfigured */
+	for_each_pci_bridge(dev, bus) {
+		if (!dev->is_hotplug_bridge)
+			max = pci_scan_bridge(bus, dev, max, 1);
+	}
+}
+
 /**
  * enable_slot - enable, configure a slot
  * @slot: slot to be enabled
@@ -442,25 +466,42 @@ static void enable_slot(struct acpiphp_slot *slot)
 	struct pci_dev *dev;
 	struct pci_bus *bus = slot->bus;
 	struct acpiphp_func *func;
-	int max, pass;
-	LIST_HEAD(add_list);
 
-	acpiphp_rescan_slot(slot);
-	max = acpiphp_max_busnr(bus);
-	for (pass = 0; pass < 2; pass++) {
+	if (bus->self && hotplug_is_native(bus->self)) {
+		/*
+		 * If native hotplug is used, it will take care of hotplug
+		 * slot management and resource allocation for hotplug
+		 * bridges. However, ACPI hotplug may still be used for
+		 * non-hotplug bridges to bring in additional devices such
+		 * as Thunderbolt host controller.
+		 */
 		for_each_pci_bridge(dev, bus) {
-			if (PCI_SLOT(dev->devfn) != slot->device)
-				continue;
-
-			max = pci_scan_bridge(bus, dev, max, pass);
-			if (pass && dev->subordinate) {
-				check_hotplug_bridge(slot, dev);
-				pcibios_resource_survey_bus(dev->subordinate);
-				__pci_bus_size_bridges(dev->subordinate, &add_list);
+			if (PCI_SLOT(dev->devfn) == slot->device)
+				acpiphp_native_scan_bridge(dev);
+		}
+		pci_assign_unassigned_bridge_resources(bus->self);
+	} else {
+		LIST_HEAD(add_list);
+		int max, pass;
+
+		acpiphp_rescan_slot(slot);
+		max = acpiphp_max_busnr(bus);
+		for (pass = 0; pass < 2; pass++) {
+			for_each_pci_bridge(dev, bus) {
+				if (PCI_SLOT(dev->devfn) != slot->device)
+					continue;
+
+				max = pci_scan_bridge(bus, dev, max, pass);
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
+					pcibios_resource_survey_bus(dev->subordinate);
+					__pci_bus_size_bridges(dev->subordinate,
+							       &add_list);
+				}
 			}
 		}
+		__pci_bus_assign_resources(bus, &add_list, NULL);
 	}
-	__pci_bus_assign_resources(bus, &add_list, NULL);
 
 	acpiphp_sanitize_bus(bus);
 	pcie_bus_configure_settings(bus);
-- 
2.17.0

[PATCH v8 5/7] ACPI/hotplug/PCI: Mark stale PCI devices disconnected

[Mika Westerberg]

Following PCIehp mark the unplugged PCI devices disconnected. This makes
sure PCI core code leaves the now missing hardware registers alone.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index e2bcd9fc3fd2..1355bb5e5535 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -649,6 +649,11 @@ static void trim_stale_devices(struct pci_dev *dev)
 		alive = pci_device_is_present(dev);
 
 	if (!alive) {
+		pci_dev_set_disconnected(dev, NULL);
+		if (pci_has_subordinate(dev))
+			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected,
+				     NULL);
+
 		pci_stop_and_remove_bus_device(dev);
 		if (adev)
 			acpi_bus_trim(adev);
-- 
2.17.0

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Andy Shevchenko]

On Mon, 2018-05-28 at 15:47 +0300, Mika Westerberg wrote:
> In the same way we do for pciehp, introduce a new function
> shpchp_is_native() that returns true whether the bridge should be
> handled by the native SHCP driver. Then convert the driver to use this
> function.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/hotplug/acpi_pcihp.c  |  4 ++--
>  drivers/pci/hotplug/shpchp_core.c |  2 --
>  drivers/pci/pci-acpi.c            | 21 +++++++++++++++++++++
>  include/linux/pci_hotplug.h       |  2 ++
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c
> b/drivers/pci/hotplug/acpi_pcihp.c
> index 597d22aeefc1..3979f89b250a 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct
> pci_dev *pdev)
>  	 * OSHP within the scope of the hotplug controller and its
> parents,
>  	 * up to the host bridge under which this controller exists.
>  	 */
> -	host = pci_find_host_bridge(pdev->bus);
> -	if (host->native_shpc_hotplug)
> +	if (shpchp_is_native(pdev))
>  		return 0;
>  
>  	/* If _OSC exists, we should not evaluate OSHP */
> +	host = pci_find_host_bridge(pdev->bus);
>  	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
>  	if (root->osc_support_set)
>  		goto no_control;
> diff --git a/drivers/pci/hotplug/shpchp_core.c
> b/drivers/pci/hotplug/shpchp_core.c
> index 47decc9b3bb3..0f3711404c2b 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev)
>  	if (dev->vendor == PCI_VENDOR_ID_AMD &&
>  	    dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
>  		return 1;
> -	if (!pci_find_capability(dev, PCI_CAP_ID_SHPC))
> -		return 0;
>  	if (acpi_get_hp_hw_control_from_firmware(dev))
>  		return 0;
>  	return 1;
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 52b8434d4d6e..bb83be0d0e5b 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge)
>  	return host->native_pcie_hotplug;
>  }
>  
> +/**
> + * shpchp_is_native - Check whether a hotplug port is handled by the
> OS
> + * @bridge: Hotplug port to check
> + *
> + * Returns true if the given @bridge is handled by the native SHPC
> hotplug
> + * driver.
> + */
> +bool shpchp_is_native(struct pci_dev *bridge)
> +{
> +	const struct pci_host_bridge *host;
> +
> +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> +		return false;
> +
> +	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
> +		return false;
> +
> +	host = pci_find_host_bridge(bridge->bus);
> +	return host->native_shpc_hotplug;
> +}
> +
>  /**
>   * pci_acpi_wake_bus - Root bus wakeup notification fork function.
>   * @context: Device wakeup context.
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 1f5c935eb0de..4c378368215c 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -164,6 +164,7 @@ struct hotplug_params {
>  int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params
> *hpp);
>  bool pciehp_is_native(struct pci_dev *bridge);
>  int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge);
> +bool shpchp_is_native(struct pci_dev *bridge);
>  int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle
> handle);
>  int acpi_pci_detect_ejectable(acpi_handle handle);
>  #else
> @@ -178,5 +179,6 @@ static inline int
> acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
>  	return 0;
>  }
>  static inline bool pciehp_is_native(struct pci_dev *bridge) { return
> true; }
> +static inline bool shpchp_is_native(struct pci_dev *bridge) { return
> true; }
>  #endif
>  #endif

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v8 3/7] PCI: Introduce hotplug_is_native()

[Andy Shevchenko]

On Mon, 2018-05-28 at 15:47 +0300, Mika Westerberg wrote:
> This helper function can be used for finding out whether the OS is
> supposed to handle native hotplug of a given bridge.
> 

Nice simple helper.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  include/linux/pci_hotplug.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 4c378368215c..cf5e22103f68 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -181,4 +181,9 @@ static inline int
> acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
>  static inline bool pciehp_is_native(struct pci_dev *bridge) { return
> true; }
>  static inline bool shpchp_is_native(struct pci_dev *bridge) { return
> true; }
>  #endif
> +
> +static inline bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> +}
>  #endif

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Andy Shevchenko]

On Tue, 2018-05-29 at 19:01 +0300, Mika Westerberg wrote:
> When a system is using native PCIe hotplug for Thunderbolt it will be
> only present in the system when there is a device connected. This
> pretty
> much follows the BIOS assisted hotplug behaviour.
> 
> Thunderbolt host router integrated PCIe switch has two additional PCIe
> downstream bridges that lead to NHI (Thunderbolt host controller) and
> xHCI
> (USB 3 host controller) respectively. These downstream bridges are not
> marked being hotplug capable. Reason for that is to preserve
> resources.
> Otherwise the OS would distribute remaining resources between all
> downstream bridges making these two bridges consume precious resources
> of the actual hotplug bridges.
> 
> Now, because these two bridges are not marked being hotplug capable
> the OS
> will not enable hotplug interrupt for them either and will not receive
> interrupt when devices behind them are hot-added. Solution to this is
> that the BIOS sends ACPI Notify() to the root port let the OS know it
> needs to rescan for added and/or removed devices.
> 
> Here is how the mechanism is supposed to work when a Thunderbolt
> endpoint is connected to one of the ports. In case of a standard USB-C
> device only the xHCI is hot-added otherwise steps are the same.
> 
> 1. Initially there is only the PCIe root port that is controlled by
>    the pciehp driver
> 
>   00:1b.0 (Hotplug+) --
> 
> 2. Then we get native PCIe hotplug interrupt and once it is handled
> the
>    topology looks as following
> 
>   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
>                                   +- 02:01.0 (HotPlug+)
>                                   \- 02:02.0 --
> 
> 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
>    they don't have anything behind them currently. Bridge 02:01.0 is
>    hotplug capable and used for extending the topology. At this point
>    the required PCIe devices are enabled and ACPI Notify() is sent to
>    the root port. The resulting topology is expected to look like
> 
>   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host
> controller
>                                   +- 02:01.0 (HotPlug+)
>                                   \- 02:02.0 -- xHCI host controller
> 
> However, the current ACPI hotplug implementation scans the whole
> 00:1b.0
> hotplug slot and everything behind it regardless whether native PCIe
> is
> used or not, and it expects that the BIOS has configured bridge
> resources upfront. If that's not the case it assigns resources using
> minimal allocation (everything currently found just barely fit)
> preventing future extension. In addition to that, if there is another
> native PCIe hotplug going on we may find the new PCIe switch only
> partially ready (all links are not fully trained yet) confusing pciehp
> when it finally starts to enumerate for new devices.
> 
> To make this work better with the native PCIe (pciehp) and standard
> PCI
> (shpchp) hotplug drivers, we let them handle all slot management and
> resource allocation for hotplug bridges and restrict ACPI hotplug to
> non-hotplug bridges.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 75 +++++++++++++++++++++++----
> ---
>  1 file changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
> b/drivers/pci/hotplug/acpiphp_glue.c
> index 318b6a6f6341..e2bcd9fc3fd2 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -287,11 +287,12 @@ static acpi_status
> acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
>  	/*
>  	 * Expose slots to user space for functions that have _EJ0 or
> _RMV or
>  	 * are located in dock stations.  Do not expose them for
> devices handled
> -	 * by the native PCIe hotplug (PCIeHP), becuase that code is
> supposed to
> -	 * expose slots to user space in those cases.
> +	 * by the native PCIe hotplug (PCIeHP) or standard PCI
> hotplug
> +	 * (SHPCHP), because that code is supposed to expose slots to
> user
> +	 * space in those cases.
>  	 */
>  	if ((acpi_pci_check_ejectable(pbus, handle) ||
> is_dock_device(adev))
> -	    && !(pdev && pdev->is_hotplug_bridge &&
> pciehp_is_native(pdev))) {
> +	    && !(pdev && hotplug_is_native(pdev))) {
>  		unsigned long long sun;
>  		int retval;
>  
> @@ -430,6 +431,29 @@ static int acpiphp_rescan_slot(struct
> acpiphp_slot *slot)
>  	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
>  }
>  
> +static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
> +{
> +	struct pci_bus *bus = bridge->subordinate;
> +	struct pci_dev *dev;
> +	int max;
> +
> +	if (!bus)
> +		return;
> +
> +	max = bus->busn_res.start;
> +	/* Scan already configured non-hotplug bridges */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)
> +			max = pci_scan_bridge(bus, dev, max, 0);
> +	}
> +
> +	/* Scan non-hotplug bridges that need to be reconfigured */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)
> +			max = pci_scan_bridge(bus, dev, max, 1);
> +	}
> +}
> +
>  /**
>   * enable_slot - enable, configure a slot
>   * @slot: slot to be enabled
> @@ -442,25 +466,42 @@ static void enable_slot(struct acpiphp_slot
> *slot)
>  	struct pci_dev *dev;
>  	struct pci_bus *bus = slot->bus;
>  	struct acpiphp_func *func;
> -	int max, pass;
> -	LIST_HEAD(add_list);
>  
> -	acpiphp_rescan_slot(slot);
> -	max = acpiphp_max_busnr(bus);
> -	for (pass = 0; pass < 2; pass++) {
> +	if (bus->self && hotplug_is_native(bus->self)) {
> +		/*
> +		 * If native hotplug is used, it will take care of
> hotplug
> +		 * slot management and resource allocation for
> hotplug
> +		 * bridges. However, ACPI hotplug may still be used
> for
> +		 * non-hotplug bridges to bring in additional devices
> such
> +		 * as Thunderbolt host controller.
> +		 */
>  		for_each_pci_bridge(dev, bus) {
> -			if (PCI_SLOT(dev->devfn) != slot->device)
> -				continue;
> -
> -			max = pci_scan_bridge(bus, dev, max, pass);
> -			if (pass && dev->subordinate) {
> -				check_hotplug_bridge(slot, dev);
> -				pcibios_resource_survey_bus(dev-
> >subordinate);
> -				__pci_bus_size_bridges(dev-
> >subordinate, &add_list);
> +			if (PCI_SLOT(dev->devfn) == slot->device)
> +				acpiphp_native_scan_bridge(dev);
> +		}
> +		pci_assign_unassigned_bridge_resources(bus->self);
> +	} else {
> +		LIST_HEAD(add_list);
> +		int max, pass;
> +
> +		acpiphp_rescan_slot(slot);
> +		max = acpiphp_max_busnr(bus);
> +		for (pass = 0; pass < 2; pass++) {
> +			for_each_pci_bridge(dev, bus) {
> +				if (PCI_SLOT(dev->devfn) != slot-
> >device)
> +					continue;
> +
> +				max = pci_scan_bridge(bus, dev, max,
> pass);
> +				if (pass && dev->subordinate) {
> +					check_hotplug_bridge(slot,
> dev);
> +					pcibios_resource_survey_bus(d
> ev->subordinate);
> +					__pci_bus_size_bridges(dev-
> >subordinate,
> +							       &add_l
> ist);
> +				}
>  			}
>  		}
> +		__pci_bus_assign_resources(bus, &add_list, NULL);
>  	}
> -	__pci_bus_assign_resources(bus, &add_list, NULL);
>  
>  	acpiphp_sanitize_bus(bus);
>  	pcie_bus_configure_settings(bus);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v8 6/7] PCI: Move resource distribution for a single bridge outside of the loop

[Andy Shevchenko]

On Mon, 2018-05-28 at 15:47 +0300, Mika Westerberg wrote:
> There is a special case where there is only single bridge on the bus.
> In
> that case we just assign all resources to it. Currently this is done
> as
> a part of the resource distribution loop but it does not have to be
> there, and moving it outside actually improves readability because we
> can then save one indent level in the loop.
> 
> While there we can add hotplug_bridges == 1 && normal_bridges == 0 to
> the same block because they are dealt the same way.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/setup-bus.c | 82 ++++++++++++++++++++------------------
> ---
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 072784f55ea5..79b1824e83b4 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1942,57 +1942,57 @@ static void
> pci_bus_distribute_available_resources(struct pci_bus *bus,
>  			remaining_mmio_pref -= resource_size(res);
>  	}
>  
> +	/*
> +	 * There is only one bridge on the bus so it gets all
> available
> +	 * resources which it can then distribute to the possible
> +	 * hotplug bridges below.
> +	 */
> +	if (hotplug_bridges + normal_bridges == 1) {
> +		dev = list_first_entry(&bus->devices, struct pci_dev,
> bus_list);
> +		if (dev->subordinate) {
> +			pci_bus_distribute_available_resources(dev-
> >subordinate,
> +				add_list, available_io,
> available_mmio,
> +				available_mmio_pref);
> +		}
> +		return;
> +	}
> +
>  	/*
>  	 * Go over devices on this bus and distribute the remaining
>  	 * resource space between hotplug bridges.
>  	 */
>  	for_each_pci_bridge(dev, bus) {
> +		resource_size_t align, io, mmio, mmio_pref;
>  		struct pci_bus *b;
>  
>  		b = dev->subordinate;
> -		if (!b)
> +		if (!b || !dev->is_hotplug_bridge)
>  			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);
> -		}
> +		/*
> +		 * 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);
>  	}
>  }
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v8 7/7] PCI: Document return value of pci_scan_bridge() and pci_scan_bridge_extend()

[Andy Shevchenko]

On Mon, 2018-05-28 at 15:47 +0300, Mika Westerberg wrote:
> It is not immediately clear what the two functions actually return so
> add kernel-doc comment explaining it a bit better.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/probe.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8f384e7ca2c2..7dc0b3c49941 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -999,6 +999,8 @@ static unsigned int
> pci_scan_child_bus_extend(struct pci_bus *bus,
>   * 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.
> + *
> + * Return: New subordinate number covering all buses behind this
> bridge.
>   */
>  static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev
> *dev,
>  				  int max, unsigned int
> available_buses,
> @@ -1226,6 +1228,8 @@ static int pci_scan_bridge_extend(struct pci_bus
> *bus, struct pci_dev *dev,
>   * 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.
> + *
> + * Return: New subordinate number covering all buses behind this
> bridge.
>   */
>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int
> max, int pass)
>  {

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Bjorn Helgaas]

[+cc David]

On Mon, May 28, 2018 at 03:47:51PM +0300, Mika Westerberg wrote:
> In the same way we do for pciehp, introduce a new function
> shpchp_is_native() that returns true whether the bridge should be
> handled by the native SHCP driver. Then convert the driver to use this
> function.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/hotplug/acpi_pcihp.c  |  4 ++--
>  drivers/pci/hotplug/shpchp_core.c |  2 --
>  drivers/pci/pci-acpi.c            | 21 +++++++++++++++++++++
>  include/linux/pci_hotplug.h       |  2 ++
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index 597d22aeefc1..3979f89b250a 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
>  	 * OSHP within the scope of the hotplug controller and its parents,
>  	 * up to the host bridge under which this controller exists.
>  	 */
> -	host = pci_find_host_bridge(pdev->bus);
> -	if (host->native_shpc_hotplug)
> +	if (shpchp_is_native(pdev))
>  		return 0;
>  
>  	/* If _OSC exists, we should not evaluate OSHP */
> +	host = pci_find_host_bridge(pdev->bus);
>  	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
>  	if (root->osc_support_set)
>  		goto no_control;
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index 47decc9b3bb3..0f3711404c2b 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev)
>  	if (dev->vendor == PCI_VENDOR_ID_AMD &&
>  	    dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
>  		return 1;
> -	if (!pci_find_capability(dev, PCI_CAP_ID_SHPC))
> -		return 0;
>  	if (acpi_get_hp_hw_control_from_firmware(dev))
>  		return 0;
>  	return 1;
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 52b8434d4d6e..bb83be0d0e5b 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge)
>  	return host->native_pcie_hotplug;
>  }
>  
> +/**
> + * shpchp_is_native - Check whether a hotplug port is handled by the OS
> + * @bridge: Hotplug port to check
> + *
> + * Returns true if the given @bridge is handled by the native SHPC hotplug
> + * driver.
> + */
> +bool shpchp_is_native(struct pci_dev *bridge)
> +{
> +	const struct pci_host_bridge *host;
> +
> +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> +		return false;
> +
> +	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
> +		return false;
> +
> +	host = pci_find_host_bridge(bridge->bus);
> +	return host->native_shpc_hotplug;
> +}

This is sort-of-but-not-exactly the same as is_shpc_capable().

For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true
and shpchp will claim the device, but shpchp_is_native() will return
false because there's no SHPC capability.

At least that's my interpretation of the AMD GOLAM stuff.  I don't
have a spec for it, but if GOLAM did have an SHPC capability, I assume
is_shpc_capable() would look for it *before* testing for GOLAM.

So I think these two things need to be reconciled somehow.  I
mentioned this before, but it was buried at the bottom of a long
email, sorry about that.

I wish we had a spec or details about the erratum.  It looks like
it's been there "forever": https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/hotplug/shpchp.h?id=c16b4b14d9806e639f4afefa2d651a857a212afe

But neither GOLAM (0x7450) nor POGO (0x7458) is in the PCI database at
https://pci-ids.ucw.cz/read/PC/1002, so who knows if those chips ever
even saw the light of day.  I'll cc the author of

  53044f357448 ("[PATCH] PCI Hotplug: shpchp: AMD POGO errata fix")

But that was 12 years ago, so the email address may not even work any
more.

>  /**
>   * pci_acpi_wake_bus - Root bus wakeup notification fork function.
>   * @context: Device wakeup context.
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 1f5c935eb0de..4c378368215c 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -164,6 +164,7 @@ struct hotplug_params {
>  int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp);
>  bool pciehp_is_native(struct pci_dev *bridge);
>  int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge);
> +bool shpchp_is_native(struct pci_dev *bridge);
>  int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
>  int acpi_pci_detect_ejectable(acpi_handle handle);
>  #else
> @@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
>  	return 0;
>  }
>  static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
> +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>  #endif
>  #endif
> -- 
> 2.17.0
> 

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Bjorn Helgaas]

[-cc David, his email bounced]

On Wed, May 30, 2018 at 03:23:43PM -0500, Bjorn Helgaas wrote:
> [+cc David]
> 
> On Mon, May 28, 2018 at 03:47:51PM +0300, Mika Westerberg wrote:
> > In the same way we do for pciehp, introduce a new function
> > shpchp_is_native() that returns true whether the bridge should be
> > handled by the native SHCP driver. Then convert the driver to use this
> > function.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/hotplug/acpi_pcihp.c  |  4 ++--
> >  drivers/pci/hotplug/shpchp_core.c |  2 --
> >  drivers/pci/pci-acpi.c            | 21 +++++++++++++++++++++
> >  include/linux/pci_hotplug.h       |  2 ++
> >  4 files changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> > index 597d22aeefc1..3979f89b250a 100644
> > --- a/drivers/pci/hotplug/acpi_pcihp.c
> > +++ b/drivers/pci/hotplug/acpi_pcihp.c
> > @@ -83,11 +83,11 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
> >  	 * OSHP within the scope of the hotplug controller and its parents,
> >  	 * up to the host bridge under which this controller exists.
> >  	 */
> > -	host = pci_find_host_bridge(pdev->bus);
> > -	if (host->native_shpc_hotplug)
> > +	if (shpchp_is_native(pdev))
> >  		return 0;
> >  
> >  	/* If _OSC exists, we should not evaluate OSHP */
> > +	host = pci_find_host_bridge(pdev->bus);
> >  	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
> >  	if (root->osc_support_set)
> >  		goto no_control;
> > diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> > index 47decc9b3bb3..0f3711404c2b 100644
> > --- a/drivers/pci/hotplug/shpchp_core.c
> > +++ b/drivers/pci/hotplug/shpchp_core.c
> > @@ -275,8 +275,6 @@ static int is_shpc_capable(struct pci_dev *dev)
> >  	if (dev->vendor == PCI_VENDOR_ID_AMD &&
> >  	    dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
> >  		return 1;
> > -	if (!pci_find_capability(dev, PCI_CAP_ID_SHPC))
> > -		return 0;
> >  	if (acpi_get_hp_hw_control_from_firmware(dev))
> >  		return 0;
> >  	return 1;
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 52b8434d4d6e..bb83be0d0e5b 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -394,6 +394,27 @@ bool pciehp_is_native(struct pci_dev *bridge)
> >  	return host->native_pcie_hotplug;
> >  }
> >  
> > +/**
> > + * shpchp_is_native - Check whether a hotplug port is handled by the OS
> > + * @bridge: Hotplug port to check
> > + *
> > + * Returns true if the given @bridge is handled by the native SHPC hotplug
> > + * driver.
> > + */
> > +bool shpchp_is_native(struct pci_dev *bridge)
> > +{
> > +	const struct pci_host_bridge *host;
> > +
> > +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> > +		return false;
> > +
> > +	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
> > +		return false;
> > +
> > +	host = pci_find_host_bridge(bridge->bus);
> > +	return host->native_shpc_hotplug;
> > +}
> 
> This is sort-of-but-not-exactly the same as is_shpc_capable().
> 
> For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true
> and shpchp will claim the device, but shpchp_is_native() will return
> false because there's no SHPC capability.
> 
> At least that's my interpretation of the AMD GOLAM stuff.  I don't
> have a spec for it, but if GOLAM did have an SHPC capability, I assume
> is_shpc_capable() would look for it *before* testing for GOLAM.
> 
> So I think these two things need to be reconciled somehow.  I
> mentioned this before, but it was buried at the bottom of a long
> email, sorry about that.
> 
> I wish we had a spec or details about the erratum.  It looks like
> it's been there "forever": https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/hotplug/shpchp.h?id=c16b4b14d9806e639f4afefa2d651a857a212afe
> 
> But neither GOLAM (0x7450) nor POGO (0x7458) is in the PCI database at
> https://pci-ids.ucw.cz/read/PC/1002, so who knows if those chips ever
> even saw the light of day.  I'll cc the author of
> 
>   53044f357448 ("[PATCH] PCI Hotplug: shpchp: AMD POGO errata fix")
> 
> But that was 12 years ago, so the email address may not even work any
> more.
> 
> >  /**
> >   * pci_acpi_wake_bus - Root bus wakeup notification fork function.
> >   * @context: Device wakeup context.
> > diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> > index 1f5c935eb0de..4c378368215c 100644
> > --- a/include/linux/pci_hotplug.h
> > +++ b/include/linux/pci_hotplug.h
> > @@ -164,6 +164,7 @@ struct hotplug_params {
> >  int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp);
> >  bool pciehp_is_native(struct pci_dev *bridge);
> >  int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge);
> > +bool shpchp_is_native(struct pci_dev *bridge);
> >  int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
> >  int acpi_pci_detect_ejectable(acpi_handle handle);
> >  #else
> > @@ -178,5 +179,6 @@ static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
> >  	return 0;
> >  }
> >  static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
> > +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
> >  #endif
> >  #endif
> > -- 
> > 2.17.0
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Mika Westerberg]

On Wed, May 30, 2018 at 03:23:43PM -0500, Bjorn Helgaas wrote:
> > +{
> > +	const struct pci_host_bridge *host;
> > +
> > +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> > +		return false;
> > +
> > +	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
> > +		return false;
> > +
> > +	host = pci_find_host_bridge(bridge->bus);
> > +	return host->native_shpc_hotplug;
> > +}
> 
> This is sort-of-but-not-exactly the same as is_shpc_capable().
> 
> For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true
> and shpchp will claim the device, but shpchp_is_native() will return
> false because there's no SHPC capability.
> 
> At least that's my interpretation of the AMD GOLAM stuff.  I don't
> have a spec for it, but if GOLAM did have an SHPC capability, I assume
> is_shpc_capable() would look for it *before* testing for GOLAM.

Most probably it did not have SHPC capability because I find it hard to
explain the check otherwise.

> So I think these two things need to be reconciled somehow.  I
> mentioned this before, but it was buried at the bottom of a long
> email, sorry about that.

No it wasn't. I read it and did exactly what you wanted. Now there is no
duplication in these two functions. is_shpc_capable() calls
acpi_get_hp_hw_control_from_firmware() which calls shpchp_is_native().
In fact I don't think is_shpc_capable() warrants to exist at all and it
should be removed (but in another separate cleanup patch).

> I wish we had a spec or details about the erratum.  It looks like
> it's been there "forever": https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/hotplug/shpchp.h?id=c16b4b14d9806e639f4afefa2d651a857a212afe
> 
> But neither GOLAM (0x7450) nor POGO (0x7458) is in the PCI database at
> https://pci-ids.ucw.cz/read/PC/1002, so who knows if those chips ever
> even saw the light of day.  I'll cc the author of
> 
>   53044f357448 ("[PATCH] PCI Hotplug: shpchp: AMD POGO errata fix")
> 
> But that was 12 years ago, so the email address may not even work any
> more.

Ín any case even if somehow the original patch from 2006 was wrong, I
don't have absolutely any idea why it needs to be fixed now in this
patch series? Given that there are two real fixes in this series that
fix real issues on real contemporary hardware, I don't really understand
why you are stalling them. Yes, it is good to do cleanups because it
makes the code easier to understand and thus more maintainable but
that's something typically not priorized as high as fixes for real
problems.

These fixes have been out there since february virtually unchanged, so
you would think they have had enough review already. If not please point
out what exactly I need to fix and I'll do that. Otherwise please
consider taking the series for v4.18.

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Bjorn Helgaas]

On Thu, May 31, 2018 at 09:58:52AM +0300, Mika Westerberg wrote:
> On Wed, May 30, 2018 at 03:23:43PM -0500, Bjorn Helgaas wrote:
> > > +{
> > > +	const struct pci_host_bridge *host;
> > > +
> > > +	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> > > +		return false;
> > > +
> > > +	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
> > > +		return false;
> > > +
> > > +	host = pci_find_host_bridge(bridge->bus);
> > > +	return host->native_shpc_hotplug;
> > > +}
> > 
> > This is sort-of-but-not-exactly the same as is_shpc_capable().
> > 
> > For PCI_DEVICE_ID_AMD_GOLAM_7450, is_shpc_capable() will return true
> > and shpchp will claim the device, but shpchp_is_native() will return
> > false because there's no SHPC capability.
> > 
> > At least that's my interpretation of the AMD GOLAM stuff.  I don't
> > have a spec for it, but if GOLAM did have an SHPC capability, I assume
> > is_shpc_capable() would look for it *before* testing for GOLAM.
> 
> Most probably it did not have SHPC capability because I find it hard to
> explain the check otherwise.
> 
> > So I think these two things need to be reconciled somehow.  I
> > mentioned this before, but it was buried at the bottom of a long
> > email, sorry about that.
> 
> No it wasn't. I read it and did exactly what you wanted. Now there is no
> duplication in these two functions. is_shpc_capable() calls
> acpi_get_hp_hw_control_from_firmware() which calls shpchp_is_native().
> In fact I don't think is_shpc_capable() warrants to exist at all and it
> should be removed (but in another separate cleanup patch).

Maybe I'm reading your patches wrong.  It looks to me like shpchp will
claim GOLAM_7450, which means shpchp will register slots, program the
SHPC, handle hotplug interrupts, etc.

But since shpchp_is_native() returns false, acpiphp thinks *it* should
handle hotplug.  For example, I think that given some ACPI
prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register():

  shpc_probe
    is_shpc_capable             # true for GOLAM_7450
    init_slots
      pci_hp_register

  acpi_pci_add_slots
    acpiphp_enumerate_slots
      acpi_walk_namespace(..., acpiphp_add_context)
        acpiphp_add_context
          hotplug_is_native     # false for GOLAM_7450
          acpiphp_register_hotplug_slot
            pci_hp_register

It is true that the same situation occurred before your patches, since
acpiphp_add_context() only checked pciehp_is_native().  In fact, with
the existing code, shpchp and acpiphp could both try to manage *any*
SHPC, not just GOLAM_7450.

I think the current series fixes 99% of that problem and it seems like
we should try to do that last 1% at the same time so the SHPC code
makes more sense.

Bjorn

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Mika Westerberg]

On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote:
> Maybe I'm reading your patches wrong.  It looks to me like shpchp will
> claim GOLAM_7450, which means shpchp will register slots, program the
> SHPC, handle hotplug interrupts, etc.
> 
> But since shpchp_is_native() returns false, acpiphp thinks *it* should
> handle hotplug.  For example, I think that given some ACPI
> prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register():
> 
>   shpc_probe
>     is_shpc_capable             # true for GOLAM_7450
>     init_slots
>       pci_hp_register
> 
>   acpi_pci_add_slots
>     acpiphp_enumerate_slots
>       acpi_walk_namespace(..., acpiphp_add_context)
>         acpiphp_add_context
>           hotplug_is_native     # false for GOLAM_7450
>           acpiphp_register_hotplug_slot
>             pci_hp_register
> 
> It is true that the same situation occurred before your patches, since
> acpiphp_add_context() only checked pciehp_is_native().  In fact, with
> the existing code, shpchp and acpiphp could both try to manage *any*
> SHPC, not just GOLAM_7450.
> 
> I think the current series fixes 99% of that problem and it seems like
> we should try to do that last 1% at the same time so the SHPC code
> makes more sense.

Would the following fix the last 1% for you? Applies on top of this
patch.

diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index 9675ab757323..516e4835019c 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h
@@ -105,7 +105,6 @@ struct controller {
 };
 
 /* Define AMD SHPC ID  */
-#define PCI_DEVICE_ID_AMD_GOLAM_7450	0x7450
 #define PCI_DEVICE_ID_AMD_POGO_7458	0x7458
 
 /* AMD PCI-X bridge registers */
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 0f3711404c2b..e91be287f292 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -270,22 +270,12 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	return 0;
 }
 
-static int is_shpc_capable(struct pci_dev *dev)
-{
-	if (dev->vendor == PCI_VENDOR_ID_AMD &&
-	    dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
-		return 1;
-	if (acpi_get_hp_hw_control_from_firmware(dev))
-		return 0;
-	return 1;
-}
-
 static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	int rc;
 	struct controller *ctrl;
 
-	if (!is_shpc_capable(pdev))
+	if (acpi_get_hp_hw_control_from_firmware(pdev))
 		return -ENODEV;
 
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index bbc4ea70505a..fd1c0ee33805 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -408,6 +408,14 @@ bool shpchp_is_native(struct pci_dev *bridge)
 	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		return false;
 
+	/*
+	 * It is assumed that AMD GOLAM chips support SHPC but they do not
+	 * have SHPC capability.
+	 */
+	if (bridge->vendor == PCI_VENDOR_ID_AMD &&
+	    bridge->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
+		return true;
+
 	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
 		return false;
 
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 883cb7bf78aa..5aace6cca0d7 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -561,6 +561,7 @@
 #define PCI_DEVICE_ID_AMD_OPUS_7443	0x7443
 #define PCI_DEVICE_ID_AMD_VIPER_7443	0x7443
 #define PCI_DEVICE_ID_AMD_OPUS_7445	0x7445
+#define PCI_DEVICE_ID_AMD_GOLAM_7450	0x7450
 #define PCI_DEVICE_ID_AMD_8111_PCI	0x7460
 #define PCI_DEVICE_ID_AMD_8111_LPC	0x7468
 #define PCI_DEVICE_ID_AMD_8111_IDE	0x7469

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Bjorn Helgaas]

On Thu, May 31, 2018 at 04:51:17PM +0300, Mika Westerberg wrote:
> On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote:
> > Maybe I'm reading your patches wrong.  It looks to me like shpchp will
> > claim GOLAM_7450, which means shpchp will register slots, program the
> > SHPC, handle hotplug interrupts, etc.
> > 
> > But since shpchp_is_native() returns false, acpiphp thinks *it* should
> > handle hotplug.  For example, I think that given some ACPI
> > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register():
> > 
> >   shpc_probe
> >     is_shpc_capable             # true for GOLAM_7450
> >     init_slots
> >       pci_hp_register
> > 
> >   acpi_pci_add_slots
> >     acpiphp_enumerate_slots
> >       acpi_walk_namespace(..., acpiphp_add_context)
> >         acpiphp_add_context
> >           hotplug_is_native     # false for GOLAM_7450
> >           acpiphp_register_hotplug_slot
> >             pci_hp_register
> > 
> > It is true that the same situation occurred before your patches, since
> > acpiphp_add_context() only checked pciehp_is_native().  In fact, with
> > the existing code, shpchp and acpiphp could both try to manage *any*
> > SHPC, not just GOLAM_7450.
> > 
> > I think the current series fixes 99% of that problem and it seems like
> > we should try to do that last 1% at the same time so the SHPC code
> > makes more sense.
> 
> Would the following fix the last 1% for you? Applies on top of this
> patch.

Yes, that's exactly what I was looking for!  Thanks!

> diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
> index 9675ab757323..516e4835019c 100644
> --- a/drivers/pci/hotplug/shpchp.h
> +++ b/drivers/pci/hotplug/shpchp.h
> @@ -105,7 +105,6 @@ struct controller {
>  };
>  
>  /* Define AMD SHPC ID  */
> -#define PCI_DEVICE_ID_AMD_GOLAM_7450	0x7450
>  #define PCI_DEVICE_ID_AMD_POGO_7458	0x7458
>  
>  /* AMD PCI-X bridge registers */
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index 0f3711404c2b..e91be287f292 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -270,22 +270,12 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
>  	return 0;
>  }
>  
> -static int is_shpc_capable(struct pci_dev *dev)
> -{
> -	if (dev->vendor == PCI_VENDOR_ID_AMD &&
> -	    dev->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
> -		return 1;
> -	if (acpi_get_hp_hw_control_from_firmware(dev))
> -		return 0;
> -	return 1;
> -}
> -
>  static int shpc_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	int rc;
>  	struct controller *ctrl;
>  
> -	if (!is_shpc_capable(pdev))
> +	if (acpi_get_hp_hw_control_from_firmware(pdev))
>  		return -ENODEV;
>  
>  	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index bbc4ea70505a..fd1c0ee33805 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -408,6 +408,14 @@ bool shpchp_is_native(struct pci_dev *bridge)
>  	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>  		return false;
>  
> +	/*
> +	 * It is assumed that AMD GOLAM chips support SHPC but they do not
> +	 * have SHPC capability.
> +	 */
> +	if (bridge->vendor == PCI_VENDOR_ID_AMD &&
> +	    bridge->device == PCI_DEVICE_ID_AMD_GOLAM_7450)
> +		return true;
> +
>  	if (!pci_find_capability(bridge, PCI_CAP_ID_SHPC))
>  		return false;
>  
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 883cb7bf78aa..5aace6cca0d7 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -561,6 +561,7 @@
>  #define PCI_DEVICE_ID_AMD_OPUS_7443	0x7443
>  #define PCI_DEVICE_ID_AMD_VIPER_7443	0x7443
>  #define PCI_DEVICE_ID_AMD_OPUS_7445	0x7445
> +#define PCI_DEVICE_ID_AMD_GOLAM_7450	0x7450
>  #define PCI_DEVICE_ID_AMD_8111_PCI	0x7460
>  #define PCI_DEVICE_ID_AMD_8111_LPC	0x7468
>  #define PCI_DEVICE_ID_AMD_8111_IDE	0x7469

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Mika Westerberg]

On Thu, May 31, 2018 at 11:55:56AM -0500, Bjorn Helgaas wrote:
> On Thu, May 31, 2018 at 04:51:17PM +0300, Mika Westerberg wrote:
> > On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote:
> > > Maybe I'm reading your patches wrong.  It looks to me like shpchp will
> > > claim GOLAM_7450, which means shpchp will register slots, program the
> > > SHPC, handle hotplug interrupts, etc.
> > > 
> > > But since shpchp_is_native() returns false, acpiphp thinks *it* should
> > > handle hotplug.  For example, I think that given some ACPI
> > > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register():
> > > 
> > >   shpc_probe
> > >     is_shpc_capable             # true for GOLAM_7450
> > >     init_slots
> > >       pci_hp_register
> > > 
> > >   acpi_pci_add_slots
> > >     acpiphp_enumerate_slots
> > >       acpi_walk_namespace(..., acpiphp_add_context)
> > >         acpiphp_add_context
> > >           hotplug_is_native     # false for GOLAM_7450
> > >           acpiphp_register_hotplug_slot
> > >             pci_hp_register
> > > 
> > > It is true that the same situation occurred before your patches, since
> > > acpiphp_add_context() only checked pciehp_is_native().  In fact, with
> > > the existing code, shpchp and acpiphp could both try to manage *any*
> > > SHPC, not just GOLAM_7450.
> > > 
> > > I think the current series fixes 99% of that problem and it seems like
> > > we should try to do that last 1% at the same time so the SHPC code
> > > makes more sense.
> > 
> > Would the following fix the last 1% for you? Applies on top of this
> > patch.
> 
> Yes, that's exactly what I was looking for!  Thanks!

Great. Do you want me to update this patch accordingly or will you do
that yourself?

Re: [PATCH v8 2/7] PCI: Introduce shpchp_is_native()

[Bjorn Helgaas]

On Fri, Jun 01, 2018 at 12:27:10PM +0300, Mika Westerberg wrote:
> On Thu, May 31, 2018 at 11:55:56AM -0500, Bjorn Helgaas wrote:
> > On Thu, May 31, 2018 at 04:51:17PM +0300, Mika Westerberg wrote:
> > > On Thu, May 31, 2018 at 08:12:02AM -0500, Bjorn Helgaas wrote:
> > > > Maybe I'm reading your patches wrong.  It looks to me like shpchp will
> > > > claim GOLAM_7450, which means shpchp will register slots, program the
> > > > SHPC, handle hotplug interrupts, etc.
> > > > 
> > > > But since shpchp_is_native() returns false, acpiphp thinks *it* should
> > > > handle hotplug.  For example, I think that given some ACPI
> > > > prerequisites (_EJ0/_RMV/etc), both will call pci_hp_register():
> > > > 
> > > >   shpc_probe
> > > >     is_shpc_capable             # true for GOLAM_7450
> > > >     init_slots
> > > >       pci_hp_register
> > > > 
> > > >   acpi_pci_add_slots
> > > >     acpiphp_enumerate_slots
> > > >       acpi_walk_namespace(..., acpiphp_add_context)
> > > >         acpiphp_add_context
> > > >           hotplug_is_native     # false for GOLAM_7450
> > > >           acpiphp_register_hotplug_slot
> > > >             pci_hp_register
> > > > 
> > > > It is true that the same situation occurred before your patches, since
> > > > acpiphp_add_context() only checked pciehp_is_native().  In fact, with
> > > > the existing code, shpchp and acpiphp could both try to manage *any*
> > > > SHPC, not just GOLAM_7450.
> > > > 
> > > > I think the current series fixes 99% of that problem and it seems like
> > > > we should try to do that last 1% at the same time so the SHPC code
> > > > makes more sense.
> > > 
> > > Would the following fix the last 1% for you? Applies on top of this
> > > patch.
> > 
> > Yes, that's exactly what I was looking for!  Thanks!
> 
> Great. Do you want me to update this patch accordingly or will you do
> that yourself?

No need, I squashed it in already.

Re: [PATCH v8 1/7] PCI: Take all bridges into account when calculating bus numbers for extension

[Bjorn Helgaas]

On Mon, May 28, 2018 at 03:47:50PM +0300, Mika Westerberg wrote:
> When distributing extra buses between hotplug bridges for future
> extension we currently fail to take into account the fact that there
> might be non-hotplug bridges on the bus after the hotplug bridges. In
> this particular system we have following topology:
> 
>   01:00.0 --+- 02:00.0 -- Thunderbolt host controller
>             +- 02:01.0 (HotPlug+)
>             \- 02:02.0 -- xHCI host controller
> 
> Now, pci_scan_child_bus_extend() is supposed to distribute remaining bus
> numbers to the hotplug bridge at 02:01.0 but only after all bridges on
> that bus have been been accounted for. Since we don't check upfront that
> there will be another non-hotplug bridge after the hotplug bridge
> 02:01.0 it will inadvertently extend over the bus space of the
> non-hotplug bridge:

Just to make sure I understand this correctly, I think this situation
arises when we hot-add 01:00.0.  The port above 01:00.0 was probably
programmed by the BIOS like this:

  pci 0000:00:1b.0: PCI bridge to [bus 01-39]

01:00.0 is the only device on bus 01, so we give it everything that's
left:

>   pci 0000:01:00.0: PCI bridge to [bus 02-39]
>   ...

There are three bridges on bus 02, so we need to distribute the
remaining [bus 03-39] range among them.  Previously we only assigned
bus numbers to "is_hotplug_bridge" devices (basically those with
PCI_EXP_SLTCAP_HPC or ACPI hotplug methods).

In this case, the only one with "is_hotplug_bridge" is 02:01.0.

We looked at 02:00.0, which is a bridge but doesn't have
PCI_EXP_SLTCAP_HPC set, so we gave it one secondary bus number:

  pci 0000:02:00.0: PCI bridge to [bus 03]

Then we looked at 02:01.0, which *does* have PCI_EXP_SLTCAP_HPC, and
we know it's the only "is_hotplug_bridge", so we gave it the rest:

  pci 0000:02:01.0: PCI bridge to [bus 04-39]

Then we get to 02:02.0.  There are no bus numbers available, so we
can't reach anything below it.

And this patch fixes it by assigning at least one bus number to every
bridge, even if it doesn't have PCI_EXP_SLTCAP_HPC?

Is that right?

(The messages we print are sort of weird because we claim to scan bus
3a, but config accesses to 3a will never even reach the 01:00.0
bridge.  But that's a problem for another day.)

>   pci_bus 0000:04: [bus 04-39] extended by 0x35
>   pci_bus 0000:04: bus scan returning with max=39
>   pci_bus 0000:04: busn_res: [bus 04-39] end is updated to 39
>   pci 0000:02:02.0: scanning [bus 00-00] behind bridge, pass 1
>   pci_bus 0000:3a: scanning bus
>   pci_bus 0000:3a: bus scan returning with max=3a
>   pci_bus 0000:3a: busn_res: [bus 3a] end is updated to 3a
>   pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39]
>   pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39]
>   pci_bus 0000:02: bus scan returning with max=3a
>   pci_bus 0000:02: busn_res: [bus 02-39] end can not be updated to 3a
> 
> Resulting 'lspci -t' output looks like this:
> 
>   +-1b.0-[01-39]----00.0-[02-3a]--+-00.0-[03]----00.0
>                              ^^   +-01.0-[04-39]--
>                                   \-02.0-[3a]----00.0
>                                           ^^
> The xHCI host controller (3a:00.0) behind downstream bridge at 02:02.0
> is not usable anymore.
> 
> To fix this reserve at least one bus for each bridge during scanning of
> already configured bridges. Then we use this information in the second
> scan to correct the available extra bus space for hotplug bridges.
> 
> After this change the 'lspci -t' output is what is expected:
> 
>   +-1b.0-[01-39]----00.0-[02-39]--+-00.0-[03]----00.0
>                                   +-01.0-[04-38]--
>                                   \-02.0-[39]----00.0
> 
> Fixes: 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")
> Reported-by: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/probe.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index d14e8f827d53..8f384e7ca2c2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2634,7 +2634,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  	for_each_pci_bridge(dev, bus) {
>  		cmax = max;
>  		max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
> -		used_buses += cmax - max;
> +		/*
> +		 * Reserve one bus for each bridge now to avoid
> +		 * extending hotplug bridges too much during the second
> +		 * scan below.
> +		 */
> +		used_buses++;
> +		if (cmax - max > 1)
> +			used_buses += cmax - max - 1;
>  	}
>  
>  	/* Scan bridges that need to be reconfigured */
> @@ -2657,12 +2664,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  			 * bridges if any.
>  			 */
>  			buses = available_buses / hotplug_bridges;
> -			buses = min(buses, available_buses - used_buses);
> +			buses = min(buses, available_buses - used_buses + 1);
>  		}
>  
>  		cmax = max;
>  		max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
> -		used_buses += max - cmax;
> +		/* One bus is already accounted so don't add it again */
> +		if (max - cmax > 1)
> +			used_buses += max - cmax - 1;
>  	}
>  
>  	/*
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 1/7] PCI: Take all bridges into account when calculating bus numbers for extension

[Mika Westerberg]

On Fri, Jun 01, 2018 at 08:49:58AM -0500, Bjorn Helgaas wrote:
> On Mon, May 28, 2018 at 03:47:50PM +0300, Mika Westerberg wrote:
> > When distributing extra buses between hotplug bridges for future
> > extension we currently fail to take into account the fact that there
> > might be non-hotplug bridges on the bus after the hotplug bridges. In
> > this particular system we have following topology:
> > 
> >   01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> >             +- 02:01.0 (HotPlug+)
> >             \- 02:02.0 -- xHCI host controller
> > 
> > Now, pci_scan_child_bus_extend() is supposed to distribute remaining bus
> > numbers to the hotplug bridge at 02:01.0 but only after all bridges on
> > that bus have been been accounted for. Since we don't check upfront that
> > there will be another non-hotplug bridge after the hotplug bridge
> > 02:01.0 it will inadvertently extend over the bus space of the
> > non-hotplug bridge:
> 
> Just to make sure I understand this correctly, I think this situation
> arises when we hot-add 01:00.0.  The port above 01:00.0 was probably
> programmed by the BIOS like this:
> 
>   pci 0000:00:1b.0: PCI bridge to [bus 01-39]
> 
> 01:00.0 is the only device on bus 01, so we give it everything that's
> left:
> 
> >   pci 0000:01:00.0: PCI bridge to [bus 02-39]
> >   ...
> 
> There are three bridges on bus 02, so we need to distribute the
> remaining [bus 03-39] range among them.  Previously we only assigned
> bus numbers to "is_hotplug_bridge" devices (basically those with
> PCI_EXP_SLTCAP_HPC or ACPI hotplug methods).
> 
> In this case, the only one with "is_hotplug_bridge" is 02:01.0.
> 
> We looked at 02:00.0, which is a bridge but doesn't have
> PCI_EXP_SLTCAP_HPC set, so we gave it one secondary bus number:
> 
>   pci 0000:02:00.0: PCI bridge to [bus 03]
> 
> Then we looked at 02:01.0, which *does* have PCI_EXP_SLTCAP_HPC, and
> we know it's the only "is_hotplug_bridge", so we gave it the rest:
> 
>   pci 0000:02:01.0: PCI bridge to [bus 04-39]
> 
> Then we get to 02:02.0.  There are no bus numbers available, so we
> can't reach anything below it.
> 
> And this patch fixes it by assigning at least one bus number to every
> bridge, even if it doesn't have PCI_EXP_SLTCAP_HPC?
> 
> Is that right?

Yes, that's correct.

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Bjorn Helgaas]

On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> When a system is using native PCIe hotplug for Thunderbolt it will be
> only present in the system when there is a device connected. This pretty
> much follows the BIOS assisted hotplug behaviour.
> 
> Thunderbolt host router integrated PCIe switch has two additional PCIe
> downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> (USB 3 host controller) respectively. These downstream bridges are not
> marked being hotplug capable. Reason for that is to preserve resources.
> Otherwise the OS would distribute remaining resources between all
> downstream bridges making these two bridges consume precious resources
> of the actual hotplug bridges.
> 
> Now, because these two bridges are not marked being hotplug capable the OS
> will not enable hotplug interrupt for them either and will not receive
> interrupt when devices behind them are hot-added. Solution to this is
> that the BIOS sends ACPI Notify() to the root port let the OS know it
> needs to rescan for added and/or removed devices.
> 
> Here is how the mechanism is supposed to work when a Thunderbolt
> endpoint is connected to one of the ports. In case of a standard USB-C
> device only the xHCI is hot-added otherwise steps are the same.
> 
> 1. Initially there is only the PCIe root port that is controlled by
>    the pciehp driver
> 
>   00:1b.0 (Hotplug+) --
> 
> 2. Then we get native PCIe hotplug interrupt and once it is handled the
>    topology looks as following
> 
>   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
>                                   +- 02:01.0 (HotPlug+)
>                                   \- 02:02.0 --

Help me out here.  In PCIe terms, I assume we basically hot-added this
switch:

  01:00.0 Switch Upstream port
  02:00.0 Switch Downstream Port
  02:01.0 Switch Downstream Port
  02:02.0 Switch Downstream Port

Only 02:01.0 has PCI_EXP_SLTCAP_HPC set.  We can assign secondary bus
number space to all the downstream ports, but there are currently no
devices below any of them.  Well, duh, that's exactly what you said
below:

> 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
>    they don't have anything behind them currently. Bridge 02:01.0 is
>    hotplug capable and used for extending the topology. At this point
>    the required PCIe devices are enabled and ACPI Notify() is sent to
>    the root port. The resulting topology is expected to look like
> 
>   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
>                                   +- 02:01.0 (HotPlug+)
>                                   \- 02:02.0 -- xHCI host controller
> 

I guess this means we should ultimately end up with these new devices:

  03:00.0 Thunderbolt host controller
  39:00.0 xHCI host controller

(Can you send "lspci -vv" output so I can see the names, device types,
etc?  I'm still trying to map the Thunderbolt "host router", NHI, etc
terminology into PCIe concepts.)

> However, the current ACPI hotplug implementation scans the whole 00:1b.0
> hotplug slot and everything behind it regardless whether native PCIe is
> used or not, and it expects that the BIOS has configured bridge
> resources upfront. If that's not the case it assigns resources using
> minimal allocation (everything currently found just barely fit)
> preventing future extension. 

I assume we got a Bus Check notification to the root port.  The spec
says OSPM should re-enumerate starting from the root port (I'm looking
at ACPI 6.2, sec 5.6.6).  It would sure be nice if the spec somehow
indicated that this re-enumeration should skip parts of the tree.

I'm not really sure how we were supposed to infer this coordination
requirement from the existing specs.  It does suggest that the Notify
should be sent as close as possible to the point where it's required,
which would be 02:00.0 and 02:02.0 here, but since that whole switch
was hot-added by pciehp, the firmware doesn't necessarily know
anything about it.

> In addition to that, if there is another
> native PCIe hotplug going on we may find the new PCIe switch only
> partially ready (all links are not fully trained yet) confusing pciehp
> when it finally starts to enumerate for new devices.
> 
> To make this work better with the native PCIe (pciehp) and standard PCI
> (shpchp) hotplug drivers, we let them handle all slot management and
> resource allocation for hotplug bridges and restrict ACPI hotplug to
> non-hotplug bridges.

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 75 +++++++++++++++++++++++-------
>  1 file changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 318b6a6f6341..e2bcd9fc3fd2 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -287,11 +287,12 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
>  	/*
>  	 * Expose slots to user space for functions that have _EJ0 or _RMV or
>  	 * are located in dock stations.  Do not expose them for devices handled
> -	 * by the native PCIe hotplug (PCIeHP), becuase that code is supposed to
> -	 * expose slots to user space in those cases.
> +	 * by the native PCIe hotplug (PCIeHP) or standard PCI hotplug
> +	 * (SHPCHP), because that code is supposed to expose slots to user
> +	 * space in those cases.
>  	 */
>  	if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
> -	    && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
> +	    && !(pdev && hotplug_is_native(pdev))) {
>  		unsigned long long sun;
>  		int retval;
>  
> @@ -430,6 +431,29 @@ static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
>  	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
>  }
>  
> +static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
> +{
> +	struct pci_bus *bus = bridge->subordinate;
> +	struct pci_dev *dev;
> +	int max;
> +
> +	if (!bus)
> +		return;
> +
> +	max = bus->busn_res.start;
> +	/* Scan already configured non-hotplug bridges */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)
> +			max = pci_scan_bridge(bus, dev, max, 0);
> +	}
> +
> +	/* Scan non-hotplug bridges that need to be reconfigured */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)
> +			max = pci_scan_bridge(bus, dev, max, 1);
> +	}
> +}
> +
>  /**
>   * enable_slot - enable, configure a slot
>   * @slot: slot to be enabled
> @@ -442,25 +466,42 @@ static void enable_slot(struct acpiphp_slot *slot)
>  	struct pci_dev *dev;
>  	struct pci_bus *bus = slot->bus;
>  	struct acpiphp_func *func;
> -	int max, pass;
> -	LIST_HEAD(add_list);
>  
> -	acpiphp_rescan_slot(slot);
> -	max = acpiphp_max_busnr(bus);
> -	for (pass = 0; pass < 2; pass++) {
> +	if (bus->self && hotplug_is_native(bus->self)) {
> +		/*
> +		 * If native hotplug is used, it will take care of hotplug
> +		 * slot management and resource allocation for hotplug
> +		 * bridges. However, ACPI hotplug may still be used for
> +		 * non-hotplug bridges to bring in additional devices such
> +		 * as Thunderbolt host controller.
> +		 */
>  		for_each_pci_bridge(dev, bus) {
> -			if (PCI_SLOT(dev->devfn) != slot->device)
> -				continue;
> -
> -			max = pci_scan_bridge(bus, dev, max, pass);
> -			if (pass && dev->subordinate) {
> -				check_hotplug_bridge(slot, dev);
> -				pcibios_resource_survey_bus(dev->subordinate);
> -				__pci_bus_size_bridges(dev->subordinate, &add_list);
> +			if (PCI_SLOT(dev->devfn) == slot->device)
> +				acpiphp_native_scan_bridge(dev);
> +		}
> +		pci_assign_unassigned_bridge_resources(bus->self);
> +	} else {
> +		LIST_HEAD(add_list);
> +		int max, pass;
> +
> +		acpiphp_rescan_slot(slot);
> +		max = acpiphp_max_busnr(bus);
> +		for (pass = 0; pass < 2; pass++) {
> +			for_each_pci_bridge(dev, bus) {
> +				if (PCI_SLOT(dev->devfn) != slot->device)
> +					continue;
> +
> +				max = pci_scan_bridge(bus, dev, max, pass);
> +				if (pass && dev->subordinate) {
> +					check_hotplug_bridge(slot, dev);
> +					pcibios_resource_survey_bus(dev->subordinate);
> +					__pci_bus_size_bridges(dev->subordinate,
> +							       &add_list);
> +				}
>  			}
>  		}
> +		__pci_bus_assign_resources(bus, &add_list, NULL);
>  	}
> -	__pci_bus_assign_resources(bus, &add_list, NULL);
>  
>  	acpiphp_sanitize_bus(bus);
>  	pcie_bus_configure_settings(bus);
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Mika Westerberg]

On Fri, Jun 01, 2018 at 09:11:18AM -0500, Bjorn Helgaas wrote:
> On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> > When a system is using native PCIe hotplug for Thunderbolt it will be
> > only present in the system when there is a device connected. This pretty
> > much follows the BIOS assisted hotplug behaviour.
> > 
> > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > (USB 3 host controller) respectively. These downstream bridges are not
> > marked being hotplug capable. Reason for that is to preserve resources.
> > Otherwise the OS would distribute remaining resources between all
> > downstream bridges making these two bridges consume precious resources
> > of the actual hotplug bridges.
> > 
> > Now, because these two bridges are not marked being hotplug capable the OS
> > will not enable hotplug interrupt for them either and will not receive
> > interrupt when devices behind them are hot-added. Solution to this is
> > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > needs to rescan for added and/or removed devices.
> > 
> > Here is how the mechanism is supposed to work when a Thunderbolt
> > endpoint is connected to one of the ports. In case of a standard USB-C
> > device only the xHCI is hot-added otherwise steps are the same.
> > 
> > 1. Initially there is only the PCIe root port that is controlled by
> >    the pciehp driver
> > 
> >   00:1b.0 (Hotplug+) --
> > 
> > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> >    topology looks as following
> > 
> >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> >                                   +- 02:01.0 (HotPlug+)
> >                                   \- 02:02.0 --
> 
> Help me out here.  In PCIe terms, I assume we basically hot-added this
> switch:
> 
>   01:00.0 Switch Upstream port
>   02:00.0 Switch Downstream Port
>   02:01.0 Switch Downstream Port
>   02:02.0 Switch Downstream Port
> 
> Only 02:01.0 has PCI_EXP_SLTCAP_HPC set.  We can assign secondary bus
> number space to all the downstream ports, but there are currently no
> devices below any of them.  Well, duh, that's exactly what you said
> below:
> 
> > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> >    they don't have anything behind them currently. Bridge 02:01.0 is
> >    hotplug capable and used for extending the topology. At this point
> >    the required PCIe devices are enabled and ACPI Notify() is sent to
> >    the root port. The resulting topology is expected to look like
> > 
> >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> >                                   +- 02:01.0 (HotPlug+)
> >                                   \- 02:02.0 -- xHCI host controller
> > 
> 
> I guess this means we should ultimately end up with these new devices:
> 
>   03:00.0 Thunderbolt host controller
>   39:00.0 xHCI host controller

That's right.

> (Can you send "lspci -vv" output so I can see the names, device types,
> etc?  I'm still trying to map the Thunderbolt "host router", NHI, etc
> terminology into PCIe concepts.)

The full lspci -vv is here:

  https://bugzilla.kernel.org/attachment.cgi?id=275703

Just to clarify:

  Thunderbolt host router = The whole Thunderbolt add-in-card, including
  			    PCIe switch, Thunderbolt host controller
			    (NHI) and USB 3.0 host controller (xHCI).

> > However, the current ACPI hotplug implementation scans the whole 00:1b.0
> > hotplug slot and everything behind it regardless whether native PCIe is
> > used or not, and it expects that the BIOS has configured bridge
> > resources upfront. If that's not the case it assigns resources using
> > minimal allocation (everything currently found just barely fit)
> > preventing future extension. 
> 
> I assume we got a Bus Check notification to the root port.  The spec
> says OSPM should re-enumerate starting from the root port (I'm looking
> at ACPI 6.2, sec 5.6.6).  It would sure be nice if the spec somehow
> indicated that this re-enumeration should skip parts of the tree.
> 
> I'm not really sure how we were supposed to infer this coordination
> requirement from the existing specs.  It does suggest that the Notify
> should be sent as close as possible to the point where it's required,
> which would be 02:00.0 and 02:02.0 here, but since that whole switch
> was hot-added by pciehp, the firmware doesn't necessarily know
> anything about it.

Right.

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Bjorn Helgaas]

On Fri, Jun 01, 2018 at 05:24:04PM +0300, Mika Westerberg wrote:
> On Fri, Jun 01, 2018 at 09:11:18AM -0500, Bjorn Helgaas wrote:
> > On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> > > When a system is using native PCIe hotplug for Thunderbolt it will be
> > > only present in the system when there is a device connected. This pretty
> > > much follows the BIOS assisted hotplug behaviour.
> > > 
> > > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > > (USB 3 host controller) respectively. These downstream bridges are not
> > > marked being hotplug capable. Reason for that is to preserve resources.
> > > Otherwise the OS would distribute remaining resources between all
> > > downstream bridges making these two bridges consume precious resources
> > > of the actual hotplug bridges.
> > > 
> > > Now, because these two bridges are not marked being hotplug capable the OS
> > > will not enable hotplug interrupt for them either and will not receive
> > > interrupt when devices behind them are hot-added. Solution to this is
> > > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > > needs to rescan for added and/or removed devices.
> > > 
> > > Here is how the mechanism is supposed to work when a Thunderbolt
> > > endpoint is connected to one of the ports. In case of a standard USB-C
> > > device only the xHCI is hot-added otherwise steps are the same.
> > > 
> > > 1. Initially there is only the PCIe root port that is controlled by
> > >    the pciehp driver
> > > 
> > >   00:1b.0 (Hotplug+) --
> > > 
> > > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> > >    topology looks as following
> > > 
> > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> > >                                   +- 02:01.0 (HotPlug+)
> > >                                   \- 02:02.0 --
> > 
> > Help me out here.  In PCIe terms, I assume we basically hot-added this
> > switch:
> > 
> >   01:00.0 Switch Upstream port
> >   02:00.0 Switch Downstream Port
> >   02:01.0 Switch Downstream Port
> >   02:02.0 Switch Downstream Port
> > 
> > Only 02:01.0 has PCI_EXP_SLTCAP_HPC set.  We can assign secondary bus
> > number space to all the downstream ports, but there are currently no
> > devices below any of them.  Well, duh, that's exactly what you said
> > below:
> > 
> > > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> > >    they don't have anything behind them currently. Bridge 02:01.0 is
> > >    hotplug capable and used for extending the topology. At this point
> > >    the required PCIe devices are enabled and ACPI Notify() is sent to
> > >    the root port. The resulting topology is expected to look like
> > > 
> > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> > >                                   +- 02:01.0 (HotPlug+)
> > >                                   \- 02:02.0 -- xHCI host controller
> > > 
> > 
> > I guess this means we should ultimately end up with these new devices:
> > 
> >   03:00.0 Thunderbolt host controller
> >   39:00.0 xHCI host controller
> 
> That's right.
> 
> > (Can you send "lspci -vv" output so I can see the names, device types,
> > etc?  I'm still trying to map the Thunderbolt "host router", NHI, etc
> > terminology into PCIe concepts.)
> 
> The full lspci -vv is here:
> 
>   https://bugzilla.kernel.org/attachment.cgi?id=275703

Thanks, that's quite an intimidating PCIe tree with several levels of
Thunderbolt stuff.

If you disconnect/reconnect the cable (or I guess the add-in card at
the top level) closest to the root port, does this all work correctly?
I assume the pciehp hotplug adds just the top-level switch (01:00.0),
then an ACPI Notify() adds the NHI and xHCI and configures the
tunnels, then another pciehp event adds the next-level switch, and
another Notify() sets up more tunnels, etc, etc?

> Just to clarify:
> 
>   Thunderbolt host router = The whole Thunderbolt add-in-card, including
>   			    PCIe switch, Thunderbolt host controller
> 			    (NHI) and USB 3.0 host controller (xHCI).

I assume the main reason for using ACPI hotplug here is because Linux
doesn't know how to set up the Thunderbolt tunnels, so some sort of
firmware has to do it?

How does the BIOS figure out when to send the Notify()?  If the host
router is built into the motherboard, I can see how there might be
some path for BIOS to notice a device being connected to the
Thunderbolt host router, and then it could power up the host router
(causing a pciehp hot-add), and then send the Notify().

But if this is actually a separate add-in card, does that mean the
tunnel setup has to be done via the option ROM somehow?

Or does the add-in card only work on systems that already have
Thunderbolt support in their BIOS?  If so, how does this work if the
card is hot-added?  Do we add the switch via pciehp, and something
else in Linux tells ACPI to issue the Notify()?

Bjorn

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Mika Westerberg]

On Fri, Jun 01, 2018 at 01:41:09PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 01, 2018 at 05:24:04PM +0300, Mika Westerberg wrote:
> > On Fri, Jun 01, 2018 at 09:11:18AM -0500, Bjorn Helgaas wrote:
> > > On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> > > > When a system is using native PCIe hotplug for Thunderbolt it will be
> > > > only present in the system when there is a device connected. This pretty
> > > > much follows the BIOS assisted hotplug behaviour.
> > > > 
> > > > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > > > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > > > (USB 3 host controller) respectively. These downstream bridges are not
> > > > marked being hotplug capable. Reason for that is to preserve resources.
> > > > Otherwise the OS would distribute remaining resources between all
> > > > downstream bridges making these two bridges consume precious resources
> > > > of the actual hotplug bridges.
> > > > 
> > > > Now, because these two bridges are not marked being hotplug capable the OS
> > > > will not enable hotplug interrupt for them either and will not receive
> > > > interrupt when devices behind them are hot-added. Solution to this is
> > > > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > > > needs to rescan for added and/or removed devices.
> > > > 
> > > > Here is how the mechanism is supposed to work when a Thunderbolt
> > > > endpoint is connected to one of the ports. In case of a standard USB-C
> > > > device only the xHCI is hot-added otherwise steps are the same.
> > > > 
> > > > 1. Initially there is only the PCIe root port that is controlled by
> > > >    the pciehp driver
> > > > 
> > > >   00:1b.0 (Hotplug+) --
> > > > 
> > > > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> > > >    topology looks as following
> > > > 
> > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> > > >                                   +- 02:01.0 (HotPlug+)
> > > >                                   \- 02:02.0 --
> > > 
> > > Help me out here.  In PCIe terms, I assume we basically hot-added this
> > > switch:
> > > 
> > >   01:00.0 Switch Upstream port
> > >   02:00.0 Switch Downstream Port
> > >   02:01.0 Switch Downstream Port
> > >   02:02.0 Switch Downstream Port
> > > 
> > > Only 02:01.0 has PCI_EXP_SLTCAP_HPC set.  We can assign secondary bus
> > > number space to all the downstream ports, but there are currently no
> > > devices below any of them.  Well, duh, that's exactly what you said
> > > below:
> > > 
> > > > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> > > >    they don't have anything behind them currently. Bridge 02:01.0 is
> > > >    hotplug capable and used for extending the topology. At this point
> > > >    the required PCIe devices are enabled and ACPI Notify() is sent to
> > > >    the root port. The resulting topology is expected to look like
> > > > 
> > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> > > >                                   +- 02:01.0 (HotPlug+)
> > > >                                   \- 02:02.0 -- xHCI host controller
> > > > 
> > > 
> > > I guess this means we should ultimately end up with these new devices:
> > > 
> > >   03:00.0 Thunderbolt host controller
> > >   39:00.0 xHCI host controller
> > 
> > That's right.
> > 
> > > (Can you send "lspci -vv" output so I can see the names, device types,
> > > etc?  I'm still trying to map the Thunderbolt "host router", NHI, etc
> > > terminology into PCIe concepts.)
> > 
> > The full lspci -vv is here:
> > 
> >   https://bugzilla.kernel.org/attachment.cgi?id=275703
> 
> Thanks, that's quite an intimidating PCIe tree with several levels of
> Thunderbolt stuff.
> 
> If you disconnect/reconnect the cable (or I guess the add-in card at
> the top level) closest to the root port, does this all work correctly?

Yes it does.

> I assume the pciehp hotplug adds just the top-level switch (01:00.0),
> then an ACPI Notify() adds the NHI and xHCI and configures the
> tunnels, then another pciehp event adds the next-level switch, and
> another Notify() sets up more tunnels, etc, etc?

It is the firmware running on the Thunderbolt host router that sets up
the tunnels and triggers standard PCIe hotplug once it is done. Notify()
is only used to bring in those two controllers to the first PCIe switch.
Reason for using Notify() here is that then we don't need to mark the
two downstream ports leading to xHCI and NHI to be hotplug ports and
thus the OS does not spread the available bus space/resources to those
ports.

If you keep connecting more devices then standard PCIe hotplug is used
and there will be no Notify().

> > Just to clarify:
> > 
> >   Thunderbolt host router = The whole Thunderbolt add-in-card, including
> >   			    PCIe switch, Thunderbolt host controller
> > 			    (NHI) and USB 3.0 host controller (xHCI).
> 
> I assume the main reason for using ACPI hotplug here is because Linux
> doesn't know how to set up the Thunderbolt tunnels, so some sort of
> firmware has to do it?

Firmware does it regardless of what OS is running (with the exception of
Apple hardware, of course). Once it establishes a tunnel a standard PCIe
hotplug event is triggered.

ACPI hotplug is only used to bring in those two devices of the host
router. You might wonder why they aren't all handled by the pciehp and
the reason is that if you only connect USB-C device (not TBT) ACPI
hotplug only finds xHCI since you don't need the NHI for that.

> How does the BIOS figure out when to send the Notify()?  If the host
> router is built into the motherboard, I can see how there might be
> some path for BIOS to notice a device being connected to the
> Thunderbolt host router, and then it could power up the host router
> (causing a pciehp hot-add), and then send the Notify().

There is a GPIO on the AIC that is wired to trigger ACPI GPE and the GPE
handler does the Notify().

> But if this is actually a separate add-in card, does that mean the
> tunnel setup has to be done via the option ROM somehow?

It is done in the firmware running on the host router (AIC).

> Or does the add-in card only work on systems that already have
> Thunderbolt support in their BIOS?  If so, how does this work if the
> card is hot-added?  Do we add the switch via pciehp, and something
> else in Linux tells ACPI to issue the Notify()?

The BIOS needs to have Thunderbolt support built in but I think that is
pretty "generic" and that is one of the reasons the Notify() is send to
the root port and not to the exact downstream ports where those two
controllers (xHCI, NHI) are connected to. I don't know all the details
but I think it works like that.

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Bjorn Helgaas]

On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> When a system is using native PCIe hotplug for Thunderbolt it will be
> only present in the system when there is a device connected. This pretty
> much follows the BIOS assisted hotplug behaviour.
> ...

> +static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
> +{
> +	struct pci_bus *bus = bridge->subordinate;
> +	struct pci_dev *dev;
> +	int max;
> +
> +	if (!bus)
> +		return;
> +
> +	max = bus->busn_res.start;
> +	/* Scan already configured non-hotplug bridges */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)

Here we test "dev->is_hotplug_bridge" and below we use
"hotplug_is_native(bus->self)".  Is the difference significant, or
should we use hotplug_is_native() here as well?

If we do need to use is_hotplug_bridge here, maybe a comment about the
difference would be helpful.

> +			max = pci_scan_bridge(bus, dev, max, 0);
> +	}
> +
> +	/* Scan non-hotplug bridges that need to be reconfigured */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)
> +			max = pci_scan_bridge(bus, dev, max, 1);
> +	}
> +}
> +
>  /**
>   * enable_slot - enable, configure a slot
>   * @slot: slot to be enabled
> @@ -442,25 +466,42 @@ static void enable_slot(struct acpiphp_slot *slot)
>  	struct pci_dev *dev;
>  	struct pci_bus *bus = slot->bus;
>  	struct acpiphp_func *func;
> -	int max, pass;
> -	LIST_HEAD(add_list);
>  
> -	acpiphp_rescan_slot(slot);
> -	max = acpiphp_max_busnr(bus);
> -	for (pass = 0; pass < 2; pass++) {
> +	if (bus->self && hotplug_is_native(bus->self)) {
> +		/*
> +		 * If native hotplug is used, it will take care of hotplug
> +		 * slot management and resource allocation for hotplug
> +		 * bridges. However, ACPI hotplug may still be used for
> +		 * non-hotplug bridges to bring in additional devices such
> +		 * as Thunderbolt host controller.
> +		 */
>  		for_each_pci_bridge(dev, bus) {
> -			if (PCI_SLOT(dev->devfn) != slot->device)
> -				continue;
> -
> -			max = pci_scan_bridge(bus, dev, max, pass);
> -			if (pass && dev->subordinate) {
> -				check_hotplug_bridge(slot, dev);
> -				pcibios_resource_survey_bus(dev->subordinate);
> -				__pci_bus_size_bridges(dev->subordinate, &add_list);
> +			if (PCI_SLOT(dev->devfn) == slot->device)
> +				acpiphp_native_scan_bridge(dev);
> +		}
> +		pci_assign_unassigned_bridge_resources(bus->self);
> +	} else {

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Mika Westerberg]

On Fri, Jun 01, 2018 at 04:35:05PM -0500, Bjorn Helgaas wrote:
> On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> > When a system is using native PCIe hotplug for Thunderbolt it will be
> > only present in the system when there is a device connected. This pretty
> > much follows the BIOS assisted hotplug behaviour.
> > ...
> 
> > +static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
> > +{
> > +	struct pci_bus *bus = bridge->subordinate;
> > +	struct pci_dev *dev;
> > +	int max;
> > +
> > +	if (!bus)
> > +		return;
> > +
> > +	max = bus->busn_res.start;
> > +	/* Scan already configured non-hotplug bridges */
> > +	for_each_pci_bridge(dev, bus) {
> > +		if (!dev->is_hotplug_bridge)
> 
> Here we test "dev->is_hotplug_bridge" and below we use
> "hotplug_is_native(bus->self)".  Is the difference significant, or
> should we use hotplug_is_native() here as well?
> 
> If we do need to use is_hotplug_bridge here, maybe a comment about the
> difference would be helpful.

I think here hotplug_is_native() would work as well. The idea is that we need to
avoid scanning bridges that are handled by pciehp (or shpchp).

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Bjorn Helgaas]

On Fri, Jun 01, 2018 at 10:19:05PM +0300, Mika Westerberg wrote:
> On Fri, Jun 01, 2018 at 01:41:09PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 01, 2018 at 05:24:04PM +0300, Mika Westerberg wrote:
> > > On Fri, Jun 01, 2018 at 09:11:18AM -0500, Bjorn Helgaas wrote:
> > > > On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> > > > > When a system is using native PCIe hotplug for Thunderbolt it will be
> > > > > only present in the system when there is a device connected. This pretty
> > > > > much follows the BIOS assisted hotplug behaviour.
> > > > > 
> > > > > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > > > > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > > > > (USB 3 host controller) respectively. These downstream bridges are not
> > > > > marked being hotplug capable. Reason for that is to preserve resources.
> > > > > Otherwise the OS would distribute remaining resources between all
> > > > > downstream bridges making these two bridges consume precious resources
> > > > > of the actual hotplug bridges.
> > > > > 
> > > > > Now, because these two bridges are not marked being hotplug capable the OS
> > > > > will not enable hotplug interrupt for them either and will not receive
> > > > > interrupt when devices behind them are hot-added. Solution to this is
> > > > > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > > > > needs to rescan for added and/or removed devices.
> > > > > 
> > > > > Here is how the mechanism is supposed to work when a Thunderbolt
> > > > > endpoint is connected to one of the ports. In case of a standard USB-C
> > > > > device only the xHCI is hot-added otherwise steps are the same.
> > > > > 
> > > > > 1. Initially there is only the PCIe root port that is controlled by
> > > > >    the pciehp driver
> > > > > 
> > > > >   00:1b.0 (Hotplug+) --
> > > > > 
> > > > > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> > > > >    topology looks as following
> > > > > 
> > > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> > > > >                                   +- 02:01.0 (HotPlug+)
> > > > >                                   \- 02:02.0 --
> > > > 
> > > > Help me out here.  In PCIe terms, I assume we basically hot-added this
> > > > switch:
> > > > 
> > > >   01:00.0 Switch Upstream port
> > > >   02:00.0 Switch Downstream Port
> > > >   02:01.0 Switch Downstream Port
> > > >   02:02.0 Switch Downstream Port
> > > > 
> > > > Only 02:01.0 has PCI_EXP_SLTCAP_HPC set.  We can assign secondary bus
> > > > number space to all the downstream ports, but there are currently no
> > > > devices below any of them.  Well, duh, that's exactly what you said
> > > > below:
> > > > 
> > > > > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> > > > >    they don't have anything behind them currently. Bridge 02:01.0 is
> > > > >    hotplug capable and used for extending the topology. At this point
> > > > >    the required PCIe devices are enabled and ACPI Notify() is sent to
> > > > >    the root port. The resulting topology is expected to look like
> > > > > 
> > > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> > > > >                                   +- 02:01.0 (HotPlug+)
> > > > >                                   \- 02:02.0 -- xHCI host controller
> > > > > 
> > > > 
> > > > I guess this means we should ultimately end up with these new devices:
> > > > 
> > > >   03:00.0 Thunderbolt host controller
> > > >   39:00.0 xHCI host controller
> > > 
> > > That's right.

And after the host router firmware sets up the tunnels, is there a
step 4 where we get another pciehp event from 02:01.0 and we enumerate
the Thunderbolt switch (which I assume looks like a regular PCIe
switch to the PCI core)?  Does the add-in card actually contain all
the following devices (from the lspci you pointed me to)?

  01:00.0 Switch Upstream Port   to [bus 02-39]
  02:00.0 Switch Downstream Port to [bus 03]     (to NHI)
  02:01.0 Switch Downstream Port to [bus 04-38]  (to Thunderbolt switch)
  02:02.0 Switch Downstream Port to [bus 39]     (to xHCI)

  03:00.0 Thunderbolt Host Controller (NHI) Endpoint
  39:00.0 xHCI Endpoint

  04:00.0 Switch Upstream Port   to [bus 05-38]   \
  05:01.0 Switch Downstream Port to [bus 06-09]   |  Thunderbolt Switch
  05:04.0 Switch Downstream Port to [bus 0a-38]   /

That would correspond to Figure 1-1 here: https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html
except that the figure doesn't show the xHCI controller.

> > > > (Can you send "lspci -vv" output so I can see the names, device types,
> > > > etc?  I'm still trying to map the Thunderbolt "host router", NHI, etc
> > > > terminology into PCIe concepts.)
> > > 
> > > The full lspci -vv is here:
> > > 
> > >   https://bugzilla.kernel.org/attachment.cgi?id=275703
> > 
> > Thanks, that's quite an intimidating PCIe tree with several levels of
> > Thunderbolt stuff.
> > 
> > If you disconnect/reconnect the cable (or I guess the add-in card at
> > the top level) closest to the root port, does this all work correctly?
> 
> Yes it does.

I'm honestly amazed :)

> > I assume the pciehp hotplug adds just the top-level switch (01:00.0),
> > then an ACPI Notify() adds the NHI and xHCI and configures the
> > tunnels, then another pciehp event adds the next-level switch, and
> > another Notify() sets up more tunnels, etc, etc?
> 
> It is the firmware running on the Thunderbolt host router that sets up
> the tunnels and triggers standard PCIe hotplug once it is done. Notify()
> is only used to bring in those two controllers to the first PCIe switch.
> Reason for using Notify() here is that then we don't need to mark the
> two downstream ports leading to xHCI and NHI to be hotplug ports and
> thus the OS does not spread the available bus space/resources to those
> ports.
> 
> If you keep connecting more devices then standard PCIe hotplug is used
> and there will be no Notify().
> 
> > > Just to clarify:
> > > 
> > >   Thunderbolt host router = The whole Thunderbolt add-in-card, including
> > >   			    PCIe switch, Thunderbolt host controller
> > > 			    (NHI) and USB 3.0 host controller (xHCI).
> > 
> > I assume the main reason for using ACPI hotplug here is because Linux
> > doesn't know how to set up the Thunderbolt tunnels, so some sort of
> > firmware has to do it?
> 
> Firmware does it regardless of what OS is running (with the exception of
> Apple hardware, of course). Once it establishes a tunnel a standard PCIe
> hotplug event is triggered.
> 
> ACPI hotplug is only used to bring in those two devices of the host
> router. You might wonder why they aren't all handled by the pciehp and
> the reason is that if you only connect USB-C device (not TBT) ACPI
> hotplug only finds xHCI since you don't need the NHI for that.
>
> > How does the BIOS figure out when to send the Notify()?  If the host
> > router is built into the motherboard, I can see how there might be
> > some path for BIOS to notice a device being connected to the
> > Thunderbolt host router, and then it could power up the host router
> > (causing a pciehp hot-add), and then send the Notify().
> 
> There is a GPIO on the AIC that is wired to trigger ACPI GPE and the GPE
> handler does the Notify().

So this GPIO must be part of the reason for the mysterious Thunderbolt
header, i.e., https://superuser.com/questions/1024865/for-what-is-the-thunderbolt-aic-connector-used

You mentioned two reasons for using the ACPI Notify():

  1) To avoid having the OS assign more resources than necessary to the
     bridges leading to NHI and xHCI.

  2) To avoid adding NHI at all if we only need USB-C.

They both seem sort of minor.  If those devices were brought in via
the original pciehp hot-add, we would only allocate the resources they
need since they're both endpoints and we'd know exactly what they
needed.  An unused NHI would only consume 260K of MMIO space and one
bus number (which I think we will always assign anyway because of
"PCI: Take all bridges into account when calculating bus numbers for
extension").

The requirement for the GPIO header and a separate cable to it is a
huge hassle so it seems like there must be more to it than just those
two things.  But that's not really germane to this patch anyway
because we have to support the hardware/firmware as it is, not as we
might imagine things could be.

> > But if this is actually a separate add-in card, does that mean the
> > tunnel setup has to be done via the option ROM somehow?
> 
> It is done in the firmware running on the host router (AIC).
> 
> > Or does the add-in card only work on systems that already have
> > Thunderbolt support in their BIOS?  If so, how does this work if the
> > card is hot-added?  Do we add the switch via pciehp, and something
> > else in Linux tells ACPI to issue the Notify()?
> 
> The BIOS needs to have Thunderbolt support built in but I think that is
> pretty "generic" and that is one of the reasons the Notify() is send to
> the root port and not to the exact downstream ports where those two
> controllers (xHCI, NHI) are connected to. I don't know all the details
> but I think it works like that.

Thanks for all this background.  It really helps me put things
together.

Bjorn

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Mika Westerberg]

On Fri, Jun 01, 2018 at 04:50:27PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 01, 2018 at 10:19:05PM +0300, Mika Westerberg wrote:
> > On Fri, Jun 01, 2018 at 01:41:09PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Jun 01, 2018 at 05:24:04PM +0300, Mika Westerberg wrote:
> > > > On Fri, Jun 01, 2018 at 09:11:18AM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> > > > > > When a system is using native PCIe hotplug for Thunderbolt it will be
> > > > > > only present in the system when there is a device connected. This pretty
> > > > > > much follows the BIOS assisted hotplug behaviour.
> > > > > > 
> > > > > > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > > > > > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > > > > > (USB 3 host controller) respectively. These downstream bridges are not
> > > > > > marked being hotplug capable. Reason for that is to preserve resources.
> > > > > > Otherwise the OS would distribute remaining resources between all
> > > > > > downstream bridges making these two bridges consume precious resources
> > > > > > of the actual hotplug bridges.
> > > > > > 
> > > > > > Now, because these two bridges are not marked being hotplug capable the OS
> > > > > > will not enable hotplug interrupt for them either and will not receive
> > > > > > interrupt when devices behind them are hot-added. Solution to this is
> > > > > > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > > > > > needs to rescan for added and/or removed devices.
> > > > > > 
> > > > > > Here is how the mechanism is supposed to work when a Thunderbolt
> > > > > > endpoint is connected to one of the ports. In case of a standard USB-C
> > > > > > device only the xHCI is hot-added otherwise steps are the same.
> > > > > > 
> > > > > > 1. Initially there is only the PCIe root port that is controlled by
> > > > > >    the pciehp driver
> > > > > > 
> > > > > >   00:1b.0 (Hotplug+) --
> > > > > > 
> > > > > > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> > > > > >    topology looks as following
> > > > > > 
> > > > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> > > > > >                                   +- 02:01.0 (HotPlug+)
> > > > > >                                   \- 02:02.0 --
> > > > > 
> > > > > Help me out here.  In PCIe terms, I assume we basically hot-added this
> > > > > switch:
> > > > > 
> > > > >   01:00.0 Switch Upstream port
> > > > >   02:00.0 Switch Downstream Port
> > > > >   02:01.0 Switch Downstream Port
> > > > >   02:02.0 Switch Downstream Port
> > > > > 
> > > > > Only 02:01.0 has PCI_EXP_SLTCAP_HPC set.  We can assign secondary bus
> > > > > number space to all the downstream ports, but there are currently no
> > > > > devices below any of them.  Well, duh, that's exactly what you said
> > > > > below:
> > > > > 
> > > > > > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> > > > > >    they don't have anything behind them currently. Bridge 02:01.0 is
> > > > > >    hotplug capable and used for extending the topology. At this point
> > > > > >    the required PCIe devices are enabled and ACPI Notify() is sent to
> > > > > >    the root port. The resulting topology is expected to look like
> > > > > > 
> > > > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> > > > > >                                   +- 02:01.0 (HotPlug+)
> > > > > >                                   \- 02:02.0 -- xHCI host controller
> > > > > > 
> > > > > 
> > > > > I guess this means we should ultimately end up with these new devices:
> > > > > 
> > > > >   03:00.0 Thunderbolt host controller
> > > > >   39:00.0 xHCI host controller
> > > > 
> > > > That's right.
> 
> And after the host router firmware sets up the tunnels, is there a
> step 4 where we get another pciehp event from 02:01.0 and we enumerate
> the Thunderbolt switch (which I assume looks like a regular PCIe
> switch to the PCI core)?

Yes there is.

> Does the add-in card actually contain all
> the following devices (from the lspci you pointed me to)?
> 
>   01:00.0 Switch Upstream Port   to [bus 02-39]
>   02:00.0 Switch Downstream Port to [bus 03]     (to NHI)
>   02:01.0 Switch Downstream Port to [bus 04-38]  (to Thunderbolt switch)
>   02:02.0 Switch Downstream Port to [bus 39]     (to xHCI)
> 
>   03:00.0 Thunderbolt Host Controller (NHI) Endpoint
>   39:00.0 xHCI Endpoint

This belongs to the Thunderbolt host router (AIC, add-in-card).

>   04:00.0 Switch Upstream Port   to [bus 05-38]   \
>   05:01.0 Switch Downstream Port to [bus 06-09]   |  Thunderbolt Switch
>   05:04.0 Switch Downstream Port to [bus 0a-38]   /
> 
> That would correspond to Figure 1-1 here: https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html
> except that the figure doesn't show the xHCI controller.

This one shows another Thunderbolt device connected to the host router
via Thunderbolt cable (it also includes PCIe switch and PCIe endpoint
and one PCIe downstream port where the topology is extended, 05:04). In
case of Titan Ridge (the next Thunderbolt host router) there will be
also xHCI.

> > > > > (Can you send "lspci -vv" output so I can see the names, device types,
> > > > > etc?  I'm still trying to map the Thunderbolt "host router", NHI, etc
> > > > > terminology into PCIe concepts.)
> > > > 
> > > > The full lspci -vv is here:
> > > > 
> > > >   https://bugzilla.kernel.org/attachment.cgi?id=275703
> > > 
> > > Thanks, that's quite an intimidating PCIe tree with several levels of
> > > Thunderbolt stuff.
> > > 
> > > If you disconnect/reconnect the cable (or I guess the add-in card at
> > > the top level) closest to the root port, does this all work correctly?
> > 
> > Yes it does.
> 
> I'm honestly amazed :)

I've spent good part of January sitting in the lab testing this with
various systems and it was the best solution so far :)

> > > I assume the pciehp hotplug adds just the top-level switch (01:00.0),
> > > then an ACPI Notify() adds the NHI and xHCI and configures the
> > > tunnels, then another pciehp event adds the next-level switch, and
> > > another Notify() sets up more tunnels, etc, etc?
> > 
> > It is the firmware running on the Thunderbolt host router that sets up
> > the tunnels and triggers standard PCIe hotplug once it is done. Notify()
> > is only used to bring in those two controllers to the first PCIe switch.
> > Reason for using Notify() here is that then we don't need to mark the
> > two downstream ports leading to xHCI and NHI to be hotplug ports and
> > thus the OS does not spread the available bus space/resources to those
> > ports.
> > 
> > If you keep connecting more devices then standard PCIe hotplug is used
> > and there will be no Notify().
> > 
> > > > Just to clarify:
> > > > 
> > > >   Thunderbolt host router = The whole Thunderbolt add-in-card, including
> > > >   			    PCIe switch, Thunderbolt host controller
> > > > 			    (NHI) and USB 3.0 host controller (xHCI).
> > > 
> > > I assume the main reason for using ACPI hotplug here is because Linux
> > > doesn't know how to set up the Thunderbolt tunnels, so some sort of
> > > firmware has to do it?
> > 
> > Firmware does it regardless of what OS is running (with the exception of
> > Apple hardware, of course). Once it establishes a tunnel a standard PCIe
> > hotplug event is triggered.
> > 
> > ACPI hotplug is only used to bring in those two devices of the host
> > router. You might wonder why they aren't all handled by the pciehp and
> > the reason is that if you only connect USB-C device (not TBT) ACPI
> > hotplug only finds xHCI since you don't need the NHI for that.
> >
> > > How does the BIOS figure out when to send the Notify()?  If the host
> > > router is built into the motherboard, I can see how there might be
> > > some path for BIOS to notice a device being connected to the
> > > Thunderbolt host router, and then it could power up the host router
> > > (causing a pciehp hot-add), and then send the Notify().
> > 
> > There is a GPIO on the AIC that is wired to trigger ACPI GPE and the GPE
> > handler does the Notify().
> 
> So this GPIO must be part of the reason for the mysterious Thunderbolt
> header, i.e., https://superuser.com/questions/1024865/for-what-is-the-thunderbolt-aic-connector-used
> 
> You mentioned two reasons for using the ACPI Notify():
> 
>   1) To avoid having the OS assign more resources than necessary to the
>      bridges leading to NHI and xHCI.
> 
>   2) To avoid adding NHI at all if we only need USB-C.
> 
> They both seem sort of minor.  If those devices were brought in via
> the original pciehp hot-add, we would only allocate the resources they
> need since they're both endpoints and we'd know exactly what they
> needed.  An unused NHI would only consume 260K of MMIO space and one
> bus number (which I think we will always assign anyway because of
> "PCI: Take all bridges into account when calculating bus numbers for
> extension").
> 
> The requirement for the GPIO header and a separate cable to it is a
> huge hassle so it seems like there must be more to it than just those
> two things.  But that's not really germane to this patch anyway
> because we have to support the hardware/firmware as it is, not as we
> might imagine things could be.

Indeed. The matter of fact is that these systems are currently shipping
and we need to deal with them. In case of laptops such as Dell, the
Alpine Ridge chip (Thunderbolt host router) is soldered on the
motherboard, though. GPIO connection is still required.

> > > But if this is actually a separate add-in card, does that mean the
> > > tunnel setup has to be done via the option ROM somehow?
> > 
> > It is done in the firmware running on the host router (AIC).
> > 
> > > Or does the add-in card only work on systems that already have
> > > Thunderbolt support in their BIOS?  If so, how does this work if the
> > > card is hot-added?  Do we add the switch via pciehp, and something
> > > else in Linux tells ACPI to issue the Notify()?
> > 
> > The BIOS needs to have Thunderbolt support built in but I think that is
> > pretty "generic" and that is one of the reasons the Notify() is send to
> > the root port and not to the exact downstream ports where those two
> > controllers (xHCI, NHI) are connected to. I don't know all the details
> > but I think it works like that.
> 
> Thanks for all this background.  It really helps me put things
> together.

No problem :)

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Bjorn Helgaas]

On Sat, Jun 02, 2018 at 12:48:18AM +0300, Mika Westerberg wrote:
> On Fri, Jun 01, 2018 at 04:35:05PM -0500, Bjorn Helgaas wrote:
> > On Tue, May 29, 2018 at 07:01:55PM +0300, Mika Westerberg wrote:
> > > When a system is using native PCIe hotplug for Thunderbolt it will be
> > > only present in the system when there is a device connected. This pretty
> > > much follows the BIOS assisted hotplug behaviour.
> > > ...
> > 
> > > +static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
> > > +{
> > > +	struct pci_bus *bus = bridge->subordinate;
> > > +	struct pci_dev *dev;
> > > +	int max;
> > > +
> > > +	if (!bus)
> > > +		return;
> > > +
> > > +	max = bus->busn_res.start;
> > > +	/* Scan already configured non-hotplug bridges */
> > > +	for_each_pci_bridge(dev, bus) {
> > > +		if (!dev->is_hotplug_bridge)
> > 
> > Here we test "dev->is_hotplug_bridge" and below we use
> > "hotplug_is_native(bus->self)".  Is the difference significant, or
> > should we use hotplug_is_native() here as well?
> > 
> > If we do need to use is_hotplug_bridge here, maybe a comment about the
> > difference would be helpful.
> 
> I think here hotplug_is_native() would work as well. The idea is
> that we need to avoid scanning bridges that are handled by pciehp
> (or shpchp).

I made that change on my branch.  That way we won't have to worry
about the cases where we set is_hotplug_bridge based on some ACPI
methods.  I think those would be bugs because I think we *do* want
acpiphp to scan them, and if we tested "!dev->is_hotplug_bridge", we
would not scan them.

Here's what's on my branch:

commit eb4c82c49180cf46dd7f9ed15ea96d1da190fea6
Author: Mika Westerberg <mika.westerberg@linux.intel.com>
Date:   Tue May 29 19:01:55 2018 +0300

    ACPI / hotplug / PCI: Don't scan bridges managed by native hotplug
    
    When acpiphp re-enumerates a PCI hierarchy because of an ACPI Notify()
    event, we should skip bridges managed by native hotplug (pciehp or shpchp).
    We don't want to scan below a native hotplug bridge until the hotplug
    controller generates a hot-add event.
    
    A typical scenario is a Root Port leading to a Thunderbolt host router that
    remains powered off until something is connected to it.  See [1] for the
    lspci details.
    
      1. Before something is connected, only the Root Port exists.  It has
         PCI_EXP_SLTCAP_HPC set and pciehp is responsible for hotplug:
    
           00:1b.0 Root Port (HotPlug+)
    
      2. When a USB-C or Thunderbolt device is connected, the Switch in the
         Thunderbolt host router is powered up, the Root Port signals a hotplug
         add event and pciehp enumerates the Switch:
    
           01:00.0 Switch Upstream Port   to [bus 02-39]
           02:00.0 Switch Downstream Port to [bus 03]    (HotPlug-, to NHI)
           02:01.0 Switch Downstream Port to [bus 04-38] (HotPlug+, to Thunderbolt connector)
           02:02.0 Switch Downstream Port to [bus 39]    (HotPlug-, to xHCI)
    
         The 02:00.0 and 02:02.0 Ports lead to Endpoints that are not powered
         up yet.  The Ports have PCI_EXP_SLTCAP_HPC cleared, so pciehp doesn't
         handle hotplug for them and we assign minimal resources to them.
    
         The 02:01.0 Port has PCI_EXP_SLTCAP_HPC set, so pciehp handles native
         hotplug events for it.
    
      3. The BIOS powers up the xHCI controller.  If a Thunderbolt device was
         connected (not just a USB-C device), it also powers up the NHI.  Then
         it sends an ACPI Notify() to the Root Port, and acpiphp enumerates the
         new device(s):
    
           03:00.0 Thunderbolt Host Controller (NHI) Endpoint
           39:00.0 xHCI Endpoint
    
      4. If a Thunderbolt device was connected, the host router firmware uses
         the NHI to set up Thunderbolt tunnels and triggers a native hotplug
         event (via 02:01.0 in this example).  Then pciehp enumerates the new
         Thunderbolt devices:
    
           04:00.0 Switch Upstream Port   to [bus 05-38]
           05:01.0 Switch Downstream Port to [bus 06-09] (HotPlug-)
           05:04.0 Switch Downstream Port to [bus 0a-38] (HotPlug+)
    
         In this example, 05:01.0 leads to another Switch and some NICs.  This
         subtree is static, so 05:01.0 doesn't support hotplug and has
         PCI_EXP_SLTCAP_HPC cleared.
    
    In step 3, acpiphp previously enumerated everything below the Root Port,
    including things below the 02:01.0 Port.  We don't want that because pciehp
    expects to manage hotplug below that Port, and firmware on the host router
    may be in the middle of configuring its Link so it may not be ready yet.
    
    To make this work better with the native PCIe (pciehp) and standard PCI
    (shpchp) hotplug drivers, we let them handle all slot management and
    resource allocation for hotplug bridges and restrict ACPI hotplug to
    non-hotplug bridges.
    
    [1] https://bugzilla.kernel.org/show_bug.cgi?id=199581#c5
    Link: https://lkml.kernel.org/r/20180529160155.1738-1-mika.westerberg@linux.intel.com
    Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    [bhelgaas: changelog, use hotplug_is_native() instead of
    dev->is_hotplug_bridge]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b45b375c0e6c..bc9e19642567 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -287,11 +287,12 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
 	/*
 	 * Expose slots to user space for functions that have _EJ0 or _RMV or
 	 * are located in dock stations.  Do not expose them for devices handled
-	 * by the native PCIe hotplug (PCIeHP), becuase that code is supposed to
-	 * expose slots to user space in those cases.
+	 * by the native PCIe hotplug (PCIeHP) or standard PCI hotplug
+	 * (SHPCHP), because that code is supposed to expose slots to user
+	 * space in those cases.
 	 */
 	if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
-	    && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+	    && !(pdev && hotplug_is_native(pdev))) {
 		unsigned long long sun;
 		int retval;
 
@@ -430,6 +431,29 @@ static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
 	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
 }
 
+static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
+{
+	struct pci_bus *bus = bridge->subordinate;
+	struct pci_dev *dev;
+	int max;
+
+	if (!bus)
+		return;
+
+	max = bus->busn_res.start;
+	/* Scan already configured non-hotplug bridges */
+	for_each_pci_bridge(dev, bus) {
+		if (!hotplug_is_native(dev))
+			max = pci_scan_bridge(bus, dev, max, 0);
+	}
+
+	/* Scan non-hotplug bridges that need to be reconfigured */
+	for_each_pci_bridge(dev, bus) {
+		if (!hotplug_is_native(dev))
+			max = pci_scan_bridge(bus, dev, max, 1);
+	}
+}
+
 /**
  * enable_slot - enable, configure a slot
  * @slot: slot to be enabled
@@ -442,25 +466,42 @@ static void enable_slot(struct acpiphp_slot *slot)
 	struct pci_dev *dev;
 	struct pci_bus *bus = slot->bus;
 	struct acpiphp_func *func;
-	int max, pass;
-	LIST_HEAD(add_list);
 
-	acpiphp_rescan_slot(slot);
-	max = acpiphp_max_busnr(bus);
-	for (pass = 0; pass < 2; pass++) {
+	if (bus->self && hotplug_is_native(bus->self)) {
+		/*
+		 * If native hotplug is used, it will take care of hotplug
+		 * slot management and resource allocation for hotplug
+		 * bridges. However, ACPI hotplug may still be used for
+		 * non-hotplug bridges to bring in additional devices such
+		 * as a Thunderbolt host controller.
+		 */
 		for_each_pci_bridge(dev, bus) {
-			if (PCI_SLOT(dev->devfn) != slot->device)
-				continue;
-
-			max = pci_scan_bridge(bus, dev, max, pass);
-			if (pass && dev->subordinate) {
-				check_hotplug_bridge(slot, dev);
-				pcibios_resource_survey_bus(dev->subordinate);
-				__pci_bus_size_bridges(dev->subordinate, &add_list);
+			if (PCI_SLOT(dev->devfn) == slot->device)
+				acpiphp_native_scan_bridge(dev);
+		}
+		pci_assign_unassigned_bridge_resources(bus->self);
+	} else {
+		LIST_HEAD(add_list);
+		int max, pass;
+
+		acpiphp_rescan_slot(slot);
+		max = acpiphp_max_busnr(bus);
+		for (pass = 0; pass < 2; pass++) {
+			for_each_pci_bridge(dev, bus) {
+				if (PCI_SLOT(dev->devfn) != slot->device)
+					continue;
+
+				max = pci_scan_bridge(bus, dev, max, pass);
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
+					pcibios_resource_survey_bus(dev->subordinate);
+					__pci_bus_size_bridges(dev->subordinate,
+							       &add_list);
+				}
 			}
 		}
+		__pci_bus_assign_resources(bus, &add_list, NULL);
 	}
-	__pci_bus_assign_resources(bus, &add_list, NULL);
 
 	acpiphp_sanitize_bus(bus);
 	pcie_bus_configure_settings(bus);

Re: [PATCH v8 0/7] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Bjorn Helgaas]

On Mon, May 28, 2018 at 03:47:49PM +0300, Mika Westerberg wrote:
> When Thunderbolt host router is configured to be in native enumeration mode
> it is only present in the system if there is something connected to the
> ports. This pretty much follows how the BIOS assisted mode works.
> 
> In native enumeration mode the Thunderbolt host controller (NHI) and USB
> host controller (xHCI) are not hot-added using native PCIe hotplug but
> instead they will be hotplugged via BIOS triggered ACPI Notify() to the
> root port. This is done to preserve resources since NHI and xHCI only need
> 1 MB of MMIO space and no additional buses. Currently Linux does not
> support this very well and ends up failing the hotplug in one way or
> another. More detailed explanation is in changelog of patch [4/7].
> 
> This series fixes this issue and in addition includes fixes for few other
> issues found during testing on a system that has Thunderbolt controller in
> native PCIe enumeration mode. However, the fixes here are not in any way
> Thunderbolt specific and should be applicable to other systems as well.

I applied all these on pci/hotplug for v4.18.

Thanks for your help and patience!  I reordered some things and
tweaked some changelogs, so let me know if I broke anything.  I'll put
this into -next as soon as the 0-day robot builds it.

Bjorn

Re: [PATCH v8 4/7] ACPI/hotplug/PCI: Do not scan all bridges when native PCIe hotplug is used

[Mika Westerberg]

On Sat, Jun 02, 2018 at 12:46:59AM -0500, Bjorn Helgaas wrote:
> > I think here hotplug_is_native() would work as well. The idea is
> > that we need to avoid scanning bridges that are handled by pciehp
> > (or shpchp).
> 
> I made that change on my branch.  That way we won't have to worry
> about the cases where we set is_hotplug_bridge based on some ACPI
> methods.  I think those would be bugs because I think we *do* want
> acpiphp to scan them, and if we tested "!dev->is_hotplug_bridge", we
> would not scan them.

I agree.

Re: [PATCH v8 0/7] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Mika Westerberg]

On Sat, Jun 02, 2018 at 12:50:50AM -0500, Bjorn Helgaas wrote:
> On Mon, May 28, 2018 at 03:47:49PM +0300, Mika Westerberg wrote:
> > When Thunderbolt host router is configured to be in native enumeration mode
> > it is only present in the system if there is something connected to the
> > ports. This pretty much follows how the BIOS assisted mode works.
> > 
> > In native enumeration mode the Thunderbolt host controller (NHI) and USB
> > host controller (xHCI) are not hot-added using native PCIe hotplug but
> > instead they will be hotplugged via BIOS triggered ACPI Notify() to the
> > root port. This is done to preserve resources since NHI and xHCI only need
> > 1 MB of MMIO space and no additional buses. Currently Linux does not
> > support this very well and ends up failing the hotplug in one way or
> > another. More detailed explanation is in changelog of patch [4/7].
> > 
> > This series fixes this issue and in addition includes fixes for few other
> > issues found during testing on a system that has Thunderbolt controller in
> > native PCIe enumeration mode. However, the fixes here are not in any way
> > Thunderbolt specific and should be applicable to other systems as well.
> 
> I applied all these on pci/hotplug for v4.18.

Thank you!

> Thanks for your help and patience!  I reordered some things and
> tweaked some changelogs, so let me know if I broke anything.  I'll put
> this into -next as soon as the 0-day robot builds it.

I just finished testing of your branch and everything still work as
expected :)