[PATCH 0/9] PCIe port PM: high gloss polish & hotplug support

[PATCH 0/9] PCIe port PM: high gloss polish & hotplug support

[Lukas Wunner]

Linux v4.8 added support to runtime suspend PCIe ports to D3hot.

Patches 1 to 6 of this series apply high gloss polish to that code to
speed it up, reduce its size and improve its readability.  None of these
patches should result in a functional change.  (And they didn't in my
tests.)

Patches 7 and 8 are cleanups / refactorings for acpiphp.  They're prep
work for patch 9 and likewise shouldn't cause a functional change.

Patch 9 extends runtime PM support to hotplug ports.


@Keith Busch: I seem to recall you're working with large arrays of NVMe
drives connected to native PCIe hotplug ports.  Perhaps you could give
this series a spin, it might save quite a bit of power.

To ease reviewing I've pushed this series to GitHub:
https://github.com/l1k/linux/commits/pcie_port_pm_v1

Thanks,

Lukas


Lukas Wunner (9):
  PCI: Don't acquire ref on parent in pci_bridge_d3_update()
  PCI: Autosense device removal in pci_bridge_d3_update()
  PCI: Speed up algorithm in pci_bridge_d3_update()
  PCI: Activate runtime PM on a PCIe port only if it can suspend
  PCI: Consolidate conditions to allow runtime PM on PCIe ports
  PCI: Unfold conditions to block runtime PM on PCIe ports
  ACPI / hotplug / PCI: Use cached copy of PCI_EXP_SLTCAP_HPC bit
  ACPI / hotplug / PCI: Make device_is_managed_by_native_pciehp() public
  PCI: pciehp: Add runtime PM support for PCIe hotplug ports

 drivers/pci/bus.c                  |   2 +-
 drivers/pci/hotplug/acpiphp_glue.c |  31 +-----------
 drivers/pci/hotplug/pciehp_ctrl.c  |   6 +++
 drivers/pci/pci-acpi.c             |  24 +++++++++
 drivers/pci/pci.c                  | 101 ++++++++++++++++++-------------------
 drivers/pci/pci.h                  |   4 +-
 drivers/pci/pcie/portdrv_pci.c     |  13 ++---
 drivers/pci/remove.c               |   2 +-
 include/linux/pci_hotplug.h        |   2 +
 9 files changed, 89 insertions(+), 96 deletions(-)

-- 
2.9.3

[PATCH 1/9] PCI: Don't acquire ref on parent in pci_bridge_d3_update()

[Lukas Wunner]

This function is always called with an existing pci_dev struct, which
holds a reference on the pci_bus struct it resides on, which in turn
holds a reference on pci_bus->bridge, which is the pci_dev's parent.

Hence there's no need to acquire an additional ref on the parent.

More specifically, the pci_dev exists until pci_destroy_dev() drops the
final reference on it, so all calls to pci_bridge_d3_update() must be
finished before that.  It is arguably the caller's responsibility to
ensure that it doesn't call pci_bridge_d3_update() with a pci_dev that
might suddenly disappear, but in any case the existing callers are all
safe:

- The call in pci_destroy_dev() happens before the call to put_device().
- The call in pci_bus_add_device() is synchronized with pci_destroy_dev()
  using pci_lock_rescan_remove().
- The calls to pci_d3cold_disable() from the xhci and nouveau drivers
  are safe because a ref on the pci_dev is held as long as it's bound to
  a driver.
- The calls to pci_d3cold_enable() / pci_d3cold_disable() when modifying
  the sysfs "d3cold_allowed" entry are also safe because kernfs_drain()
  waits for existing sysfs users to finish before removing the entry,
  and pci_destroy_dev() is called way after that.

No functional change intended.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ba34907..6e0c399 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2292,7 +2292,6 @@ static void pci_bridge_d3_update(struct pci_dev *dev, bool remove)
 	if (!bridge || !pci_bridge_d3_possible(bridge))
 		return;
 
-	pci_dev_get(bridge);
 	/*
 	 * If the device is removed we do not care about its D3cold
 	 * capabilities.
@@ -2314,8 +2313,6 @@ static void pci_bridge_d3_update(struct pci_dev *dev, bool remove)
 		/* Propagate change to upstream bridges */
 		pci_bridge_d3_update(bridge, false);
 	}
-
-	pci_dev_put(bridge);
 }
 
 /**
-- 
2.9.3

[PATCH 9/9] PCI: pciehp: Add runtime PM support for PCIe hotplug ports

[Lukas Wunner]

Linux 4.8 added support for runtime suspending PCIe ports to D3hot with
commit 006d44e49a25 ("PCI: Add runtime PM support for PCIe ports"), but
excluded hotplug ports.  Those are now afforded runtime PM by the
present commit.

Hotplug ports require a few extra considerations:

- The configuration space of the port remains accessible in D3hot, so
  all the functions to read or modify the Slot Status and Slot Control
  registers need not be modified.  Even turning on slot power doesn't
  seem to require the port to be in D0, at least the PCIe spec doesn't
  say so and I confirmed that by testing with a Thunderbolt controller.

- However D0 is required to access devices on the subordinate bus.  This
  happens in pciehp_check_link_status() and pciehp_configure_device()
  (both called from board_added()) and in pciehp_unconfigure_device()
  (called from remove_board()), so acquire a runtime PM ref for their
  invocation.

- The hotplug port stays active as long as it has active children.  If
  all hotplugged devices below the port runtime suspend, the port is
  allowed to runtime suspend as well.  Plug and unplug detection
  continues to work in D3hot.

- Hotplug interrupts are delivered in-band, so while the hotplug port
  itself is allowed to go to D3hot, its parent ports must stay in D0 for
  interrupts to come through.  Add a corresponding restriction to
  pci_dev_check_d3cold().

- Runtime PM may only be allowed if the hotplug port is handled natively
  by the OS.  On ACPI systems, the port may alternatively be handled by
  the firmware and things break if the OS puts the port into D3 behind
  the firmware's back:  E.g. Thunderbolt hotplug ports on non-Macs are
  handled by Intel's firmware in System Management Mode and the firmware
  is known to access devices on the port's subordinate bus without
  checking first if the port is in D0:
  https://bugzilla.kernel.org/show_bug.cgi?id=53811

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 6 ++++++
 drivers/pci/pci.c                 | 9 +++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index efe69e8..ffd3fe6 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -31,6 +31,7 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <linux/pci.h>
 #include "../pci.h"
 #include "pciehp.h"
@@ -98,6 +99,7 @@ static int board_added(struct slot *p_slot)
 	pciehp_green_led_blink(p_slot);
 
 	/* Check link training status */
+	pm_runtime_get_sync(&ctrl->pcie->port->dev);
 	retval = pciehp_check_link_status(ctrl);
 	if (retval) {
 		ctrl_err(ctrl, "Failed to check link status\n");
@@ -118,12 +120,14 @@ static int board_added(struct slot *p_slot)
 		if (retval != -EEXIST)
 			goto err_exit;
 	}
+	pm_runtime_put(&ctrl->pcie->port->dev);
 
 	pciehp_green_led_on(p_slot);
 	pciehp_set_attention_status(p_slot, 0);
 	return 0;
 
 err_exit:
+	pm_runtime_put(&ctrl->pcie->port->dev);
 	set_slot_off(ctrl, p_slot);
 	return retval;
 }
@@ -137,7 +141,9 @@ static int remove_board(struct slot *p_slot)
 	int retval;
 	struct controller *ctrl = p_slot->ctrl;
 
+	pm_runtime_get_sync(&ctrl->pcie->port->dev);
 	retval = pciehp_unconfigure_device(p_slot);
+	pm_runtime_put(&ctrl->pcie->port->dev);
 	if (retval)
 		return retval;
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f1ddb6b..b10bcca 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2241,13 +2241,10 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 			return false;
 
 		/*
-		 * Hotplug interrupts cannot be delivered if the link is down,
-		 * so parents of a hotplug port must stay awake. In addition,
 		 * Hotplug ports handled by firmware in System Management Mode
 		 * may not be put into D3 by the OS (Thunderbolt on non-Macs).
-		 * For simplicity, disallow in general for now.
 		 */
-		if (bridge->is_hotplug_bridge)
+		if (bridge->is_hotplug_bridge && !pciehp_is_native(bridge))
 			return false;
 
 		if (pci_bridge_d3_force)
@@ -2283,6 +2280,10 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 	if (!pci_power_manageable(dev))
 		*d3cold_ok = false;
 
+	/* Hotplug interrupts cannot be delivered if the link is down. */
+	if (dev->is_hotplug_bridge)
+		*d3cold_ok = false;
+
 	return !*d3cold_ok;
 }
 
-- 
2.9.3

[PATCH 4/9] PCI: Activate runtime PM on a PCIe port only if it can suspend

[Lukas Wunner]

Currently pcie_portdrv_probe() activates runtime PM on a PCIe port even
if it will never actually suspend because the BIOS is too old or the
"pcie_port_pm=off" option was specified on the kernel command line.

A few CPU cycles can be saved by not activating runtime PM at all in
these cases, because rpm_idle() and rpm_suspend() will bail out right
at the beginning when calling rpm_check_suspend_allowed(), instead of
carrying out various locking and assignments, invoking rpm_callback(),
getting back -EBUSY and rolling everything back.

The conditions checked in pci_bridge_d3_possible() are all static, they
never change during uptime of the system, hence it's safe to call this
to determine if runtime PM should be activated.

No functional change intended.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c              | 2 +-
 drivers/pci/pci.h              | 1 +
 drivers/pci/pcie/portdrv_pci.c | 5 +++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bc60f2c..854d185 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2226,7 +2226,7 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)
  * This function checks if it is possible to move the bridge to D3.
  * Currently we only allow D3 for recent enough PCIe ports.
  */
-static bool pci_bridge_d3_possible(struct pci_dev *bridge)
+bool pci_bridge_d3_possible(struct pci_dev *bridge)
 {
 	unsigned int year;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 27048bb..ffffef3 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -85,6 +85,7 @@ void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_allocate_cap_save_buffers(struct pci_dev *dev);
 void pci_free_cap_save_buffers(struct pci_dev *dev);
+bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 79327cc..1ae712c 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -19,6 +19,7 @@
 #include <linux/dmi.h>
 #include <linux/pci-aspm.h>
 
+#include "../pci.h"
 #include "portdrv.h"
 #include "aer/aerdrv.h"
 
@@ -157,7 +158,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	 * subordinate devices).  We can't be sure for native PCIe hotplug
 	 * either so prevent that as well.
 	 */
-	if (!dev->is_hotplug_bridge) {
+	if (pci_bridge_d3_possible(dev) && !dev->is_hotplug_bridge) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -175,7 +176,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (!dev->is_hotplug_bridge) {
+	if (pci_bridge_d3_possible(dev) && !dev->is_hotplug_bridge) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
-- 
2.9.3

[PATCH 8/9] ACPI / hotplug / PCI: Make device_is_managed_by_native_pciehp() public

[Lukas Wunner]

We're about to add runtime PM of hotplug ports, but we need to restrict
it to ports that are handled natively by the OS:  If they're handled by
the firmware (which is the case for Thunderbolt on non-Macs), things
would break if the OS put the ports into D3hot behind the firmware's
back.

To determine if a hotplug port is handled natively, one has to walk up
from the port to the root bridge and check the cached _OSC Control Field
for the value of the "PCI Express Native Hot Plug control" bit.  There's
already a function to do that, device_is_managed_by_native_pciehp(),
but it's private to drivers/pci/hotplug/acpiphp_glue.c and only compiled
in if CONFIG_HOTPLUG_PCI_ACPI is enabled.

Make it public and move it to drivers/pci/pci-acpi.c, so that it is
available in the more general CONFIG_ACPI case.

The function contains a check if the device in question is a hotplug
port and returns false if it's not.  The caller we're going to add
doesn't need this as it only calls the function if it actually *is* a
hotplug port.  Move the check out of the function into the single
existing caller.

Give the function a shorter name and add some kerneldoc and polish.

No functional change intended.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/acpiphp_glue.c | 28 +---------------------------
 drivers/pci/pci-acpi.c             | 24 ++++++++++++++++++++++++
 include/linux/pci_hotplug.h        |  2 ++
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b286a56..5ed2dca 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -222,32 +222,6 @@ static void acpiphp_post_dock_fixup(struct acpi_device *adev)
 	acpiphp_let_context_go(context);
 }
 
-/* Check whether the PCI device is managed by native PCIe hotplug driver */
-static bool device_is_managed_by_native_pciehp(struct pci_dev *pdev)
-{
-	acpi_handle tmp;
-	struct acpi_pci_root *root;
-
-	/* Check whether the PCIe port supports native PCIe hotplug */
-	if (!pdev->is_hotplug_bridge)
-		return false;
-
-	/*
-	 * Check whether native PCIe hotplug has been enabled for
-	 * this PCIe hierarchy.
-	 */
-	tmp = acpi_find_root_bridge_handle(pdev);
-	if (!tmp)
-		return false;
-	root = acpi_pci_find_root(tmp);
-	if (!root)
-		return false;
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
-		return false;
-
-	return true;
-}
-
 /**
  * acpiphp_add_context - Add ACPIPHP context to an ACPI device object.
  * @handle: ACPI handle of the object to add a context to.
@@ -331,7 +305,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
 	 * expose slots to user space in those cases.
 	 */
 	if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
-	    && !(pdev && device_is_managed_by_native_pciehp(pdev))) {
+	    && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
 		unsigned long long sun;
 		int retval;
 
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index d966d47..ed3daa8 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -294,6 +294,30 @@ int pci_get_hp_params(struct pci_dev *dev, struct hotplug_params *hpp)
 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
+ *
+ * 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.
+ */
+bool pciehp_is_native(struct pci_dev *pdev)
+{
+	struct acpi_pci_root *root;
+	acpi_handle handle;
+
+	handle = acpi_find_root_bridge_handle(pdev);
+	if (!handle)
+		return false;
+
+	root = acpi_pci_find_root(handle);
+	if (!root)
+		return false;
+
+	return root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL;
+}
+
+/**
  * pci_acpi_wake_bus - Root bus wakeup notification fork function.
  * @work: Work item to handle.
  */
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 8c78950..2e855af 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -176,6 +176,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);
 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);
@@ -185,5 +186,6 @@ 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; }
 #endif
 #endif
-- 
2.9.3

[PATCH 5/9] PCI: Consolidate conditions to allow runtime PM on PCIe ports

[Lukas Wunner]

The conditions to allow runtime PM on PCIe ports are currently spread
across two different files:  The condition relating to hotplug ports is
located in portdrv_pci.c whereas all other conditions are located in
pci.c.

Consolidate all conditions in a single place in pci.c, thus making it
easier to follow the logic and amend conditions down the road.

Note that the condition relating to hotplug ports is inserted *before*
the condition relating to the "pcie_port_pm=force" command line option,
so runtime PM is not afforded to hotplug ports even if this option is
given.  That's exactly how the code behaved up until now.  If this is
not desired, the ordering of the conditions can simply be reversed.

No functional change intended.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c              | 11 +++++++++++
 drivers/pci/pcie/portdrv_pci.c | 12 ++----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 854d185..a19056e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2239,6 +2239,17 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 	case PCI_EXP_TYPE_DOWNSTREAM:
 		if (pci_bridge_d3_disable)
 			return false;
+
+		/*
+		 * Hotplug interrupts cannot be delivered if the link is down,
+		 * so parents of a hotplug port must stay awake. In addition,
+		 * Hotplug ports handled by firmware in System Management Mode
+		 * may not be put into D3 by the OS (Thunderbolt on non-Macs).
+		 * For simplicity, disallow in general for now.
+		 */
+		if (bridge->is_hotplug_bridge)
+			return false;
+
 		if (pci_bridge_d3_force)
 			return true;
 
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 1ae712c..8aa3f14 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -150,15 +150,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 	pci_save_state(dev);
 
-	/*
-	 * Prevent runtime PM if the port is advertising support for PCIe
-	 * hotplug.  Otherwise the BIOS hotplug SMI code might not be able
-	 * to enumerate devices behind this port properly (the port is
-	 * powered down preventing all config space accesses to the
-	 * subordinate devices).  We can't be sure for native PCIe hotplug
-	 * either so prevent that as well.
-	 */
-	if (pci_bridge_d3_possible(dev) && !dev->is_hotplug_bridge) {
+	if (pci_bridge_d3_possible(dev)) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -176,7 +168,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (pci_bridge_d3_possible(dev) && !dev->is_hotplug_bridge) {
+	if (pci_bridge_d3_possible(dev)) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
-- 
2.9.3

[PATCH 6/9] PCI: Unfold conditions to block runtime PM on PCIe ports

[Lukas Wunner]

The conditions to block D3 on parent ports are currently condensed into
a single expression in pci_dev_check_d3cold().  Upcoming commits will
add further conditions for hotplug ports, making this expression fairly
large and impenetrable.  Unfold the conditions to maintain readability
when they are amended.

No functional change intended.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a19056e..f1ddb6b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2270,19 +2270,20 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 {
 	bool *d3cold_ok = data;
-	bool no_d3cold;
 
-	/*
-	 * The device needs to be allowed to go D3cold and if it is wake
-	 * capable to do so from D3cold.
-	 */
-	no_d3cold = dev->no_d3cold || !dev->d3cold_allowed ||
-		(device_may_wakeup(&dev->dev) && !pci_pme_capable(dev, PCI_D3cold)) ||
-		!pci_power_manageable(dev);
+	/* The device needs to be allowed to go D3cold. */
+	if (dev->no_d3cold || !dev->d3cold_allowed)
+		*d3cold_ok = false;
+
+	/* If it is wakeup capable it must be able to do so from D3cold. */
+	if (device_may_wakeup(&dev->dev) && !pci_pme_capable(dev, PCI_D3cold))
+		*d3cold_ok = false;
 
-	*d3cold_ok = !no_d3cold;
+	/* If it is a bridge it must be allowed to go to D3. */
+	if (!pci_power_manageable(dev))
+		*d3cold_ok = false;
 
-	return no_d3cold;
+	return !*d3cold_ok;
 }
 
 /*
-- 
2.9.3

Re: [PATCH 6/9] PCI: Unfold conditions to block runtime PM on PCIe ports

[Mika Westerberg]

On Wed, Oct 19, 2016 at 04:07:13PM +0200, Lukas Wunner wrote:
> The conditions to block D3 on parent ports are currently condensed into
> a single expression in pci_dev_check_d3cold().  Upcoming commits will
> add further conditions for hotplug ports, making this expression fairly
> large and impenetrable.  Unfold the conditions to maintain readability
> when they are amended.
> 
> No functional change intended.

This actually results a functional change because now all the conditions
are evaluated whereas previosly it bailed out immediately when the
condition was true ;-)

Not that it matters here.

> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a19056e..f1ddb6b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2270,19 +2270,20 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
>  {
>  	bool *d3cold_ok = data;
> -	bool no_d3cold;
>  
> -	/*
> -	 * The device needs to be allowed to go D3cold and if it is wake
> -	 * capable to do so from D3cold.
> -	 */
> -	no_d3cold = dev->no_d3cold || !dev->d3cold_allowed ||
> -		(device_may_wakeup(&dev->dev) && !pci_pme_capable(dev, PCI_D3cold)) ||
> -		!pci_power_manageable(dev);
> +	/* The device needs to be allowed to go D3cold. */
> +	if (dev->no_d3cold || !dev->d3cold_allowed)
> +		*d3cold_ok = false;
> +
> +	/* If it is wakeup capable it must be able to do so from D3cold. */
> +	if (device_may_wakeup(&dev->dev) && !pci_pme_capable(dev, PCI_D3cold))
> +		*d3cold_ok = false;
>  
> -	*d3cold_ok = !no_d3cold;
> +	/* If it is a bridge it must be allowed to go to D3. */
> +	if (!pci_power_manageable(dev))
> +		*d3cold_ok = false;
>  
> -	return no_d3cold;
> +	return !*d3cold_ok;
>  }
>  
>  /*
> -- 
> 2.9.3

Re: [PATCH 6/9] PCI: Unfold conditions to block runtime PM on PCIe ports

[Lukas Wunner]

On Thu, Oct 20, 2016 at 05:18:10PM +0300, Mika Westerberg wrote:
> On Wed, Oct 19, 2016 at 04:07:13PM +0200, Lukas Wunner wrote:
> > The conditions to block D3 on parent ports are currently condensed into
> > a single expression in pci_dev_check_d3cold().  Upcoming commits will
> > add further conditions for hotplug ports, making this expression fairly
> > large and impenetrable.  Unfold the conditions to maintain readability
> > when they are amended.
> > 
> > No functional change intended.
> 
> This actually results a functional change because now all the conditions
> are evaluated whereas previosly it bailed out immediately when the
> condition was true ;-)

You're right. That's a step backward, good catch.

I've reworked this commit to restore the old behaviour and will respin
after some more testing and comparing disassembler output.

Thanks!

Lukas

> 
> Not that it matters here.
> 
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  drivers/pci/pci.c | 21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index a19056e..f1ddb6b 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2270,19 +2270,20 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
> >  static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
> >  {
> >  	bool *d3cold_ok = data;
> > -	bool no_d3cold;
> >  
> > -	/*
> > -	 * The device needs to be allowed to go D3cold and if it is wake
> > -	 * capable to do so from D3cold.
> > -	 */
> > -	no_d3cold = dev->no_d3cold || !dev->d3cold_allowed ||
> > -		(device_may_wakeup(&dev->dev) && !pci_pme_capable(dev, PCI_D3cold)) ||
> > -		!pci_power_manageable(dev);
> > +	/* The device needs to be allowed to go D3cold. */
> > +	if (dev->no_d3cold || !dev->d3cold_allowed)
> > +		*d3cold_ok = false;
> > +
> > +	/* If it is wakeup capable it must be able to do so from D3cold. */
> > +	if (device_may_wakeup(&dev->dev) && !pci_pme_capable(dev, PCI_D3cold))
> > +		*d3cold_ok = false;
> >  
> > -	*d3cold_ok = !no_d3cold;
> > +	/* If it is a bridge it must be allowed to go to D3. */
> > +	if (!pci_power_manageable(dev))
> > +		*d3cold_ok = false;
> >  
> > -	return no_d3cold;
> > +	return !*d3cold_ok;
> >  }
> >  
> >  /*
> > -- 
> > 2.9.3

[PATCH 7/9] ACPI / hotplug / PCI: Use cached copy of PCI_EXP_SLTCAP_HPC bit

[Lukas Wunner]

We cache the PCI_EXP_SLTCAP_HPC bit in pci_dev->is_hotplug_bridge on
device probe, so there's no need to read it again when adding the ACPI
hotplug context.

Here's the call chain to prove that no ordering issue is introduced:

pci_scan_child_bus [drivers/pci/probe.c]
  pci_scan_slot
    pci_scan_single_device
      pci_scan_device
        pci_setup_device
          set_pcie_hotplug_bridge
            [is_hotplug_bridge bit is set here]
  pci_scan_bridge
    pci_add_new_bus
      pci_alloc_child_bus
        pcibios_add_bus  [arch/(x86|arm64|ia64)/...]
          acpi_pci_add_bus [drivers/pci/pci-acpi.c]
            acpiphp_enumerate_slots [drivers/pci/hotplug/acpiphp_glue.c]
              acpiphp_add_context
                device_is_managed_by_native_pciehp
                  [is_hotplug_bridge bit is queried here]

No functional change intended.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/acpiphp_glue.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index a46b585..b286a56 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -225,14 +225,11 @@ static void acpiphp_post_dock_fixup(struct acpi_device *adev)
 /* Check whether the PCI device is managed by native PCIe hotplug driver */
 static bool device_is_managed_by_native_pciehp(struct pci_dev *pdev)
 {
-	u32 reg32;
 	acpi_handle tmp;
 	struct acpi_pci_root *root;
 
 	/* Check whether the PCIe port supports native PCIe hotplug */
-	if (pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &reg32))
-		return false;
-	if (!(reg32 & PCI_EXP_SLTCAP_HPC))
+	if (!pdev->is_hotplug_bridge)
 		return false;
 
 	/*
-- 
2.9.3

[PATCH 3/9] PCI: Speed up algorithm in pci_bridge_d3_update()

[Lukas Wunner]

After a device has been added, removed or had its D3cold attributes
changed, we recheck whether its parent bridge may runtime suspend to
D3hot with pci_bridge_d3_update().

The most naive algorithm would be to iterate over the bridge's
children and check if any of them are blocking D3.

The function already tries to be a bit smarter than that by first
checking the device that was changed.  If this device already blocks D3
on the bridge, then walking over all the other children can be skipped.
A drawback of this approach is that if the device is *not* blocking D3,
it will be checked a second time by pci_walk_bus().  But that's cheap
and is outweighed by the performance gain of potentially skipping
pci_walk_bus() altogether.

The algorithm can be optimized further by taking into account if D3 is
currently allowed for the bridge, as shown in the following truth table:

(a)  remove &&  bridge_d3:  D3 is currently allowed for the bridge and
                            removing one of its children won't change
                            that.  No action necessary.
(b)  remove && !bridge_d3:  D3 may now be allowed for the bridge if the
                            removed child was the only one blocking it.
                            Check all its siblings to verify that.
(c) !remove &&  bridge_d3:  D3 may now be disallowed but this can only
                            be caused by the added/changed child, not
                            any of its siblings.  Check only that single
                            device.
(d) !remove && !bridge_d3:  D3 may now be allowed for the bridge if the
                            changed child was the only one blocking it.
                            Check all its siblings to verify that.
                            By checking beforehand if the changed child
                            is blocking D3, we may be able to skip
                            checking its siblings.

Currently we do not special-case option (a) and in case of option (c) we
gratuitously call pci_walk_bus().  Speed up the algorithm by adding
these optimizations.  Reword the comments a bit in an attempt to improve
clarity.

No functional change intended.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c12fae7..bc60f2c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2293,20 +2293,32 @@ void pci_bridge_d3_update(struct pci_dev *dev)
 		return;
 
 	/*
-	 * If the device is removed we do not care about its D3cold
-	 * capabilities.
+	 * If D3 is currently allowed for the bridge, removing one of its
+	 * children won't change that.
+	 */
+	if (remove && bridge->bridge_d3)
+		return;
+
+	/*
+	 * If D3 is currently allowed for the bridge and a child is added or
+	 * changed, disallowance of D3 can only be caused by that child, so
+	 * we only need to check that single device, not any of its siblings.
+	 *
+	 * If D3 is currently not allowed for the bridge, checking the device
+	 * first may allow us to skip checking its siblings.
 	 */
 	if (!remove)
 		pci_dev_check_d3cold(dev, &d3cold_ok);
 
-	if (d3cold_ok) {
-		/*
-		 * We need to go through all children to find out if all of
-		 * them can still go to D3cold.
-		 */
+	/*
+	 * If D3 is currently not allowed for the bridge, this may be caused
+	 * either by the device being changed/removed or any of its siblings,
+	 * so we need to go through all children to find out if one of them
+	 * continues to block D3.
+	 */
+	if (d3cold_ok && !bridge->bridge_d3)
 		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
 			     &d3cold_ok);
-	}
 
 	if (bridge->bridge_d3 != d3cold_ok) {
 		bridge->bridge_d3 = d3cold_ok;
-- 
2.9.3

[PATCH 2/9] PCI: Autosense device removal in pci_bridge_d3_update()

[Lukas Wunner]

The algorithm to update the flag indicating whether a bridge may go to
D3 makes a few optimizations based on whether the update was caused by
the removal of a device on the one hand, versus the addition of a device
or the change of its D3cold flags on the other hand.

The information whether the update pertains to a removal is currently
passed in by the caller, but the function may as well determine that
itself by examining the device in question, thereby allowing for a
considerable simplification and reduction of the code.

Out of several options to determine removal, I've chosen the function
device_is_registered() because it's cheap:  It merely returns the
dev->kobj.state_in_sysfs flag.  That flag is set through device_add()
when the root bus is scanned and cleared through device_remove().
The call to pci_bridge_d3_update() happens after each of these calls,
respectively, so the ordering is correct.

No functional change intended.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/bus.c    |  2 +-
 drivers/pci/pci.c    | 35 +++++------------------------------
 drivers/pci/pci.h    |  3 +--
 drivers/pci/remove.c |  2 +-
 4 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index c288e5a..bc56cf1 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -320,7 +320,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_fixup_device(pci_fixup_final, dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
-	pci_bridge_d3_device_changed(dev);
+	pci_bridge_d3_update(dev);
 
 	dev->match_driver = true;
 	retval = device_attach(&dev->dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6e0c399..c12fae7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2277,14 +2277,14 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 /*
  * pci_bridge_d3_update - Update bridge D3 capabilities
  * @dev: PCI device which is changed
- * @remove: Is the device being removed
  *
  * Update upstream bridge PM capabilities accordingly depending on if the
  * device PM configuration was changed or the device is being removed.  The
  * change is also propagated upstream.
  */
-static void pci_bridge_d3_update(struct pci_dev *dev, bool remove)
+void pci_bridge_d3_update(struct pci_dev *dev)
 {
+	bool remove = !device_is_registered(&dev->dev);
 	struct pci_dev *bridge;
 	bool d3cold_ok = true;
 
@@ -2311,36 +2311,11 @@ static void pci_bridge_d3_update(struct pci_dev *dev, bool remove)
 	if (bridge->bridge_d3 != d3cold_ok) {
 		bridge->bridge_d3 = d3cold_ok;
 		/* Propagate change to upstream bridges */
-		pci_bridge_d3_update(bridge, false);
+		pci_bridge_d3_update(bridge);
 	}
 }
 
 /**
- * pci_bridge_d3_device_changed - Update bridge D3 capabilities on change
- * @dev: PCI device that was changed
- *
- * If a device is added or its PM configuration, such as is it allowed to
- * enter D3cold, is changed this function updates upstream bridge PM
- * capabilities accordingly.
- */
-void pci_bridge_d3_device_changed(struct pci_dev *dev)
-{
-	pci_bridge_d3_update(dev, false);
-}
-
-/**
- * pci_bridge_d3_device_removed - Update bridge D3 capabilities on remove
- * @dev: PCI device being removed
- *
- * Function updates upstream bridge PM capabilities based on other devices
- * still left on the bus.
- */
-void pci_bridge_d3_device_removed(struct pci_dev *dev)
-{
-	pci_bridge_d3_update(dev, true);
-}
-
-/**
  * pci_d3cold_enable - Enable D3cold for device
  * @dev: PCI device to handle
  *
@@ -2352,7 +2327,7 @@ void pci_d3cold_enable(struct pci_dev *dev)
 {
 	if (dev->no_d3cold) {
 		dev->no_d3cold = false;
-		pci_bridge_d3_device_changed(dev);
+		pci_bridge_d3_update(dev);
 	}
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_enable);
@@ -2369,7 +2344,7 @@ void pci_d3cold_disable(struct pci_dev *dev)
 {
 	if (!dev->no_d3cold) {
 		dev->no_d3cold = true;
-		pci_bridge_d3_device_changed(dev);
+		pci_bridge_d3_update(dev);
 	}
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_disable);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4518562..27048bb 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -85,8 +85,7 @@ void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_allocate_cap_save_buffers(struct pci_dev *dev);
 void pci_free_cap_save_buffers(struct pci_dev *dev);
-void pci_bridge_d3_device_changed(struct pci_dev *dev);
-void pci_bridge_d3_device_removed(struct pci_dev *dev);
+void pci_bridge_d3_update(struct pci_dev *dev);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index f9357e0..73a03d3 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -40,7 +40,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 	list_del(&dev->bus_list);
 	up_write(&pci_bus_sem);
 
-	pci_bridge_d3_device_removed(dev);
+	pci_bridge_d3_update(dev);
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }
-- 
2.9.3

Re: [PATCH 0/9] PCIe port PM: high gloss polish & hotplug support

[Mika Westerberg]

On Wed, Oct 19, 2016 at 04:07:13PM +0200, Lukas Wunner wrote:
> Linux v4.8 added support to runtime suspend PCIe ports to D3hot.

For some reason the patches arrived to my inbox in wrong order.

> Patches 1 to 6 of this series apply high gloss polish to that code to
> speed it up, reduce its size and improve its readability.  None of these
> patches should result in a functional change.  (And they didn't in my
> tests.)
> 
> Patches 7 and 8 are cleanups / refactorings for acpiphp.  They're prep
> work for patch 9 and likewise shouldn't cause a functional change.

I tested the patches 1-8 on a system with pretty recent thunderbolt
hardware, using ACPI based hotplug and it still works fine (meaning the
port is stays runtime active).

Also I can verify the ports that can suspend on my test systems still
suspend as expected.

So for patches 1-8 you can add my,

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Nice work!

Re: [PATCH 0/9] PCIe port PM: high gloss polish & hotplug support

[Rafael J. Wysocki]

On Wed, Oct 19, 2016 at 4:07 PM, Lukas Wunner <lukas@wunner.de> wrote:
> Linux v4.8 added support to runtime suspend PCIe ports to D3hot.
>
> Patches 1 to 6 of this series apply high gloss polish to that code to
> speed it up, reduce its size and improve its readability.  None of these
> patches should result in a functional change.  (And they didn't in my
> tests.)
>
> Patches 7 and 8 are cleanups / refactorings for acpiphp.  They're prep
> work for patch 9 and likewise shouldn't cause a functional change.
>
> Patch 9 extends runtime PM support to hotplug ports.
>
>
> @Keith Busch: I seem to recall you're working with large arrays of NVMe
> drives connected to native PCIe hotplug ports.  Perhaps you could give
> this series a spin, it might save quite a bit of power.
>
> To ease reviewing I've pushed this series to GitHub:
> https://github.com/l1k/linux/commits/pcie_port_pm_v1
>
> Thanks,
>
> Lukas
>
>
> Lukas Wunner (9):
>   PCI: Don't acquire ref on parent in pci_bridge_d3_update()
>   PCI: Autosense device removal in pci_bridge_d3_update()
>   PCI: Speed up algorithm in pci_bridge_d3_update()
>   PCI: Activate runtime PM on a PCIe port only if it can suspend
>   PCI: Consolidate conditions to allow runtime PM on PCIe ports
>   PCI: Unfold conditions to block runtime PM on PCIe ports
>   ACPI / hotplug / PCI: Use cached copy of PCI_EXP_SLTCAP_HPC bit
>   ACPI / hotplug / PCI: Make device_is_managed_by_native_pciehp() public
>   PCI: pciehp: Add runtime PM support for PCIe hotplug ports

For the whole series:

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

and I'm assuming that it will go in through the PCI tree.

Thanks,
Rafael