[PATCH v6 00/12] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[PATCH v6 00/12] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Mika Westerberg]

Hi,

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 [7/12].

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:

  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 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 (12):
  PCI: Take all bridges into account when calculating bus numbers for extension
  PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume
  PCI: Request control of native PCIe hotplug only if supported
  PCI: Make pciehp_is_native() stricter
  PCI: hotplug: Convert SHPC to be builtin only
  PCI: Request control of native SHPC hotplug similarly to pciehp
  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()
  PCI: Improve "partially hidden behind bridge" log message
  ACPI / hotplug / PCI: Drop unnecessary parentheses

 drivers/acpi/pci_root.c            | 10 +++-
 drivers/pci/hotplug/Kconfig        |  5 +-
 drivers/pci/hotplug/acpi_pcihp.c   | 34 +++++-------
 drivers/pci/hotplug/acpiphp_glue.c | 84 +++++++++++++++++++++++-------
 drivers/pci/hotplug/pciehp.h       |  2 +-
 drivers/pci/hotplug/pciehp_core.c  |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c   | 13 ++++-
 drivers/pci/hotplug/shpchp.h       | 11 ----
 drivers/pci/hotplug/shpchp_core.c  |  2 +-
 drivers/pci/pci-acpi.c             | 51 +++++++++++++-----
 drivers/pci/pcie/portdrv.h         |  2 -
 drivers/pci/pcie/portdrv_core.c    |  2 +-
 drivers/pci/probe.c                | 36 ++++++++-----
 drivers/pci/setup-bus.c            | 82 ++++++++++++++---------------
 include/linux/pci.h                |  5 +-
 include/linux/pci_hotplug.h        | 31 +++++++++--
 16 files changed, 237 insertions(+), 135 deletions(-)

-- 
2.17.0

[PATCH v6 01/12] 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 ac91b6fd0bcd..7c34a2ccb514 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2638,7 +2638,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 */
@@ -2661,12 +2668,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 v6 02/12] PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume

[Mika Westerberg]

After suspend/resume cycle the Presence Detect or Data Link Layer Status
Changed bits might be set and if we don't clear them those events will
not fire anymore and nothing happens for instance when a device is now
hot-unplugged.

Fix this by clearing those bits in a newly introduced function
pcie_reenable_notification(). This should be fine because immediately
after we check if the adapter is still present by reading directly from
the status register.

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
---
Regarding this:

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

I added debug print to pci_restore_pcie_state() right before PCI_EXP_SLTCTL
is restored that reads PCI_EXP_SLTSTA. Both flags (PCI_EXP_SLTSTA_PDC and
PCI_EXP_SLTSTA_DLLSC) are alread set so they don't happen after we restore
PCI_EXP_SLTCTL.

 drivers/pci/hotplug/pciehp.h      |  2 +-
 drivers/pci/hotplug/pciehp_core.c |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c  | 13 ++++++++++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 88e917c9120f..5f892065585e 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -121,7 +121,7 @@ struct controller *pcie_init(struct pcie_device *dev);
 int pcie_init_notification(struct controller *ctrl);
 int pciehp_enable_slot(struct slot *p_slot);
 int pciehp_disable_slot(struct slot *p_slot);
-void pcie_enable_notification(struct controller *ctrl);
+void pcie_reenable_notification(struct controller *ctrl);
 int pciehp_power_on_slot(struct slot *slot);
 void pciehp_power_off_slot(struct slot *slot);
 void pciehp_get_power_status(struct slot *slot, u8 *status);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 332b723ff9e6..44a6a63802d5 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -283,7 +283,7 @@ static int pciehp_resume(struct pcie_device *dev)
 	ctrl = get_service_data(dev);
 
 	/* reinitialize the chipset's event detection logic */
-	pcie_enable_notification(ctrl);
+	pcie_reenable_notification(ctrl);
 
 	slot = ctrl->slot;
 
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 18a42f8f5dc5..98ea75aa32c7 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -659,7 +659,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 	return handled;
 }
 
-void pcie_enable_notification(struct controller *ctrl)
+static void pcie_enable_notification(struct controller *ctrl)
 {
 	u16 cmd, mask;
 
@@ -697,6 +697,17 @@ void pcie_enable_notification(struct controller *ctrl)
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
 }
 
+void pcie_reenable_notification(struct controller *ctrl)
+{
+	/*
+	 * Clear both Presence and Data Link Layer Changed to make sure
+	 * those events still fire after we have re-enabled them.
+	 */
+	pcie_capability_write_word(ctrl->pcie->port, PCI_EXP_SLTSTA,
+				   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
+	pcie_enable_notification(ctrl);
+}
+
 static void pcie_disable_notification(struct controller *ctrl)
 {
 	u16 mask;
-- 
2.17.0

[PATCH v6 03/12] PCI: Request control of native PCIe hotplug only if supported

[Mika Westerberg]

Currently we request control of native PCIe hotplug unconditionally.
That may cause problems because native PCIe hotplug events are handled
by pciehp driver and if it is not enabled those events will be lost.
Make this bit more robust and request control of native PCIe hotplug
only if we are actually prepared to do so (pciehp driver is enabled).

While there rename host->native_hotplug to host->native_pcie_hotplug
because we do the same for SHPC hotplug in subsequent patches.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/pci_root.c         | 6 ++++--
 drivers/pci/pcie/portdrv_core.c | 2 +-
 drivers/pci/probe.c             | 2 +-
 include/linux/pci.h             | 2 +-
 include/linux/pci_hotplug.h     | 7 +++++++
 5 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 0da18bde6a16..97590dff6bd8 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -29,6 +29,7 @@
 #include <linux/pci.h>
 #include <linux/pci-acpi.h>
 #include <linux/pci-aspm.h>
+#include <linux/pci_hotplug.h>
 #include <linux/dmar.h>
 #include <linux/acpi.h>
 #include <linux/slab.h>
@@ -472,9 +473,10 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 	}
 
 	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
-		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
 		| OSC_PCI_EXPRESS_PME_CONTROL;
 
+	if (pciehp_available())
+		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
 	if (pci_aer_available()) {
 		if (aer_acpi_firmware_first())
 			dev_info(&device->dev,
@@ -900,7 +902,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
-		host_bridge->native_hotplug = 0;
+		host_bridge->native_pcie_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
 		host_bridge->native_aer = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index c9c0663db282..6cb30aec2452 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -199,7 +199,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	int services = 0;
 
 	if (dev->is_hotplug_bridge &&
-	    (pcie_ports_native || host->native_hotplug)) {
+	    (pcie_ports_native || host->native_pcie_hotplug)) {
 		services |= PCIE_PORT_SERVICE_HP;
 
 		/*
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7c34a2ccb514..a6c3b8d30f8f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -552,7 +552,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 	 * OS from interfering.
 	 */
 	bridge->native_aer = 1;
-	bridge->native_hotplug = 1;
+	bridge->native_pcie_hotplug = 1;
 	bridge->native_pme = 1;
 
 	return bridge;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 73178a2fcee0..359a197d0310 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -471,7 +471,7 @@ struct pci_host_bridge {
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
 	unsigned int	native_aer:1;		/* OS may use PCIe AER */
-	unsigned int	native_hotplug:1;	/* OS may use PCIe hotplug */
+	unsigned int	native_pcie_hotplug:1;	/* OS may use PCIe hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 26213024e81b..46fb90b5164b 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -174,4 +174,11 @@ static inline int pci_get_hp_params(struct pci_dev *dev,
 }
 static inline bool pciehp_is_native(struct pci_dev *pdev) { return true; }
 #endif
+
+#ifdef CONFIG_HOTPLUG_PCI_PCIE
+#define pciehp_available()	true
+#else
+#define pciehp_available()	false
+#endif
+
 #endif
-- 
2.17.0

[PATCH v6 04/12] PCI: Make pciehp_is_native() stricter

[Mika Westerberg]

Currently pciehp_is_native() returns true for any PCI device in a
hierarchy where _OSC says we can use pciehp. This is not correct because
there may be bridges without PCI_EXP_SLTCAP_HPC capability and those
should be managed by acpiphp instead.

Improve pciehp_is_native() to return true only if the given bridge is
actually expected to be handled by pciehp. In any other case return
false instead to let acpiphp handle those.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci-acpi.c      | 28 ++++++++++++++++------------
 drivers/pci/pcie/portdrv.h  |  2 --
 include/linux/pci.h         |  2 ++
 include/linux/pci_hotplug.h |  4 ++--
 4 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 1abdbf267c19..d3114f3a7ab8 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -370,26 +370,30 @@ EXPORT_SYMBOL_GPL(pci_get_hp_params);
 
 /**
  * pciehp_is_native - Check whether a hotplug port is handled by the OS
- * @pdev: Hotplug port to check
+ * @bridge: Hotplug port to check
  *
- * Walk up from @pdev to the host bridge, obtain its cached _OSC Control Field
- * and return the value of the "PCI Express Native Hot Plug control" bit.
- * On failure to obtain the _OSC Control Field return %false.
+ * Returns true if the given @bridge is handled by the native PCIe hotplug
+ * driver.
  */
-bool pciehp_is_native(struct pci_dev *pdev)
+bool pciehp_is_native(struct pci_dev *bridge)
 {
-	struct acpi_pci_root *root;
-	acpi_handle handle;
+	const struct pci_host_bridge *host;
+	u32 slot_cap;
 
-	handle = acpi_find_root_bridge_handle(pdev);
-	if (!handle)
+	if (!pciehp_available())
+		return false;
+	if (!bridge)
 		return false;
 
-	root = acpi_pci_find_root(handle);
-	if (!root)
+	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
+	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
 		return false;
 
-	return root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+	if (pcie_ports_native)
+		return true;
+
+	host = pci_find_host_bridge(bridge->bus);
+	return host->native_pcie_hotplug;
 }
 
 /**
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index d0c6783dbfe3..aa542dc10d23 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -11,8 +11,6 @@
 
 #include <linux/compiler.h>
 
-extern bool pcie_ports_native;
-
 /* Service Type */
 #define PCIE_PORT_SERVICE_PME_SHIFT	0	/* Power Management Event */
 #define PCIE_PORT_SERVICE_PME		(1 << PCIE_PORT_SERVICE_PME_SHIFT)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 359a197d0310..2f6689a14e8d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1451,8 +1451,10 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d,
 
 #ifdef CONFIG_PCIEPORTBUS
 extern bool pcie_ports_disabled;
+extern bool pcie_ports_native;
 #else
 #define pcie_ports_disabled	true
+#define pcie_ports_native	false
 #endif
 
 #ifdef CONFIG_PCIEASPM
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 46fb90b5164b..ee2b1674a601 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -162,7 +162,7 @@ struct hotplug_params {
 #ifdef CONFIG_ACPI
 #include <linux/acpi.h>
 int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp);
-bool pciehp_is_native(struct pci_dev *pdev);
+bool pciehp_is_native(struct pci_dev *bridge);
 int acpi_get_hp_hw_control_from_firmware(struct pci_dev *dev, u32 flags);
 int acpi_pci_check_ejectable(struct pci_bus *pbus, acpi_handle handle);
 int acpi_pci_detect_ejectable(acpi_handle handle);
@@ -172,7 +172,7 @@ static inline int pci_get_hp_params(struct pci_dev *dev,
 {
 	return -ENODEV;
 }
-static inline bool pciehp_is_native(struct pci_dev *pdev) { return true; }
+static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
 #endif
 
 #ifdef CONFIG_HOTPLUG_PCI_PCIE
-- 
2.17.0

[PATCH v6 05/12] PCI: hotplug: Convert SHPC to be builtin only

[Mika Westerberg]

We need to be able coordinate between SHPC and acpiphp in order to
determine which driver handles hotplug of a given bridge. Because
acpiphp is already bool convert SHPC to be bool as well.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/hotplug/Kconfig | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
index a8f21d051e0c..e9f78eb390d2 100644
--- a/drivers/pci/hotplug/Kconfig
+++ b/drivers/pci/hotplug/Kconfig
@@ -104,14 +104,11 @@ config HOTPLUG_PCI_CPCI_GENERIC
 	  When in doubt, say N.
 
 config HOTPLUG_PCI_SHPC
-	tristate "SHPC PCI Hotplug driver"
+	bool "SHPC PCI Hotplug driver"
 	help
 	  Say Y here if you have a motherboard with a SHPC PCI Hotplug
 	  controller.
 
-	  To compile this driver as a module, choose M here: the
-	  module will be called shpchp.
-
 	  When in doubt, say N.
 
 config HOTPLUG_PCI_POWERNV
-- 
2.17.0

[PATCH v6 06/12] PCI: Request control of native SHPC hotplug similarly to pciehp

[Mika Westerberg]

Now that the SHPC driver is converted to builtin we can change the way
SHPC control is requested to follow more closely what we do for pciehp.
We then take advantege of the newly introduced host->native_shpc_hotplug
in acpi_get_hp_hw_control_from_firmware() to determine whether OSHP
needs to be evaluated.

In addition provide two new functions that can be used to query whether
native SHPC driver is expected to handle hotplug of a given bridge or
not, following what we do for pciehp.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/pci_root.c           |  4 ++++
 drivers/pci/hotplug/acpi_pcihp.c  | 34 +++++++++++--------------------
 drivers/pci/hotplug/shpchp.h      | 11 ----------
 drivers/pci/hotplug/shpchp_core.c |  2 +-
 drivers/pci/pci-acpi.c            | 23 +++++++++++++++++++++
 drivers/pci/probe.c               |  1 +
 include/linux/pci.h               |  1 +
 include/linux/pci_hotplug.h       | 20 +++++++++++++++++-
 8 files changed, 61 insertions(+), 35 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 97590dff6bd8..42c079f782e3 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -477,6 +477,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 
 	if (pciehp_available())
 		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+	if (shpchp_available())
+		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 	if (pci_aer_available()) {
 		if (aer_acpi_firmware_first())
 			dev_info(&device->dev,
@@ -903,6 +905,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	host_bridge = to_pci_host_bridge(bus->bridge);
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
 		host_bridge->native_pcie_hotplug = 0;
+	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
+		host_bridge->native_shpc_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
 		host_bridge->native_aer = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index c9816166978e..678c03944bb7 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -62,23 +62,18 @@ static acpi_status acpi_run_oshp(acpi_handle handle)
 
 /**
  * acpi_get_hp_hw_control_from_firmware
- * @dev: the pci_dev of the bridge that has a hotplug controller
- * @flags: requested control bits for _OSC
+ * @pdev: the pci_dev of the bridge that has a hotplug controller
  *
  * Attempt to take hotplug control from firmware.
  */
-int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
+int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
 {
+	const struct pci_host_bridge *host;
+	const struct acpi_pci_root *root;
 	acpi_status status;
 	acpi_handle chandle, handle;
 	struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };
 
-	flags &= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
-	if (!flags) {
-		err("Invalid flags %u specified!\n", flags);
-		return -EINVAL;
-	}
-
 	/*
 	 * Per PCI firmware specification, we should run the ACPI _OSC
 	 * method to get control of hotplug hardware before using it. If
@@ -88,19 +83,14 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
 	 * OSHP within the scope of the hotplug controller and its parents,
 	 * up to the host bridge under which this controller exists.
 	 */
-	handle = acpi_find_root_bridge_handle(pdev);
-	if (handle) {
-		acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
-		dbg("Trying to get hotplug control for %s\n",
-				(char *)string.pointer);
-		status = acpi_pci_osc_control_set(handle, &flags, flags);
-		if (ACPI_SUCCESS(status))
-			goto got_one;
-		if (status == AE_SUPPORT)
-			goto no_control;
-		kfree(string.pointer);
-		string = (struct acpi_buffer){ ACPI_ALLOCATE_BUFFER, NULL };
-	}
+	host = pci_find_host_bridge(pdev->bus);
+	if (host->native_shpc_hotplug)
+		return 0;
+
+	/* If the there is _OSC we should not evaluate OSHP */
+	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
+	if (root->osc_support_set)
+		goto no_control;
 
 	handle = ACPI_HANDLE(&pdev->dev);
 	if (!handle) {
diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index c55730b61c9a..9675ab757323 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h
@@ -173,17 +173,6 @@ static inline const char *slot_name(struct slot *slot)
 	return hotplug_slot_name(slot->hotplug_slot);
 }
 
-#ifdef CONFIG_ACPI
-#include <linux/pci-acpi.h>
-static inline int get_hp_hw_control_from_firmware(struct pci_dev *dev)
-{
-	u32 flags = OSC_PCI_SHPC_NATIVE_HP_CONTROL;
-	return acpi_get_hp_hw_control_from_firmware(dev, flags);
-}
-#else
-#define get_hp_hw_control_from_firmware(dev) (0)
-#endif
-
 struct ctrl_reg {
 	volatile u32 base_offset;
 	volatile u32 slot_avail1;
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 1f0f96908b5a..47decc9b3bb3 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -277,7 +277,7 @@ static int is_shpc_capable(struct pci_dev *dev)
 		return 1;
 	if (!pci_find_capability(dev, PCI_CAP_ID_SHPC))
 		return 0;
-	if (get_hp_hw_control_from_firmware(dev))
+	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 d3114f3a7ab8..f912916a3a1b 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -396,6 +396,29 @@ 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 (!shpchp_available())
+		return false;
+	if (!bridge)
+		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/drivers/pci/probe.c b/drivers/pci/probe.c
index a6c3b8d30f8f..cd28ab122748 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -553,6 +553,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 	 */
 	bridge->native_aer = 1;
 	bridge->native_pcie_hotplug = 1;
+	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 
 	return bridge;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2f6689a14e8d..038c6d2d4ff4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -472,6 +472,7 @@ struct pci_host_bridge {
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
 	unsigned int	native_aer:1;		/* OS may use PCIe AER */
 	unsigned int	native_pcie_hotplug:1;	/* OS may use PCIe hotplug */
+	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index ee2b1674a601..f7d40d7f8100 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -163,7 +163,8 @@ struct hotplug_params {
 #include <linux/acpi.h>
 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 *dev, u32 flags);
+bool shpchp_is_native(struct pci_dev *bridge);
+int acpi_get_hp_hw_control_from_firmware(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
@@ -173,12 +174,29 @@ static inline int pci_get_hp_params(struct pci_dev *dev,
 	return -ENODEV;
 }
 static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
+static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
+
+static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
+{
+	return 0;
+}
 #endif
 
+static inline bool hotplug_is_native(struct pci_dev *bridge)
+{
+	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
+}
+
 #ifdef CONFIG_HOTPLUG_PCI_PCIE
 #define pciehp_available()	true
 #else
 #define pciehp_available()	false
 #endif
 
+#ifdef CONFIG_HOTPLUG_PCI_SHPC
+#define shpchp_available()	true
+#else
+#define shpchp_available()	false
+#endif
+
 #endif
-- 
2.17.0

[PATCH v6 07/12] 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>
---
 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 b45b375c0e6c..a8286058f490 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))) {
+	    && !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 (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 v6 08/12] 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 a8286058f490..db256bb2bd6a 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

[PATCH v6 09/12] 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 v6 10/12] 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 cd28ab122748..a0c48aa1c42f 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,
@@ -1231,6 +1233,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

[PATCH v6 11/12] PCI: Improve "partially hidden behind bridge" log message

[Mika Westerberg]

There is a sanity checker in the end of pci_scan_child_bus_extend() that
tries to detect badly configured bridges. For example given the below
topology:

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

The sanity checker notices this and logs following messages:

  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]

This is not really helpful to users and the information above is not
even correct (0000:02 is a bus not bridge). Make this a bit more
understandable by changing the sanity checker to log following message
in place of the above two messages:

  pci 0000:02:02.0: devices behind bridge are unusable, because [bus 3a] cannot be assigned for them

While there update the comment on top of the sanity checker block to
make it clear that it is not only restricted to CardBus.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/probe.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a0c48aa1c42f..10084990fbc3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1191,20 +1191,16 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
 		(is_cardbus ? "PCI CardBus %04x:%02x" : "PCI Bus %04x:%02x"),
 		pci_domain_nr(bus), child->number);
 
-	/* Has only triggered on CardBus, fixup is in yenta_socket */
+	/* Check that all devices are accessible */
 	while (bus->parent) {
 		if ((child->busn_res.end > bus->busn_res.end) ||
 		    (child->number > bus->busn_res.end) ||
 		    (child->number < bus->number) ||
 		    (child->busn_res.end < bus->number)) {
-			dev_info(&child->dev, "%pR %s hidden behind%s bridge %s %pR\n",
-				&child->busn_res,
-				(bus->number > child->busn_res.end &&
-				 bus->busn_res.end < child->number) ?
-					"wholly" : "partially",
-				bus->self->transparent ? " transparent" : "",
-				dev_name(&bus->dev),
-				&bus->busn_res);
+			dev_info(&dev->dev,
+				 "devices behind bridge are unusable, because %pR cannot be assigned for them\n",
+				 &child->busn_res);
+			break;
 		}
 		bus = bus->parent;
 	}
-- 
2.17.0

[PATCH v6 12/12] ACPI / hotplug / PCI: Drop unnecessary parentheses

[Mika Westerberg]

There is no need for them and it makes the code look uglier than it
should. Remove them.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index db256bb2bd6a..07d511d62296 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -522,7 +522,7 @@ static void enable_slot(struct acpiphp_slot *slot)
 		if (!dev) {
 			/* Do not set SLOT_ENABLED flag if some funcs
 			   are not added. */
-			slot->flags &= (~SLOT_ENABLED);
+			slot->flags &= ~SLOT_ENABLED;
 			continue;
 		}
 	}
@@ -551,7 +551,7 @@ static void disable_slot(struct acpiphp_slot *slot)
 	list_for_each_entry(func, &slot->funcs, sibling)
 		acpi_bus_trim(func_to_acpi_device(func));
 
-	slot->flags &= (~SLOT_ENABLED);
+	slot->flags &= ~SLOT_ENABLED;
 }
 
 static bool slot_no_hotplug(struct acpiphp_slot *slot)
-- 
2.17.0

Re: [PATCH v6 03/12] PCI: Request control of native PCIe hotplug only if supported

[Lukas Wunner]

On Thu, May 10, 2018 at 09:28:35PM +0300, Mika Westerberg wrote:
> +
> +#ifdef CONFIG_HOTPLUG_PCI_PCIE
> +#define pciehp_available()	true
> +#else
> +#define pciehp_available()	false
> +#endif
> +

More succinctly,

#define pciehp_available() IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)

(Or use that directly instead of defining an additional macro.)

Thanks,

Lukas

Re: [PATCH v6 04/12] PCI: Make pciehp_is_native() stricter

[Lukas Wunner]

On Thu, May 10, 2018 at 09:28:36PM +0300, Mika Westerberg wrote:
> +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> +	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
>  		return false;

Hm, any reason not to use "if (!bridge->is_hotplug_bridge)" ?

Thanks,

Lukas

Re: [PATCH v6 04/12] PCI: Make pciehp_is_native() stricter

[Mika Westerberg]

On Thu, May 10, 2018 at 09:51:49PM +0200, Lukas Wunner wrote:
> On Thu, May 10, 2018 at 09:28:36PM +0300, Mika Westerberg wrote:
> > +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> > +	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
> >  		return false;
> 
> Hm, any reason not to use "if (!bridge->is_hotplug_bridge)" ?

That can be set for non-native PCIe bridges. For example acpiphp sets it
in some cases. Here we read the capability directly to make sure we are
really dealing with native PCIe hotplug.

Re: [PATCH v6 03/12] PCI: Request control of native PCIe hotplug only if supported

[Rafael J. Wysocki]

On Thursday, May 10, 2018 9:44:33 PM CEST Lukas Wunner wrote:
> On Thu, May 10, 2018 at 09:28:35PM +0300, Mika Westerberg wrote:
> > +
> > +#ifdef CONFIG_HOTPLUG_PCI_PCIE
> > +#define pciehp_available()	true
> > +#else
> > +#define pciehp_available()	false
> > +#endif
> > +
> 
> More succinctly,
> 
> #define pciehp_available() IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)
> 
> (Or use that directly instead of defining an additional macro.)

Right and I would do the latter.

Thanks,
Rafael

Re: [PATCH v6 03/12] PCI: Request control of native PCIe hotplug only if supported

[Rafael J. Wysocki]

On Thursday, May 10, 2018 8:28:35 PM CEST Mika Westerberg wrote:
> Currently we request control of native PCIe hotplug unconditionally.
> That may cause problems because native PCIe hotplug events are handled
> by pciehp driver and if it is not enabled those events will be lost.
> Make this bit more robust and request control of native PCIe hotplug
> only if we are actually prepared to do so (pciehp driver is enabled).
> 
> While there rename host->native_hotplug to host->native_pcie_hotplug
> because we do the same for SHPC hotplug in subsequent patches.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/pci_root.c         | 6 ++++--
>  drivers/pci/pcie/portdrv_core.c | 2 +-
>  drivers/pci/probe.c             | 2 +-
>  include/linux/pci.h             | 2 +-
>  include/linux/pci_hotplug.h     | 7 +++++++
>  5 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 0da18bde6a16..97590dff6bd8 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -29,6 +29,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-acpi.h>
>  #include <linux/pci-aspm.h>
> +#include <linux/pci_hotplug.h>
>  #include <linux/dmar.h>
>  #include <linux/acpi.h>
>  #include <linux/slab.h>
> @@ -472,9 +473,10 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>  	}
>  
>  	control = OSC_PCI_EXPRESS_CAPABILITY_CONTROL
> -		| OSC_PCI_EXPRESS_NATIVE_HP_CONTROL
>  		| OSC_PCI_EXPRESS_PME_CONTROL;
>  
> +	if (pciehp_available())
> +		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
>  	if (pci_aer_available()) {
>  		if (aer_acpi_firmware_first())
>  			dev_info(&device->dev,
> @@ -900,7 +902,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> -		host_bridge->native_hotplug = 0;
> +		host_bridge->native_pcie_hotplug = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
>  		host_bridge->native_aer = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index c9c0663db282..6cb30aec2452 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -199,7 +199,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	int services = 0;
>  
>  	if (dev->is_hotplug_bridge &&
> -	    (pcie_ports_native || host->native_hotplug)) {
> +	    (pcie_ports_native || host->native_pcie_hotplug)) {
>  		services |= PCIE_PORT_SERVICE_HP;
>  
>  		/*
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 7c34a2ccb514..a6c3b8d30f8f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -552,7 +552,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>  	 * OS from interfering.
>  	 */
>  	bridge->native_aer = 1;
> -	bridge->native_hotplug = 1;
> +	bridge->native_pcie_hotplug = 1;
>  	bridge->native_pme = 1;
>  
>  	return bridge;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 73178a2fcee0..359a197d0310 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -471,7 +471,7 @@ struct pci_host_bridge {
>  	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>  	unsigned int	no_ext_tags:1;		/* No Extended Tags */
>  	unsigned int	native_aer:1;		/* OS may use PCIe AER */
> -	unsigned int	native_hotplug:1;	/* OS may use PCIe hotplug */
> +	unsigned int	native_pcie_hotplug:1;	/* OS may use PCIe hotplug */
>  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 26213024e81b..46fb90b5164b 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -174,4 +174,11 @@ static inline int pci_get_hp_params(struct pci_dev *dev,
>  }
>  static inline bool pciehp_is_native(struct pci_dev *pdev) { return true; }
>  #endif
> +
> +#ifdef CONFIG_HOTPLUG_PCI_PCIE
> +#define pciehp_available()	true
> +#else
> +#define pciehp_available()	false
> +#endif
> +
>  #endif
> 

Modulo the Lucas' comment on using IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)
directly instead of the above:

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

Re: [PATCH v6 04/12] PCI: Make pciehp_is_native() stricter

[Rafael J. Wysocki]

On Thursday, May 10, 2018 8:28:36 PM CEST Mika Westerberg wrote:
> Currently pciehp_is_native() returns true for any PCI device in a
> hierarchy where _OSC says we can use pciehp. This is not correct because
> there may be bridges without PCI_EXP_SLTCAP_HPC capability and those
> should be managed by acpiphp instead.
> 
> Improve pciehp_is_native() to return true only if the given bridge is
> actually expected to be handled by pciehp. In any other case return
> false instead to let acpiphp handle those.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pci-acpi.c      | 28 ++++++++++++++++------------
>  drivers/pci/pcie/portdrv.h  |  2 --
>  include/linux/pci.h         |  2 ++
>  include/linux/pci_hotplug.h |  4 ++--
>  4 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 1abdbf267c19..d3114f3a7ab8 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -370,26 +370,30 @@ EXPORT_SYMBOL_GPL(pci_get_hp_params);
>  
>  /**
>   * pciehp_is_native - Check whether a hotplug port is handled by the OS
> - * @pdev: Hotplug port to check
> + * @bridge: Hotplug port to check
>   *
> - * Walk up from @pdev to the host bridge, obtain its cached _OSC Control Field
> - * and return the value of the "PCI Express Native Hot Plug control" bit.
> - * On failure to obtain the _OSC Control Field return %false.
> + * Returns true if the given @bridge is handled by the native PCIe hotplug
> + * driver.
>   */
> -bool pciehp_is_native(struct pci_dev *pdev)
> +bool pciehp_is_native(struct pci_dev *bridge)
>  {
> -	struct acpi_pci_root *root;
> -	acpi_handle handle;
> +	const struct pci_host_bridge *host;
> +	u32 slot_cap;
>  
> -	handle = acpi_find_root_bridge_handle(pdev);
> -	if (!handle)
> +	if (!pciehp_available())
> +		return false;
> +	if (!bridge)
>  		return false;

You could write this as 

    if (!pciehp_available() || !bridge)
        return false;

Would be equivalent, but more concise IMO.

But anyway

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

Re: [PATCH v6 05/12] PCI: hotplug: Convert SHPC to be builtin only

[Rafael J. Wysocki]

On Thursday, May 10, 2018 8:28:37 PM CEST Mika Westerberg wrote:
> We need to be able coordinate between SHPC and acpiphp in order to
> determine which driver handles hotplug of a given bridge. Because
> acpiphp is already bool convert SHPC to be bool as well.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/hotplug/Kconfig | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
> index a8f21d051e0c..e9f78eb390d2 100644
> --- a/drivers/pci/hotplug/Kconfig
> +++ b/drivers/pci/hotplug/Kconfig
> @@ -104,14 +104,11 @@ config HOTPLUG_PCI_CPCI_GENERIC
>  	  When in doubt, say N.
>  
>  config HOTPLUG_PCI_SHPC
> -	tristate "SHPC PCI Hotplug driver"
> +	bool "SHPC PCI Hotplug driver"
>  	help
>  	  Say Y here if you have a motherboard with a SHPC PCI Hotplug
>  	  controller.
>  
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called shpchp.
> -
>  	  When in doubt, say N.
>  
>  config HOTPLUG_PCI_POWERNV
> 

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

Re: [PATCH v6 06/12] PCI: Request control of native SHPC hotplug similarly to pciehp

[Rafael J. Wysocki]

On Thursday, May 10, 2018 8:28:38 PM CEST Mika Westerberg wrote:
> Now that the SHPC driver is converted to builtin we can change the way
> SHPC control is requested to follow more closely what we do for pciehp.
> We then take advantege of the newly introduced host->native_shpc_hotplug
> in acpi_get_hp_hw_control_from_firmware() to determine whether OSHP
> needs to be evaluated.
> 
> In addition provide two new functions that can be used to query whether
> native SHPC driver is expected to handle hotplug of a given bridge or
> not, following what we do for pciehp.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/pci_root.c           |  4 ++++
>  drivers/pci/hotplug/acpi_pcihp.c  | 34 +++++++++++--------------------
>  drivers/pci/hotplug/shpchp.h      | 11 ----------
>  drivers/pci/hotplug/shpchp_core.c |  2 +-
>  drivers/pci/pci-acpi.c            | 23 +++++++++++++++++++++
>  drivers/pci/probe.c               |  1 +
>  include/linux/pci.h               |  1 +
>  include/linux/pci_hotplug.h       | 20 +++++++++++++++++-
>  8 files changed, 61 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 97590dff6bd8..42c079f782e3 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -477,6 +477,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>  
>  	if (pciehp_available())
>  		control |= OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
> +	if (shpchp_available())
> +		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>  	if (pci_aer_available()) {
>  		if (aer_acpi_firmware_first())
>  			dev_info(&device->dev,
> @@ -903,6 +905,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  	host_bridge = to_pci_host_bridge(bus->bridge);
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
>  		host_bridge->native_pcie_hotplug = 0;
> +	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> +		host_bridge->native_shpc_hotplug = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
>  		host_bridge->native_aer = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index c9816166978e..678c03944bb7 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -62,23 +62,18 @@ static acpi_status acpi_run_oshp(acpi_handle handle)
>  
>  /**
>   * acpi_get_hp_hw_control_from_firmware
> - * @dev: the pci_dev of the bridge that has a hotplug controller
> - * @flags: requested control bits for _OSC
> + * @pdev: the pci_dev of the bridge that has a hotplug controller
>   *
>   * Attempt to take hotplug control from firmware.
>   */
> -int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
> +int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev)
>  {
> +	const struct pci_host_bridge *host;
> +	const struct acpi_pci_root *root;
>  	acpi_status status;
>  	acpi_handle chandle, handle;
>  	struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };
>  
> -	flags &= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> -	if (!flags) {
> -		err("Invalid flags %u specified!\n", flags);
> -		return -EINVAL;
> -	}
> -
>  	/*
>  	 * Per PCI firmware specification, we should run the ACPI _OSC
>  	 * method to get control of hotplug hardware before using it. If
> @@ -88,19 +83,14 @@ int acpi_get_hp_hw_control_from_firmware(struct pci_dev *pdev, u32 flags)
>  	 * OSHP within the scope of the hotplug controller and its parents,
>  	 * up to the host bridge under which this controller exists.
>  	 */
> -	handle = acpi_find_root_bridge_handle(pdev);
> -	if (handle) {
> -		acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> -		dbg("Trying to get hotplug control for %s\n",
> -				(char *)string.pointer);
> -		status = acpi_pci_osc_control_set(handle, &flags, flags);
> -		if (ACPI_SUCCESS(status))
> -			goto got_one;
> -		if (status == AE_SUPPORT)
> -			goto no_control;
> -		kfree(string.pointer);
> -		string = (struct acpi_buffer){ ACPI_ALLOCATE_BUFFER, NULL };
> -	}
> +	host = pci_find_host_bridge(pdev->bus);
> +	if (host->native_shpc_hotplug)
> +		return 0;
> +
> +	/* If the there is _OSC we should not evaluate OSHP */
> +	root = acpi_pci_find_root(ACPI_HANDLE(&host->dev));
> +	if (root->osc_support_set)
> +		goto no_control;
>  
>  	handle = ACPI_HANDLE(&pdev->dev);
>  	if (!handle) {
> diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
> index c55730b61c9a..9675ab757323 100644
> --- a/drivers/pci/hotplug/shpchp.h
> +++ b/drivers/pci/hotplug/shpchp.h
> @@ -173,17 +173,6 @@ static inline const char *slot_name(struct slot *slot)
>  	return hotplug_slot_name(slot->hotplug_slot);
>  }
>  
> -#ifdef CONFIG_ACPI
> -#include <linux/pci-acpi.h>
> -static inline int get_hp_hw_control_from_firmware(struct pci_dev *dev)
> -{
> -	u32 flags = OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> -	return acpi_get_hp_hw_control_from_firmware(dev, flags);
> -}
> -#else
> -#define get_hp_hw_control_from_firmware(dev) (0)
> -#endif
> -
>  struct ctrl_reg {
>  	volatile u32 base_offset;
>  	volatile u32 slot_avail1;
> diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
> index 1f0f96908b5a..47decc9b3bb3 100644
> --- a/drivers/pci/hotplug/shpchp_core.c
> +++ b/drivers/pci/hotplug/shpchp_core.c
> @@ -277,7 +277,7 @@ static int is_shpc_capable(struct pci_dev *dev)
>  		return 1;
>  	if (!pci_find_capability(dev, PCI_CAP_ID_SHPC))
>  		return 0;
> -	if (get_hp_hw_control_from_firmware(dev))
> +	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 d3114f3a7ab8..f912916a3a1b 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -396,6 +396,29 @@ 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 (!shpchp_available())
> +		return false;
> +	if (!bridge)
> +		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/drivers/pci/probe.c b/drivers/pci/probe.c
> index a6c3b8d30f8f..cd28ab122748 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -553,6 +553,7 @@ struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>  	 */
>  	bridge->native_aer = 1;
>  	bridge->native_pcie_hotplug = 1;
> +	bridge->native_shpc_hotplug = 1;
>  	bridge->native_pme = 1;
>  
>  	return bridge;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2f6689a14e8d..038c6d2d4ff4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -472,6 +472,7 @@ struct pci_host_bridge {
>  	unsigned int	no_ext_tags:1;		/* No Extended Tags */
>  	unsigned int	native_aer:1;		/* OS may use PCIe AER */
>  	unsigned int	native_pcie_hotplug:1;	/* OS may use PCIe hotplug */
> +	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
>  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index ee2b1674a601..f7d40d7f8100 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -163,7 +163,8 @@ struct hotplug_params {
>  #include <linux/acpi.h>
>  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 *dev, u32 flags);
> +bool shpchp_is_native(struct pci_dev *bridge);
> +int acpi_get_hp_hw_control_from_firmware(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
> @@ -173,12 +174,29 @@ static inline int pci_get_hp_params(struct pci_dev *dev,
>  	return -ENODEV;
>  }
>  static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
> +static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
> +
> +static inline int acpi_get_hp_hw_control_from_firmware(struct pci_dev *bridge)
> +{
> +	return 0;
> +}
>  #endif
>  
> +static inline bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> +}
> +
>  #ifdef CONFIG_HOTPLUG_PCI_PCIE
>  #define pciehp_available()	true
>  #else
>  #define pciehp_available()	false
>  #endif
>  
> +#ifdef CONFIG_HOTPLUG_PCI_SHPC
> +#define shpchp_available()	true
> +#else
> +#define shpchp_available()	false
> +#endif
> +
>  #endif
> 

In analogy with the PCIe case, you can use
IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC) directly instead of defining a new macro
for that.

Apart from the above:

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

Re: [PATCH v6 11/12] PCI: Improve "partially hidden behind bridge" log message

[Rafael J. Wysocki]

On Thursday, May 10, 2018 8:28:43 PM CEST Mika Westerberg wrote:
> There is a sanity checker in the end of pci_scan_child_bus_extend() that
> tries to detect badly configured bridges. For example given the below
> topology:
> 
>   +-1b.0-[01-39]----00.0-[02-3a]--+-00.0-[03]----00.0
>                                   +-01.0-[04-39]--
>                                   \-02.0-[3a]----00.0
> 
> The sanity checker notices this and logs following messages:
> 
>   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]
> 
> This is not really helpful to users and the information above is not
> even correct (0000:02 is a bus not bridge). Make this a bit more
> understandable by changing the sanity checker to log following message
> in place of the above two messages:
> 
>   pci 0000:02:02.0: devices behind bridge are unusable, because [bus 3a] cannot be assigned for them
> 
> While there update the comment on top of the sanity checker block to
> make it clear that it is not only restricted to CardBus.
> 
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/probe.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index a0c48aa1c42f..10084990fbc3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1191,20 +1191,16 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
>  		(is_cardbus ? "PCI CardBus %04x:%02x" : "PCI Bus %04x:%02x"),
>  		pci_domain_nr(bus), child->number);
>  
> -	/* Has only triggered on CardBus, fixup is in yenta_socket */
> +	/* Check that all devices are accessible */
>  	while (bus->parent) {
>  		if ((child->busn_res.end > bus->busn_res.end) ||
>  		    (child->number > bus->busn_res.end) ||
>  		    (child->number < bus->number) ||
>  		    (child->busn_res.end < bus->number)) {
> -			dev_info(&child->dev, "%pR %s hidden behind%s bridge %s %pR\n",
> -				&child->busn_res,
> -				(bus->number > child->busn_res.end &&
> -				 bus->busn_res.end < child->number) ?
> -					"wholly" : "partially",
> -				bus->self->transparent ? " transparent" : "",
> -				dev_name(&bus->dev),
> -				&bus->busn_res);
> +			dev_info(&dev->dev,
> +				 "devices behind bridge are unusable, because %pR cannot be assigned for them\n",
> +				 &child->busn_res);
> +			break;
>  		}
>  		bus = bus->parent;
>  	}
> 

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

Re: [PATCH v6 07/12] ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used

[Rafael J. Wysocki]

On Thursday, May 10, 2018 8:28:39 PM CEST 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.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.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 b45b375c0e6c..a8286058f490 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))) {
> +	    && !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 (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);
> 

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

Re: [PATCH v6 04/12] PCI: Make pciehp_is_native() stricter

[Bjorn Helgaas]

On Tue, May 15, 2018 at 11:17:14AM +0200, Rafael J. Wysocki wrote:
> On Thursday, May 10, 2018 8:28:36 PM CEST Mika Westerberg wrote:
> > Currently pciehp_is_native() returns true for any PCI device in a
> > hierarchy where _OSC says we can use pciehp. This is not correct because
> > there may be bridges without PCI_EXP_SLTCAP_HPC capability and those
> > should be managed by acpiphp instead.
> > 
> > Improve pciehp_is_native() to return true only if the given bridge is
> > actually expected to be handled by pciehp. In any other case return
> > false instead to let acpiphp handle those.
> > 
> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/pci-acpi.c      | 28 ++++++++++++++++------------
> >  drivers/pci/pcie/portdrv.h  |  2 --
> >  include/linux/pci.h         |  2 ++
> >  include/linux/pci_hotplug.h |  4 ++--
> >  4 files changed, 20 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 1abdbf267c19..d3114f3a7ab8 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -370,26 +370,30 @@ EXPORT_SYMBOL_GPL(pci_get_hp_params);
> >  
> >  /**
> >   * pciehp_is_native - Check whether a hotplug port is handled by the OS
> > - * @pdev: Hotplug port to check
> > + * @bridge: Hotplug port to check
> >   *
> > - * Walk up from @pdev to the host bridge, obtain its cached _OSC Control Field
> > - * and return the value of the "PCI Express Native Hot Plug control" bit.
> > - * On failure to obtain the _OSC Control Field return %false.
> > + * Returns true if the given @bridge is handled by the native PCIe hotplug
> > + * driver.
> >   */
> > -bool pciehp_is_native(struct pci_dev *pdev)
> > +bool pciehp_is_native(struct pci_dev *bridge)
> >  {
> > -	struct acpi_pci_root *root;
> > -	acpi_handle handle;
> > +	const struct pci_host_bridge *host;
> > +	u32 slot_cap;
> >  
> > -	handle = acpi_find_root_bridge_handle(pdev);
> > -	if (!handle)
> > +	if (!pciehp_available())
> > +		return false;
> > +	if (!bridge)
> >  		return false;
> 
> You could write this as 
> 
>     if (!pciehp_available() || !bridge)
>         return false;
> 
> Would be equivalent, but more concise IMO.

Why do we even need to bother checking "bridge" for NULL?

I don't believe in gratuitously checking for NULL because it can hide
higher-level bugs, i.e., if this is called with a NULL pointer, that's
likely a bug in the caller, and if we silently return "false", we'll
never discover the caller's bug.

Re: [PATCH v6 00/12] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Bjorn Helgaas]

On Thu, May 10, 2018 at 09:28:32PM +0300, Mika Westerberg wrote:
> Hi,
> 
> 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 [7/12].
> 
> 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:
> 
>   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 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 (12):
>   PCI: Take all bridges into account when calculating bus numbers for extension
>   PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume
>   PCI: Request control of native PCIe hotplug only if supported
>   PCI: Make pciehp_is_native() stricter
>   PCI: hotplug: Convert SHPC to be builtin only
>   PCI: Request control of native SHPC hotplug similarly to pciehp
>   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()
>   PCI: Improve "partially hidden behind bridge" log message
>   ACPI / hotplug / PCI: Drop unnecessary parentheses

I applied the following to pci/hotplug for v4.18:

  PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume
  PCI: Request control of native PCIe hotplug only if supported
  PCI: Rename host->native_hotplug to host->native_pcie_hotplug
  PCI: hotplug: Make pciehp_is_native() stricter
  PCI: hotplug: Convert SHPC to be builtin only
  PCI: shpchp: Simplify acpi_get_hp_hw_control_from_firmware()
  PCI: shpchp: Request control of native SHPC hotplug via _OSC
  PCI: Improve "partially hidden behind bridge" log message
  ACPI / hotplug / PCI: Drop unnecessary parentheses

I split a couple of these into smaller pieces and removed one NULL
pointer check, but otherwise they're straightforward.

These are the simple pieces that probably don't fix anything by
themselves.  I'm looking at the other parts too and have a couple
comments there, but at least we can clear these off the plate.

Re: [PATCH v6 04/12] PCI: Make pciehp_is_native() stricter

[Mika Westerberg]

On Thu, May 24, 2018 at 01:08:37PM -0500, Bjorn Helgaas wrote:
> Why do we even need to bother checking "bridge" for NULL?
> 
> I don't believe in gratuitously checking for NULL because it can hide
> higher-level bugs, i.e., if this is called with a NULL pointer, that's
> likely a bug in the caller, and if we silently return "false", we'll
> never discover the caller's bug.

But it also makes caller's life easier because now you don't need to do
this:

   if (bus->self && hotplug_is_native(bus->self))

because it is perfectly legal to call that function for host bridge. So
you do this instead

   if (hotplug_is_native(bus->self))

and it is not hiding a bug IMHO.

Re: [PATCH v6 04/12] PCI: Make pciehp_is_native() stricter

[Bjorn Helgaas]

On Thu, May 24, 2018 at 09:31:07PM +0300, Mika Westerberg wrote:
> On Thu, May 24, 2018 at 01:08:37PM -0500, Bjorn Helgaas wrote:
> > Why do we even need to bother checking "bridge" for NULL?
> > 
> > I don't believe in gratuitously checking for NULL because it can hide
> > higher-level bugs, i.e., if this is called with a NULL pointer, that's
> > likely a bug in the caller, and if we silently return "false", we'll
> > never discover the caller's bug.
> 
> But it also makes caller's life easier because now you don't need to do
> this:
> 
>    if (bus->self && hotplug_is_native(bus->self))
> 
> because it is perfectly legal to call that function for host bridge. So
> you do this instead
> 
>    if (hotplug_is_native(bus->self))
> 
> and it is not hiding a bug IMHO.

True, it's not hiding a bug, but I think it is useful for readers of
acpiphp_add_context() to see the NULL check there and realize that
bus->self (or "pdev" in current upstream) is allowed to be NULL.  That
helps understand the way acpiphp works (well, I understand very little
of it, but every little clue helps).

Re: [PATCH v6 00/12] PCI: Fixes and cleanups for native PCIe, SHPC and ACPI hotplug

[Mika Westerberg]

On Thu, May 24, 2018 at 01:30:05PM -0500, Bjorn Helgaas wrote:
> I applied the following to pci/hotplug for v4.18:
> 
>   PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume
>   PCI: Request control of native PCIe hotplug only if supported
>   PCI: Rename host->native_hotplug to host->native_pcie_hotplug
>   PCI: hotplug: Make pciehp_is_native() stricter
>   PCI: hotplug: Convert SHPC to be builtin only
>   PCI: shpchp: Simplify acpi_get_hp_hw_control_from_firmware()
>   PCI: shpchp: Request control of native SHPC hotplug via _OSC
>   PCI: Improve "partially hidden behind bridge" log message
>   ACPI / hotplug / PCI: Drop unnecessary parentheses
> 
> I split a couple of these into smaller pieces and removed one NULL
> pointer check, but otherwise they're straightforward.

Thanks!

> These are the simple pieces that probably don't fix anything by
> themselves.  I'm looking at the other parts too and have a couple
> comments there, but at least we can clear these off the plate.

You are right, the patches you applied don't fix anything and are more
like "nice to have" material. The critical patches that you did not
apply are:

  PCI: Take all bridges into account when calculating bus numbers for extension
  ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used

These two IMHO should be prioritized higher than doing some cleanup for
SHPC which pretty much is not used in modern systems anyway. Without the
above two patches all systems using native mode will not work properly
when you plug in TBT or normal USB-C device.