[PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[Mika Westerberg]

Current Linux PCI core does not do any kind of power management to PCIe
ports. This means that we waste energy and consume laptop battery even if
the port has nothing connected to. These patches aim to change that to the
right direction.

Previous versions of the patches can be found below:

  v1: http://www.spinics.net/lists/linux-pci/msg49313.html
  v2: http://www.spinics.net/lists/linux-pci/msg50167.html
  v3: http://www.spinics.net/lists/linux-pci/msg50345.html
  v4: http://www.spinics.net/lists/linux-pci/msg50665.html

This assumes that recent (starting from 2015) PCIe ports are capable of
transition to D3hot/D3cold. We add a new flag to struct pci_dev 'bridge_d3'
that is set whenever the PCI core thinks the port can be put to D3. The
check in pci_pm_suspend_noirq() is then extended to cover devices where
'bridge_d3' is set.

We then add two new functions pci_bridge_d3_device_changed/removed(). These
are used to set and clear 'bridge_d3' whenever there is a change in device
power management policy (or if the device is removed). For example when
userspace forbids the device to enter D3cold pci_bridge_d3_device_changed()
will clear 'bridge_d3' of the upstream bridge.

For all PCI ports where 'bridge_d3' is set we also enable and unblock
runtime PM automatically. Only exception is when the PCIe port claims to
support hotplug. More information about that is in the changelog of
patch [4/4].

Since this also touches xhci, I'm adding Mathias and Greg to check if the
change looks reasonable.

Changes to v4:

  - Use "Put PCIe ports into D3" instead of "move" in subject and comments.

  - Add new helper function pci_power_manageable() and use it to check if
    the port can be suspended instead of open-coding it everytime.

  - Changed pci_dev_check_d3cold() to follow what Rafael suggested.

  - Dropped call to pm_runtime_mark_last_busy() in pcie_port_runtime_resume()
    as PM core does that automatically when the function returns 0.

  - Added ACKs from Rafael.

Changes to v3:

  - Make 'no_d3cold' persist for pci_dev. Once this is set through
    pci_d3cold_enable/disable(), it will persist unless changed again using
    the same functions. Those also handle setting and clearing 'bridge_d3'
    as necessary.

  - Add kernel command line parameter "pcie_port_pm=off|force" that can
    be used to forcibly disable/enable the feature.

  - Use runtime PM autosuspend instead of scheduling runtime suspend
    manually

  - Allow runtime PM always except when we are dealing with a hotplug
    bridge. Actual checking whether the port can be suspended is done in
    ->idle() and ->runtime_suspend() hooks based on 'bridge_d3'.

Changes to v2:

  - Renamed and split pci_enable_d3cold() into two functions
    pci_d3cold_enable()/disable().

  - Renamed pci_bridge_pm_update() into two functions
    pci_bridge_d3_device_changed() and pci_bridge_d3_device_removed() that
    should match better what they are doing.

  - Propagate ->bridge_d3 change to upstream bridges in
    pci_bridge_d3_update().

  - Removed pci_can_suspend() in favor of doing ->bridge_d3 check directly
    in pci_pm_suspend_noirq().

  - Extend runtime PM enabling for ports that are using native PCIe
    hotplug.

  - Call pm_runtime_no_callbacks() for PCIe port service devices (the are
    handled by the parent device).

Changes to v1:

  - Dropped patch [2/6] as there is no need to use that function from other
    files anymore.

  - Dropped patches [5-6/6] in favor of using cut-off date.

  - Updated changelog of [1/4] to mention where in the PCI core PCI bridge
    and PCIe ports are skipped from being power managed.

  - Instead of checking at suspend time if it is possible to transition the
    port to D3, do it whenever power management status of a device (below a
    port) is changed or when it is added or removed to the bus.

  - Added patch [3/4] to runtime resume a bridge when ACPI hotplug event is
    received.

Mika Westerberg (4):
  PCI: No need to set d3cold_allowed to PCIe ports
  PCI: Put PCIe ports into D3 during suspend
  ACPI / hotplug / PCI: Runtime resume bridge before rescan
  PCI: Add runtime PM support for PCIe ports

 Documentation/kernel-parameters.txt |   4 +
 drivers/pci/bus.c                   |   1 +
 drivers/pci/hotplug/acpiphp_glue.c  |   8 +-
 drivers/pci/pci-driver.c            |   5 +-
 drivers/pci/pci-sysfs.c             |   5 ++
 drivers/pci/pci.c                   | 175 ++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  11 +++
 drivers/pci/pcie/portdrv_core.c     |   2 +
 drivers/pci/pcie/portdrv_pci.c      |  51 ++++++++++-
 drivers/pci/remove.c                |   2 +
 drivers/usb/host/xhci-pci.c         |   2 +-
 include/linux/pci.h                 |   3 +
 12 files changed, 259 insertions(+), 10 deletions(-)

-- 
2.8.0.rc3

[PATCH v5 1/4] PCI: No need to set d3cold_allowed to PCIe ports

[Mika Westerberg]

The Linux PCI core skips PCI bridges and PCIe ports when system is
suspended. The PCI core checks return value of pci_has_subordinate() in
pci_pm_suspend_noirq() to skip all devices where it is non-zero (which
means PCI bridges and PCIe ports).

Since PCIe ports are never suspended in the first place, there is no need
to set d3cold_allowed for them.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pcie/portdrv_pci.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index be35da2e105e..6c6bb03392ea 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -134,11 +134,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	pci_save_state(dev);
-	/*
-	 * D3cold may not work properly on some PCIe port, so disable
-	 * it by default.
-	 */
-	dev->d3cold_allowed = false;
 	return 0;
 }
 
-- 
2.8.0.rc3

[PATCH v5 2/4] PCI: Put PCIe ports into D3 during suspend

[Mika Westerberg]

Currently the Linux PCI core does not touch power state of PCI bridges and
PCIe ports when system suspend is entered. Leaving them in D0 consumes
power which is not good thing in portable devices such as laptops. This may
also prevent the CPU from entering deeper C-states.

With recent PCIe hardware we can power down the ports to save power given
that we take into account few restrictions:

  - The PCIe port hardware is recent enough, starting from 2015.

  - Devices connected to PCIe ports are effectively in D3cold once the port
    is transitioned to D3 (the config space is not accessible anymore and
    the link may be powered down).

  - Devices behind the PCIe port need to be allowed to transition to D3cold
    and back. There is a way both drivers and userspace can forbid this.

  - If the device behind the PCIe port is capable of waking the system it
    needs to be able to do so from D3cold.

This patch adds a new flag to struct pci_device called 'bridge_d3'. This
flag is set and cleared by the PCI core whenever there is a change in power
management state of any of the devices behind the PCIe port. When system
later on is suspended we only need to check this flag and if it is true
transition the port to D3 otherwise we leave it in D0.

Also provide override mechanism via command line parameter
"pcie_port_pm=[off|force]" that can be used to disable or enable the
feature regardless of the BIOS manufacturing date.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/kernel-parameters.txt |   4 +
 drivers/pci/bus.c                   |   1 +
 drivers/pci/pci-driver.c            |   5 +-
 drivers/pci/pci-sysfs.c             |   5 ++
 drivers/pci/pci.c                   | 175 ++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h                   |  11 +++
 drivers/pci/remove.c                |   2 +
 drivers/usb/host/xhci-pci.c         |   2 +-
 include/linux/pci.h                 |   3 +
 9 files changed, 203 insertions(+), 5 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 0b3de80ec8f6..90e1f43760be 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3008,6 +3008,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		compat	Treat PCIe ports as PCI-to-PCI bridges, disable the PCIe
 			ports driver.
 
+	pcie_port_pm=	[PCIE] PCIe port power management handling:
+		off	Disable power management of all PCIe ports
+		force	Forcibly enable power management of all PCIe ports
+
 	pcie_pme=	[PCIE,PM] Native PCIe PME signaling options:
 		nomsi	Do not use MSI for native PCIe PME signaling (this makes
 			all PCIe root ports use INTx for all services).
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 6c9f5467bc5f..b1738162d69a 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -291,6 +291,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);
 
 	dev->match_driver = true;
 	retval = device_attach(&dev->dev);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d7ffd66814bb..e39a67c8ef39 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -777,7 +777,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
 
 	if (!pci_dev->state_saved) {
 		pci_save_state(pci_dev);
-		if (!pci_has_subordinate(pci_dev))
+		if (pci_power_manageable(pci_dev))
 			pci_prepare_to_sleep(pci_dev);
 	}
 
@@ -1144,7 +1144,6 @@ static int pci_pm_runtime_suspend(struct device *dev)
 		return -ENOSYS;
 
 	pci_dev->state_saved = false;
-	pci_dev->no_d3cold = false;
 	error = pm->runtime_suspend(dev);
 	if (error) {
 		/*
@@ -1161,8 +1160,6 @@ static int pci_pm_runtime_suspend(struct device *dev)
 
 		return error;
 	}
-	if (!pci_dev->d3cold_allowed)
-		pci_dev->no_d3cold = true;
 
 	pci_fixup_device(pci_fixup_suspend, pci_dev);
 
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 342b6918bbde..1a0854be5b18 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -406,6 +406,11 @@ static ssize_t d3cold_allowed_store(struct device *dev,
 		return -EINVAL;
 
 	pdev->d3cold_allowed = !!val;
+	if (pdev->d3cold_allowed)
+		pci_d3cold_enable(pdev);
+	else
+		pci_d3cold_disable(pdev);
+
 	pm_runtime_resume(dev);
 
 	return count;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 25e0327d4429..872a0b9a0c79 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -9,6 +9,7 @@
 
 #include <linux/kernel.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/of_pci.h>
@@ -101,6 +102,21 @@ unsigned int pcibios_max_latency = 255;
 /* If set, the PCIe ARI capability will not be used. */
 static bool pcie_ari_disabled;
 
+/* Disable bridge_d3 for all PCIe ports */
+static bool pci_bridge_d3_disable;
+/* Force bridge_d3 for all PCIe ports */
+static bool pci_bridge_d3_force;
+
+static int __init pcie_port_pm_setup(char *str)
+{
+	if (!strcmp(str, "off"))
+		pci_bridge_d3_disable = true;
+	else if (!strcmp(str, "force"))
+		pci_bridge_d3_force = true;
+	return 1;
+}
+__setup("pcie_port_pm=", pcie_port_pm_setup);
+
 /**
  * pci_bus_max_busnr - returns maximum PCI bus number of given bus' children
  * @bus: pointer to PCI bus structure to search
@@ -2156,6 +2172,164 @@ void pci_config_pm_runtime_put(struct pci_dev *pdev)
 }
 
 /**
+ * pci_bridge_d3_possible - Is it possible to put the bridge into D3
+ * @bridge: Bridge to check
+ *
+ * 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)
+{
+	unsigned int year;
+
+	if (!pci_is_pcie(bridge))
+		return false;
+
+	switch (pci_pcie_type(bridge)) {
+	case PCI_EXP_TYPE_ROOT_PORT:
+	case PCI_EXP_TYPE_UPSTREAM:
+	case PCI_EXP_TYPE_DOWNSTREAM:
+		if (pci_bridge_d3_disable)
+			return false;
+		if (pci_bridge_d3_force)
+			return true;
+
+		/*
+		 * It should be safe to put PCIe ports from 2015 or newer
+		 * to D3.
+		 */
+		if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) &&
+		    year >= 2015) {
+			return true;
+		}
+		break;
+	}
+
+	return false;
+}
+
+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);
+
+	*d3cold_ok = !no_d3cold;
+
+	return no_d3cold;
+}
+
+/*
+ * pci_bridge_d3_update - Update bridge D3 capabilities
+ * @dev: PCI device which is changed
+ * @remove: Is the device being removed
+ *
+ * Updates 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)
+{
+	struct pci_dev *bridge;
+	bool d3cold_ok = true;
+
+	bridge = pci_upstream_bridge(dev);
+	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.
+	 */
+	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.
+		 */
+		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
+			     &d3cold_ok);
+	}
+
+	if (bridge->bridge_d3 != d3cold_ok) {
+		bridge->bridge_d3 = d3cold_ok;
+		/* Propagate change to upstream bridges */
+		pci_bridge_d3_update(bridge, false);
+	}
+
+	pci_dev_put(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
+ *
+ * This function can be used in drivers to enable D3cold from the device
+ * they handle. It also updates upstream PCI bridge PM capabilities
+ * accordingly.
+ */
+void pci_d3cold_enable(struct pci_dev *dev)
+{
+	if (dev->no_d3cold) {
+		dev->no_d3cold = false;
+		pci_bridge_d3_device_changed(dev);
+	}
+}
+EXPORT_SYMBOL_GPL(pci_d3cold_enable);
+
+/**
+ * pci_d3cold_disable - Disable D3cold for device
+ * @dev: PCI device to handle
+ *
+ * This function can be used in drivers to disable D3cold from the device
+ * they handle. It also updates upstream PCI bridge PM capabilities
+ * accordingly.
+ */
+void pci_d3cold_disable(struct pci_dev *dev)
+{
+	if (!dev->no_d3cold) {
+		dev->no_d3cold = true;
+		pci_bridge_d3_device_changed(dev);
+	}
+}
+EXPORT_SYMBOL_GPL(pci_d3cold_disable);
+
+/**
  * pci_pm_init - Initialize PM functions of given PCI device
  * @dev: PCI device to handle.
  */
@@ -2189,6 +2363,7 @@ void pci_pm_init(struct pci_dev *dev)
 	dev->pm_cap = pm;
 	dev->d3_delay = PCI_PM_D3_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
+	dev->bridge_d3 = pci_bridge_d3_possible(dev);
 	dev->d3cold_allowed = true;
 
 	dev->d1_support = false;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a814bbb80fcb..9730c474b016 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -82,6 +82,8 @@ 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);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {
@@ -94,6 +96,15 @@ static inline bool pci_has_subordinate(struct pci_dev *pci_dev)
 	return !!(pci_dev->subordinate);
 }
 
+static inline bool pci_power_manageable(struct pci_dev *pci_dev)
+{
+	/*
+	 * Currently we allow normal PCI devices and PCI bridges transition
+	 * into D3 if their bridge_d3 is set.
+	 */
+	return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3;
+}
+
 struct pci_vpd_ops {
 	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
 	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8982026637d5..d1ef7acf6930 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -96,6 +96,8 @@ static void pci_remove_bus_device(struct pci_dev *dev)
 		dev->subordinate = NULL;
 	}
 
+	pci_bridge_d3_device_removed(dev);
+
 	pci_destroy_dev(dev);
 }
 
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 48672fac7ff3..ac352fe391f4 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -382,7 +382,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 	 * need to have the registers polled during D3, so avoid D3cold.
 	 */
 	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
-		pdev->no_d3cold = true;
+		pci_d3cold_disable(pdev);
 
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_quirk(hcd);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 932ec74909c6..aa7d586710c0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -296,6 +296,7 @@ struct pci_dev {
 	unsigned int	d2_support:1;	/* Low power state D2 is supported */
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
 	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
+	unsigned int	bridge_d3:1;	/* Allow D3 for bridge */
 	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* disallow turning off io/mem
 						   decoding during bar sizing */
@@ -1085,6 +1086,8 @@ int pci_back_from_sleep(struct pci_dev *dev);
 bool pci_dev_run_wake(struct pci_dev *dev);
 bool pci_check_pme_status(struct pci_dev *dev);
 void pci_pme_wakeup_bus(struct pci_bus *bus);
+void pci_d3cold_enable(struct pci_dev *dev);
+void pci_d3cold_disable(struct pci_dev *dev);
 
 static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 				  bool enable)
-- 
2.8.0.rc3

[PATCH v5 3/4] ACPI / hotplug / PCI: Runtime resume bridge before rescan

[Mika Westerberg]

If a PCI bridge (or PCIe port) that is runtime suspended gets an ACPI
hotplug event, such as BUS_CHECK we need to make sure it is resumed before
devices below the bridge are re-scanned. Otherwise the devices behind the
port are not accessible and will be treated as hot-unplugged.

To fix this, resume PCI bridges from runtime suspend while rescanning.

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

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index fa49f9143b80..d64ce8aa99b3 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -756,8 +756,10 @@ static void hotplug_event(u32 type, struct acpiphp_context *context)
 
 	acpi_lock_hp_context();
 	bridge = context->bridge;
-	if (bridge)
+	if (bridge) {
 		get_bridge(bridge);
+		pm_runtime_get_sync(&bridge->pci_dev->dev);
+	}
 
 	acpi_unlock_hp_context();
 
@@ -797,8 +799,10 @@ static void hotplug_event(u32 type, struct acpiphp_context *context)
 	}
 
 	pci_unlock_rescan_remove();
-	if (bridge)
+	if (bridge) {
+		pm_runtime_put(&bridge->pci_dev->dev);
 		put_bridge(bridge);
+	}
 }
 
 static int acpiphp_hotplug_notify(struct acpi_device *adev, u32 type)
-- 
2.8.0.rc3

[PATCH v5 4/4] PCI: Add runtime PM support for PCIe ports

[Mika Westerberg]

Add back runtime PM support for PCIe ports that was removed in
commit fe9a743a2601 ("PCI/PM: Drop unused runtime PM support code for
PCIe ports").

First of all we cannot enable it automatically for all ports since there
has been problems previously as can be seen in [1]. In summary suspended
PCIe ports were not able to deal with ACPI based hotplug reliably. One
reason why this might happen is the fact that when a PCIe port is powered
down, config space access to the devices behind the port is not possible.
If the BIOS hotplug SMI handler assumes the port is always in D0 it will
not be able to find the hotplugged devices. To be on the safe side only
enable runtime PM if the port does not claim to support hotplug.

For PCIe ports not using hotplug we enable and allow runtime PM
automatically. Since 'bridge_d3' can be changed any time we check this in
driver ->runtime_idle() and ->runtime_suspend() and only allow runtime
suspend if the flag is still set. Use autosuspend with default of 10ms idle
time to prevent the port from repeatedly suspending and resuming on
continuous configuration space access of devices behind the port.

The actual power transition to D3 and back is handled in the PCI core.

Idea to automatically unblock (allow) runtime PM for PCIe ports came from
Dave Airlie.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=53811

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

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 88122dc2e1b1..65b1a624826b 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/pcieport_if.h>
@@ -343,6 +344,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 		     get_descriptor_id(pci_pcie_type(pdev), service));
 	device->parent = &pdev->dev;
 	device_enable_async_suspend(device);
+	pm_runtime_no_callbacks(device);
 
 	retval = device_register(device);
 	if (retval) {
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 6c6bb03392ea..f4409af00756 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -93,6 +93,26 @@ static int pcie_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int pcie_port_runtime_suspend(struct device *dev)
+{
+	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
+}
+
+static int pcie_port_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static int pcie_port_runtime_idle(struct device *dev)
+{
+	/*
+	 * Rely the PCI core has set bridge_d3 whenever it thinks the port
+	 * should be good to go to D3. Everything else, including moving
+	 * the port to D3, is handled by the PCI core.
+	 */
+	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
+}
+
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.suspend	= pcie_port_device_suspend,
 	.resume		= pcie_port_device_resume,
@@ -101,6 +121,9 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
 	.poweroff	= pcie_port_device_suspend,
 	.restore	= pcie_port_device_resume,
 	.resume_noirq	= pcie_port_resume_noirq,
+	.runtime_suspend = pcie_port_runtime_suspend,
+	.runtime_resume	= pcie_port_runtime_resume,
+	.runtime_idle	= pcie_port_runtime_idle,
 };
 
 #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
@@ -134,11 +157,38 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 		return status;
 
 	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 (!dev->is_hotplug_bridge) {
+		/*
+		 * Keep the port resumed 10ms to make sure things like
+		 * config space accesses from userspace (lspci) will not
+		 * cause the port to repeatedly suspend and resume.
+		 */
+		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
+		pm_runtime_use_autosuspend(&dev->dev);
+		pm_runtime_put_autosuspend(&dev->dev);
+		pm_runtime_allow(&dev->dev);
+	}
+
 	return 0;
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
+	if (!dev->is_hotplug_bridge) {
+		pm_runtime_forbid(&dev->dev);
+		pm_runtime_get_noresume(&dev->dev);
+		pm_runtime_dont_use_autosuspend(&dev->dev);
+	}
+
 	pcie_port_device_remove(dev);
 }
 
-- 
2.8.0.rc3

Re: [PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[Mathias Nyman]

On 29.04.2016 11:51, Mika Westerberg wrote:
> Current Linux PCI core does not do any kind of power management to PCIe
> ports. This means that we waste energy and consume laptop battery even if
> the port has nothing connected to. These patches aim to change that to the
> right direction.
>
> Previous versions of the patches can be found below:
>
>    v1: http://www.spinics.net/lists/linux-pci/msg49313.html
>    v2: http://www.spinics.net/lists/linux-pci/msg50167.html
>    v3: http://www.spinics.net/lists/linux-pci/msg50345.html
>    v4: http://www.spinics.net/lists/linux-pci/msg50665.html
>
> This assumes that recent (starting from 2015) PCIe ports are capable of
> transition to D3hot/D3cold. We add a new flag to struct pci_dev 'bridge_d3'
> that is set whenever the PCI core thinks the port can be put to D3. The
> check in pci_pm_suspend_noirq() is then extended to cover devices where
> 'bridge_d3' is set.
>
> We then add two new functions pci_bridge_d3_device_changed/removed(). These
> are used to set and clear 'bridge_d3' whenever there is a change in device
> power management policy (or if the device is removed). For example when
> userspace forbids the device to enter D3cold pci_bridge_d3_device_changed()
> will clear 'bridge_d3' of the upstream bridge.
>
> For all PCI ports where 'bridge_d3' is set we also enable and unblock
> runtime PM automatically. Only exception is when the PCIe port claims to
> support hotplug. More information about that is in the changelog of
> patch [4/4].
>
> Since this also touches xhci, I'm adding Mathias and Greg to check if the
> change looks reasonable.
>

For the non-functional one-line only xhci change in patch 2/4:
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Out of curiosity, does enabling bridge d3 in any way impact #PME
wakeup signaling initiated by devices behind that bridge?

-Mathias

Re: [PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[Rafael J. Wysocki]

On Friday, April 29, 2016 02:46:33 PM Mathias Nyman wrote:
> On 29.04.2016 11:51, Mika Westerberg wrote:
> > Current Linux PCI core does not do any kind of power management to PCIe
> > ports. This means that we waste energy and consume laptop battery even if
> > the port has nothing connected to. These patches aim to change that to the
> > right direction.
> >
> > Previous versions of the patches can be found below:
> >
> >    v1: http://www.spinics.net/lists/linux-pci/msg49313.html
> >    v2: http://www.spinics.net/lists/linux-pci/msg50167.html
> >    v3: http://www.spinics.net/lists/linux-pci/msg50345.html
> >    v4: http://www.spinics.net/lists/linux-pci/msg50665.html
> >
> > This assumes that recent (starting from 2015) PCIe ports are capable of
> > transition to D3hot/D3cold. We add a new flag to struct pci_dev 'bridge_d3'
> > that is set whenever the PCI core thinks the port can be put to D3. The
> > check in pci_pm_suspend_noirq() is then extended to cover devices where
> > 'bridge_d3' is set.
> >
> > We then add two new functions pci_bridge_d3_device_changed/removed(). These
> > are used to set and clear 'bridge_d3' whenever there is a change in device
> > power management policy (or if the device is removed). For example when
> > userspace forbids the device to enter D3cold pci_bridge_d3_device_changed()
> > will clear 'bridge_d3' of the upstream bridge.
> >
> > For all PCI ports where 'bridge_d3' is set we also enable and unblock
> > runtime PM automatically. Only exception is when the PCIe port claims to
> > support hotplug. More information about that is in the changelog of
> > patch [4/4].
> >
> > Since this also touches xhci, I'm adding Mathias and Greg to check if the
> > change looks reasonable.
> >
> 
> For the non-functional one-line only xhci change in patch 2/4:
> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> Out of curiosity, does enabling bridge d3 in any way impact #PME
> wakeup signaling initiated by devices behind that bridge?

If the device reports (via the config space) that it is capable of signaling
PME from D3_{cold}, the wakeup signaling won't be affected (as long as the
hardware design follows the spec).

The Mika's patches take that into account AFAICS.

Thanks,
Rafael

Re: [PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[Mika Westerberg]

On Fri, Apr 29, 2016 at 02:10:59PM +0200, Rafael J. Wysocki wrote:
> > For the non-functional one-line only xhci change in patch 2/4:
> > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > 
> > Out of curiosity, does enabling bridge d3 in any way impact #PME
> > wakeup signaling initiated by devices behind that bridge?
> 
> If the device reports (via the config space) that it is capable of signaling
> PME from D3_{cold}, the wakeup signaling won't be affected (as long as the
> hardware design follows the spec).
> 
> The Mika's patches take that into account AFAICS.

Yes, that's right.

Re: [PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[Lukas Wunner]

Hi,

On Fri, Apr 29, 2016 at 11:51:55AM +0300, Mika Westerberg wrote:
> Current Linux PCI core does not do any kind of power management to PCIe
> ports. This means that we waste energy and consume laptop battery even if
> the port has nothing connected to. These patches aim to change that to the
> right direction.

I've rebased my Thunderbolt runtime pm patches on Mika's v5.
Works for me, so this series is

Tested-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas

Re: [PATCH v5 2/4] PCI: Put PCIe ports into D3 during suspend

[Rafael J. Wysocki]

On Friday, April 29, 2016 11:51:57 AM Mika Westerberg wrote:
> Currently the Linux PCI core does not touch power state of PCI bridges and
> PCIe ports when system suspend is entered. Leaving them in D0 consumes
> power which is not good thing in portable devices such as laptops. This may
> also prevent the CPU from entering deeper C-states.
> 
> With recent PCIe hardware we can power down the ports to save power given
> that we take into account few restrictions:
> 
>   - The PCIe port hardware is recent enough, starting from 2015.
> 
>   - Devices connected to PCIe ports are effectively in D3cold once the port
>     is transitioned to D3 (the config space is not accessible anymore and
>     the link may be powered down).
> 
>   - Devices behind the PCIe port need to be allowed to transition to D3cold
>     and back. There is a way both drivers and userspace can forbid this.
> 
>   - If the device behind the PCIe port is capable of waking the system it
>     needs to be able to do so from D3cold.
> 
> This patch adds a new flag to struct pci_device called 'bridge_d3'. This
> flag is set and cleared by the PCI core whenever there is a change in power
> management state of any of the devices behind the PCIe port. When system
> later on is suspended we only need to check this flag and if it is true
> transition the port to D3 otherwise we leave it in D0.
> 
> Also provide override mechanism via command line parameter
> "pcie_port_pm=[off|force]" that can be used to disable or enable the
> feature regardless of the BIOS manufacturing date.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

Re: [PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[Rafael J. Wysocki]

On Friday, April 29, 2016 11:51:55 AM Mika Westerberg wrote:
> Current Linux PCI core does not do any kind of power management to PCIe
> ports. This means that we waste energy and consume laptop battery even if
> the port has nothing connected to. These patches aim to change that to the
> right direction.
> 
> Previous versions of the patches can be found below:
> 
>   v1: http://www.spinics.net/lists/linux-pci/msg49313.html
>   v2: http://www.spinics.net/lists/linux-pci/msg50167.html
>   v3: http://www.spinics.net/lists/linux-pci/msg50345.html
>   v4: http://www.spinics.net/lists/linux-pci/msg50665.html
> 
> This assumes that recent (starting from 2015) PCIe ports are capable of
> transition to D3hot/D3cold. We add a new flag to struct pci_dev 'bridge_d3'
> that is set whenever the PCI core thinks the port can be put to D3. The
> check in pci_pm_suspend_noirq() is then extended to cover devices where
> 'bridge_d3' is set.
> 
> We then add two new functions pci_bridge_d3_device_changed/removed(). These
> are used to set and clear 'bridge_d3' whenever there is a change in device
> power management policy (or if the device is removed). For example when
> userspace forbids the device to enter D3cold pci_bridge_d3_device_changed()
> will clear 'bridge_d3' of the upstream bridge.
> 
> For all PCI ports where 'bridge_d3' is set we also enable and unblock
> runtime PM automatically. Only exception is when the PCIe port claims to
> support hotplug. More information about that is in the changelog of
> patch [4/4].
> 
> Since this also touches xhci, I'm adding Mathias and Greg to check if the
> change looks reasonable.
> 
> Changes to v4:
> 
>   - Use "Put PCIe ports into D3" instead of "move" in subject and comments.
> 
>   - Add new helper function pci_power_manageable() and use it to check if
>     the port can be suspended instead of open-coding it everytime.
> 
>   - Changed pci_dev_check_d3cold() to follow what Rafael suggested.
> 
>   - Dropped call to pm_runtime_mark_last_busy() in pcie_port_runtime_resume()
>     as PM core does that automatically when the function returns 0.
> 
>   - Added ACKs from Rafael.
> 
> Changes to v3:
> 
>   - Make 'no_d3cold' persist for pci_dev. Once this is set through
>     pci_d3cold_enable/disable(), it will persist unless changed again using
>     the same functions. Those also handle setting and clearing 'bridge_d3'
>     as necessary.
> 
>   - Add kernel command line parameter "pcie_port_pm=off|force" that can
>     be used to forcibly disable/enable the feature.
> 
>   - Use runtime PM autosuspend instead of scheduling runtime suspend
>     manually
> 
>   - Allow runtime PM always except when we are dealing with a hotplug
>     bridge. Actual checking whether the port can be suspended is done in
>     ->idle() and ->runtime_suspend() hooks based on 'bridge_d3'.
> 
> Changes to v2:
> 
>   - Renamed and split pci_enable_d3cold() into two functions
>     pci_d3cold_enable()/disable().
> 
>   - Renamed pci_bridge_pm_update() into two functions
>     pci_bridge_d3_device_changed() and pci_bridge_d3_device_removed() that
>     should match better what they are doing.
> 
>   - Propagate ->bridge_d3 change to upstream bridges in
>     pci_bridge_d3_update().
> 
>   - Removed pci_can_suspend() in favor of doing ->bridge_d3 check directly
>     in pci_pm_suspend_noirq().
> 
>   - Extend runtime PM enabling for ports that are using native PCIe
>     hotplug.
> 
>   - Call pm_runtime_no_callbacks() for PCIe port service devices (the are
>     handled by the parent device).
> 
> Changes to v1:
> 
>   - Dropped patch [2/6] as there is no need to use that function from other
>     files anymore.
> 
>   - Dropped patches [5-6/6] in favor of using cut-off date.
> 
>   - Updated changelog of [1/4] to mention where in the PCI core PCI bridge
>     and PCIe ports are skipped from being power managed.
> 
>   - Instead of checking at suspend time if it is possible to transition the
>     port to D3, do it whenever power management status of a device (below a
>     port) is changed or when it is added or removed to the bus.
> 
>   - Added patch [3/4] to runtime resume a bridge when ACPI hotplug event is
>     received.
> 
> Mika Westerberg (4):
>   PCI: No need to set d3cold_allowed to PCIe ports
>   PCI: Put PCIe ports into D3 during suspend
>   ACPI / hotplug / PCI: Runtime resume bridge before rescan
>   PCI: Add runtime PM support for PCIe ports
> 
>  Documentation/kernel-parameters.txt |   4 +
>  drivers/pci/bus.c                   |   1 +
>  drivers/pci/hotplug/acpiphp_glue.c  |   8 +-
>  drivers/pci/pci-driver.c            |   5 +-
>  drivers/pci/pci-sysfs.c             |   5 ++
>  drivers/pci/pci.c                   | 175 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h                   |  11 +++
>  drivers/pci/pcie/portdrv_core.c     |   2 +
>  drivers/pci/pcie/portdrv_pci.c      |  51 ++++++++++-
>  drivers/pci/remove.c                |   2 +
>  drivers/usb/host/xhci-pci.c         |   2 +-
>  include/linux/pci.h                 |   3 +
>  12 files changed, 259 insertions(+), 10 deletions(-)

OK, I've just ACKed [2/4] and the other patches in this series were ACKed by me
before.

Bjorn, if you want me to take this series, please let me know.

Thanks,
Rafael

Re: [PATCH v5 1/4] PCI: No need to set d3cold_allowed to PCIe ports

[Bjorn Helgaas]

On Fri, Apr 29, 2016 at 11:51:56AM +0300, Mika Westerberg wrote:
> The Linux PCI core skips PCI bridges and PCIe ports when system is
> suspended. The PCI core checks return value of pci_has_subordinate() in
> pci_pm_suspend_noirq() to skip all devices where it is non-zero (which
> means PCI bridges and PCIe ports).

This patch looks fine to me.

But I wonder whether it's correct for pci_pm_suspend_noirq() (and the
other PM functions) to use pci_has_subordinate() as opposed to
pci_is_bridge().

pci_is_bridge() is true for all bridge devices (plain old PCI-PCI
bridges, PCIe ports, CardBus bridges, etc.)

pci_has_subordinate() is true only if the bridge has a struct pci_bus
allocated for its secondary bus.  This is probably the case for all or
almost all bridges, but it's conceivable that if we don't have enough
bus number space for the secondary bus, we might not allocate that
struct pci_bus.

What do you PM guys think?  I don't know what you would want to do
with a bridge that didn't have any reachable devices below it.

> Since PCIe ports are never suspended in the first place, there is no need
> to set d3cold_allowed for them.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/pcie/portdrv_pci.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index be35da2e105e..6c6bb03392ea 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -134,11 +134,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		return status;
>  
>  	pci_save_state(dev);
> -	/*
> -	 * D3cold may not work properly on some PCIe port, so disable
> -	 * it by default.
> -	 */
> -	dev->d3cold_allowed = false;
>  	return 0;
>  }
>  
> -- 
> 2.8.0.rc3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] PCI: No need to set d3cold_allowed to PCIe ports

[Rafael J. Wysocki]

On Wed, May 11, 2016 at 9:36 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Apr 29, 2016 at 11:51:56AM +0300, Mika Westerberg wrote:
>> The Linux PCI core skips PCI bridges and PCIe ports when system is
>> suspended. The PCI core checks return value of pci_has_subordinate() in
>> pci_pm_suspend_noirq() to skip all devices where it is non-zero (which
>> means PCI bridges and PCIe ports).
>
> This patch looks fine to me.
>
> But I wonder whether it's correct for pci_pm_suspend_noirq() (and the
> other PM functions) to use pci_has_subordinate() as opposed to
> pci_is_bridge().
>
> pci_is_bridge() is true for all bridge devices (plain old PCI-PCI
> bridges, PCIe ports, CardBus bridges, etc.)
>
> pci_has_subordinate() is true only if the bridge has a struct pci_bus
> allocated for its secondary bus.  This is probably the case for all or
> almost all bridges, but it's conceivable that if we don't have enough
> bus number space for the secondary bus, we might not allocate that
> struct pci_bus.
>
> What do you PM guys think?  I don't know what you would want to do
> with a bridge that didn't have any reachable devices below it.

Well, that's funny. :-)

The function we used in there used to be called pci_is_bridge(), as
per commit 46939f8b15e4 "PCI PM: Put devices into low power states
during late suspend (rev. 2)".

But then, it was renamed to pci_has_subordinate(), by commit
326c1cdae741 "PCI: Rename pci_is_bridge() to pci_has_subordinate()".

Next, a new pci_is_bridge() was introduced, by commit 1c86438c9423
"PCI: Add new pci_is_bridge() interface".  It is kind of hard to say
why that happened, because the changelog of that commit doesn't say
anything about the reason, but evidently the new one is not exactly
equivalent to the old one (as you mention).  The author of that commit
apparently was afraid to make that change in the PM code.

Now, I think what we do makes sense, because we want to avoid touching
bridges that have something below them (so as to avoid disrupting the
things below the bridge) and I'm not aware of any problems coming from
that.


>> Since PCIe ports are never suspended in the first place, there is no need
>> to set d3cold_allowed for them.
>>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  drivers/pci/pcie/portdrv_pci.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
>> index be35da2e105e..6c6bb03392ea 100644
>> --- a/drivers/pci/pcie/portdrv_pci.c
>> +++ b/drivers/pci/pcie/portdrv_pci.c
>> @@ -134,11 +134,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>>               return status;
>>
>>       pci_save_state(dev);
>> -     /*
>> -      * D3cold may not work properly on some PCIe port, so disable
>> -      * it by default.
>> -      */
>> -     dev->d3cold_allowed = false;
>>       return 0;
>>  }
>>
>> --

Re: [PATCH v5 0/4] PCI: Add support for suspending (including runtime) of PCIe ports

[Bjorn Helgaas]

On Fri, Apr 29, 2016 at 11:51:55AM +0300, Mika Westerberg wrote:
> Current Linux PCI core does not do any kind of power management to PCIe
> ports. This means that we waste energy and consume laptop battery even if
> the port has nothing connected to. These patches aim to change that to the
> right direction.
> 
> Previous versions of the patches can be found below:
> 
>   v1: http://www.spinics.net/lists/linux-pci/msg49313.html
>   v2: http://www.spinics.net/lists/linux-pci/msg50167.html
>   v3: http://www.spinics.net/lists/linux-pci/msg50345.html
>   v4: http://www.spinics.net/lists/linux-pci/msg50665.html
> 
> This assumes that recent (starting from 2015) PCIe ports are capable of
> transition to D3hot/D3cold. We add a new flag to struct pci_dev 'bridge_d3'
> that is set whenever the PCI core thinks the port can be put to D3. The
> check in pci_pm_suspend_noirq() is then extended to cover devices where
> 'bridge_d3' is set.
> 
> We then add two new functions pci_bridge_d3_device_changed/removed(). These
> are used to set and clear 'bridge_d3' whenever there is a change in device
> power management policy (or if the device is removed). For example when
> userspace forbids the device to enter D3cold pci_bridge_d3_device_changed()
> will clear 'bridge_d3' of the upstream bridge.
> 
> For all PCI ports where 'bridge_d3' is set we also enable and unblock
> runtime PM automatically. Only exception is when the PCIe port claims to
> support hotplug. More information about that is in the changelog of
> patch [4/4].
> 
> Since this also touches xhci, I'm adding Mathias and Greg to check if the
> change looks reasonable.
> 
> Changes to v4:
> 
>   - Use "Put PCIe ports into D3" instead of "move" in subject and comments.
> 
>   - Add new helper function pci_power_manageable() and use it to check if
>     the port can be suspended instead of open-coding it everytime.
> 
>   - Changed pci_dev_check_d3cold() to follow what Rafael suggested.
> 
>   - Dropped call to pm_runtime_mark_last_busy() in pcie_port_runtime_resume()
>     as PM core does that automatically when the function returns 0.
> 
>   - Added ACKs from Rafael.
> 
> Changes to v3:
> 
>   - Make 'no_d3cold' persist for pci_dev. Once this is set through
>     pci_d3cold_enable/disable(), it will persist unless changed again using
>     the same functions. Those also handle setting and clearing 'bridge_d3'
>     as necessary.
> 
>   - Add kernel command line parameter "pcie_port_pm=off|force" that can
>     be used to forcibly disable/enable the feature.
> 
>   - Use runtime PM autosuspend instead of scheduling runtime suspend
>     manually
> 
>   - Allow runtime PM always except when we are dealing with a hotplug
>     bridge. Actual checking whether the port can be suspended is done in
>     ->idle() and ->runtime_suspend() hooks based on 'bridge_d3'.
> 
> Changes to v2:
> 
>   - Renamed and split pci_enable_d3cold() into two functions
>     pci_d3cold_enable()/disable().
> 
>   - Renamed pci_bridge_pm_update() into two functions
>     pci_bridge_d3_device_changed() and pci_bridge_d3_device_removed() that
>     should match better what they are doing.
> 
>   - Propagate ->bridge_d3 change to upstream bridges in
>     pci_bridge_d3_update().
> 
>   - Removed pci_can_suspend() in favor of doing ->bridge_d3 check directly
>     in pci_pm_suspend_noirq().
> 
>   - Extend runtime PM enabling for ports that are using native PCIe
>     hotplug.
> 
>   - Call pm_runtime_no_callbacks() for PCIe port service devices (the are
>     handled by the parent device).
> 
> Changes to v1:
> 
>   - Dropped patch [2/6] as there is no need to use that function from other
>     files anymore.
> 
>   - Dropped patches [5-6/6] in favor of using cut-off date.
> 
>   - Updated changelog of [1/4] to mention where in the PCI core PCI bridge
>     and PCIe ports are skipped from being power managed.
> 
>   - Instead of checking at suspend time if it is possible to transition the
>     port to D3, do it whenever power management status of a device (below a
>     port) is changed or when it is added or removed to the bus.
> 
>   - Added patch [3/4] to runtime resume a bridge when ACPI hotplug event is
>     received.
> 
> Mika Westerberg (4):
>   PCI: No need to set d3cold_allowed to PCIe ports
>   PCI: Put PCIe ports into D3 during suspend
>   ACPI / hotplug / PCI: Runtime resume bridge before rescan
>   PCI: Add runtime PM support for PCIe ports
> 
>  Documentation/kernel-parameters.txt |   4 +
>  drivers/pci/bus.c                   |   1 +
>  drivers/pci/hotplug/acpiphp_glue.c  |   8 +-
>  drivers/pci/pci-driver.c            |   5 +-
>  drivers/pci/pci-sysfs.c             |   5 ++
>  drivers/pci/pci.c                   | 175 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h                   |  11 +++
>  drivers/pci/pcie/portdrv_core.c     |   2 +
>  drivers/pci/pcie/portdrv_pci.c      |  51 ++++++++++-
>  drivers/pci/remove.c                |   2 +
>  drivers/usb/host/xhci-pci.c         |   2 +-
>  include/linux/pci.h                 |   3 +
>  12 files changed, 259 insertions(+), 10 deletions(-)

Applied with Lukas' Tested-by and Rafael's Acks to pci/pm for v4.7,
thanks Mika!

Re: [PATCH v5 4/4] PCI: Add runtime PM support for PCIe ports

[Bjorn Helgaas]

On Fri, Apr 29, 2016 at 11:51:59AM +0300, Mika Westerberg wrote:
> Add back runtime PM support for PCIe ports that was removed in
> commit fe9a743a2601 ("PCI/PM: Drop unused runtime PM support code for
> PCIe ports").
> 
> First of all we cannot enable it automatically for all ports since there
> has been problems previously as can be seen in [1]. In summary suspended
> PCIe ports were not able to deal with ACPI based hotplug reliably. One
> reason why this might happen is the fact that when a PCIe port is powered
> down, config space access to the devices behind the port is not possible.
> If the BIOS hotplug SMI handler assumes the port is always in D0 it will
> not be able to find the hotplugged devices. To be on the safe side only
> enable runtime PM if the port does not claim to support hotplug.
> 
> For PCIe ports not using hotplug we enable and allow runtime PM
> automatically. Since 'bridge_d3' can be changed any time we check this in
> driver ->runtime_idle() and ->runtime_suspend() and only allow runtime
> suspend if the flag is still set. Use autosuspend with default of 10ms idle
> time to prevent the port from repeatedly suspending and resuming on
> continuous configuration space access of devices behind the port.
> 
> The actual power transition to D3 and back is handled in the PCI core.
> 
> Idea to automatically unblock (allow) runtime PM for PCIe ports came from
> Dave Airlie.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=53811
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/pcie/portdrv_core.c |  2 ++
>  drivers/pci/pcie/portdrv_pci.c  | 50 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 88122dc2e1b1..65b1a624826b 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
>  #include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/string.h>
>  #include <linux/slab.h>
>  #include <linux/pcieport_if.h>
> @@ -343,6 +344,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
>  		     get_descriptor_id(pci_pcie_type(pdev), service));
>  	device->parent = &pdev->dev;
>  	device_enable_async_suspend(device);
> +	pm_runtime_no_callbacks(device);
>  
>  	retval = device_register(device);
>  	if (retval) {
> diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> index 6c6bb03392ea..f4409af00756 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -93,6 +93,26 @@ static int pcie_port_resume_noirq(struct device *dev)
>  	return 0;
>  }
>  
> +static int pcie_port_runtime_suspend(struct device *dev)
> +{
> +	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> +}
> +
> +static int pcie_port_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int pcie_port_runtime_idle(struct device *dev)
> +{
> +	/*
> +	 * Rely the PCI core has set bridge_d3 whenever it thinks the port
> +	 * should be good to go to D3. Everything else, including moving
> +	 * the port to D3, is handled by the PCI core.
> +	 */
> +	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
> +}
> +
>  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.suspend	= pcie_port_device_suspend,
>  	.resume		= pcie_port_device_resume,
> @@ -101,6 +121,9 @@ static const struct dev_pm_ops pcie_portdrv_pm_ops = {
>  	.poweroff	= pcie_port_device_suspend,
>  	.restore	= pcie_port_device_resume,
>  	.resume_noirq	= pcie_port_resume_noirq,
> +	.runtime_suspend = pcie_port_runtime_suspend,
> +	.runtime_resume	= pcie_port_runtime_resume,
> +	.runtime_idle	= pcie_port_runtime_idle,
>  };
>  
>  #define PCIE_PORTDRV_PM_OPS	(&pcie_portdrv_pm_ops)
> @@ -134,11 +157,38 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		return status;
>  
>  	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.

Can we wordsmith this comment a bit?  I'm not sure what "BIOS hotplug
SMI code" even is or why it is relevant.  And it seems x86-centric so
may not apply to other arches.

> +	 */
> +	if (!dev->is_hotplug_bridge) {

Your changelog mentions ACPI hotplug (I assume this means acpiphp).
Would it be sufficient to prevent runtime PM when we're using acpiphp?
That would be more selective and would avoid penalizing non-ACPI
platforms and newer systems that use native pciehp.

PCIe r3.0, sec 6.7.3.4 says a port should generate a wakeup event
using PME on hotplug events that occur when the port is in D1, D2, or
D3hot (it doesn't mention D3cold).  Is that relevant here?  Is there a
way to control which D-states we use based on which ones can assert
PME?

> +		/*
> +		 * Keep the port resumed 10ms to make sure things like
> +		 * config space accesses from userspace (lspci) will not
> +		 * cause the port to repeatedly suspend and resume.
> +		 */
> +		pm_runtime_set_autosuspend_delay(&dev->dev, 10);
> +		pm_runtime_use_autosuspend(&dev->dev);
> +		pm_runtime_put_autosuspend(&dev->dev);
> +		pm_runtime_allow(&dev->dev);
> +	}
> +
>  	return 0;
>  }
>  
>  static void pcie_portdrv_remove(struct pci_dev *dev)
>  {
> +	if (!dev->is_hotplug_bridge) {
> +		pm_runtime_forbid(&dev->dev);
> +		pm_runtime_get_noresume(&dev->dev);
> +		pm_runtime_dont_use_autosuspend(&dev->dev);
> +	}
> +
>  	pcie_port_device_remove(dev);
>  }
>  
> -- 
> 2.8.0.rc3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/4] PCI: Add runtime PM support for PCIe ports

[Lukas Wunner]

On Fri, Jun 17, 2016 at 03:48:24PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 29, 2016 at 11:51:59AM +0300, Mika Westerberg wrote:
[snip]
> > +
> > +	/*
> > +	 * 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.
> 
> Can we wordsmith this comment a bit?  I'm not sure what "BIOS hotplug
> SMI code" even is or why it is relevant.  And it seems x86-centric so
> may not apply to other arches.

The comment pertains to:
https://bugzilla.kernel.org/show_bug.cgi?id=53811

It was discussed on April 12/13:
https://patchwork.kernel.org/patch/8782801/

Executive summary:
On non-Macs, Thunderbolt is driven by the firmware in System Management
Mode, on hotplug the CPU receives a Sytem Management Interrupt (SMI) and
jumps to SMM code. This doesn't work reliably if the hotplug port has
been transitioned to D3hot, the SMM code tries to access config space
of hotplugged devices (not possible if port is in D3hot) and is apparently
too dumb to check if the port is in D3hot and wake it up.

On Macs, Thunderbolt is not driven by the firmware except immediately on
boot: It is initially set up by an EFI driver, but once ExitBootServices
has been called, the OS is responsible. Therefore on Macs the hotplug
ports may be suspended to D3hot just fine. This is done by patch
[08/13] of my series "Runtime PM for Thunderbolt on Macs".

> 
> > +	 */
> > +	if (!dev->is_hotplug_bridge) {
> 
> Your changelog mentions ACPI hotplug (I assume this means acpiphp).
> Would it be sufficient to prevent runtime PM when we're using acpiphp?
> That would be more selective and would avoid penalizing non-ACPI
> platforms and newer systems that use native pciehp.
> 
> PCIe r3.0, sec 6.7.3.4 says a port should generate a wakeup event
> using PME on hotplug events that occur when the port is in D1, D2, or
> D3hot (it doesn't mention D3cold).  Is that relevant here?  Is there a
> way to control which D-states we use based on which ones can assert
> PME?

This was discussed between April 18 and April 21, please refer to
the above-linked patchwork entry.

I had done some tests and the result was:
"A hotplug port may go to D3hot and will still generate interrupts on
hotplug, but all its parent ports are *not* allowed to go to D3hot
because otherwise the hotplug interrupts will no longer come through.
The algorithm in pci_bridge_d3_update() and pci_dev_check_d3cold()
needs to be amended to take that into account. Hm, it's nontrivial
I guess, allowing bridge_d3 = true for the lowest hotplug bridge in a
hierarchy but not for anything above?"

Mika's reply was:
"If we need to do things like that, it will get pretty complex and we
still cannot be sure whether hotplug works. I think it is safer to go
back to what I already had and disable runtime PM from such ports."

Best regards,

Lukas

Re: [PATCH v5 4/4] PCI: Add runtime PM support for PCIe ports

[Mika Westerberg]

On Fri, Jun 17, 2016 at 11:32:09PM +0200, Lukas Wunner wrote:
> On Fri, Jun 17, 2016 at 03:48:24PM -0500, Bjorn Helgaas wrote:
> > On Fri, Apr 29, 2016 at 11:51:59AM +0300, Mika Westerberg wrote:
> [snip]
> > > +
> > > +	/*
> > > +	 * 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.
> > 
> > Can we wordsmith this comment a bit?  I'm not sure what "BIOS hotplug
> > SMI code" even is or why it is relevant.  And it seems x86-centric so
> > may not apply to other arches.
> 
> The comment pertains to:
> https://bugzilla.kernel.org/show_bug.cgi?id=53811
> 
> It was discussed on April 12/13:
> https://patchwork.kernel.org/patch/8782801/
> 
> Executive summary:
> On non-Macs, Thunderbolt is driven by the firmware in System Management
> Mode, on hotplug the CPU receives a Sytem Management Interrupt (SMI) and
> jumps to SMM code. This doesn't work reliably if the hotplug port has
> been transitioned to D3hot, the SMM code tries to access config space
> of hotplugged devices (not possible if port is in D3hot) and is apparently
> too dumb to check if the port is in D3hot and wake it up.

That's the best guess what happens on non-Macs.

Bjorn, do you want me to update the comment to open up what SMI means
and maybe add better explanation?

> On Macs, Thunderbolt is not driven by the firmware except immediately on
> boot: It is initially set up by an EFI driver, but once ExitBootServices
> has been called, the OS is responsible. Therefore on Macs the hotplug
> ports may be suspended to D3hot just fine. This is done by patch
> [08/13] of my series "Runtime PM for Thunderbolt on Macs".
> 
> > 
> > > +	 */
> > > +	if (!dev->is_hotplug_bridge) {
> > 
> > Your changelog mentions ACPI hotplug (I assume this means acpiphp).
> > Would it be sufficient to prevent runtime PM when we're using acpiphp?
> > That would be more selective and would avoid penalizing non-ACPI
> > platforms and newer systems that use native pciehp.
> > 
> > PCIe r3.0, sec 6.7.3.4 says a port should generate a wakeup event
> > using PME on hotplug events that occur when the port is in D1, D2, or
> > D3hot (it doesn't mention D3cold).  Is that relevant here?  Is there a
> > way to control which D-states we use based on which ones can assert
> > PME?
> 
> This was discussed between April 18 and April 21, please refer to
> the above-linked patchwork entry.
> 
> I had done some tests and the result was:
> "A hotplug port may go to D3hot and will still generate interrupts on
> hotplug, but all its parent ports are *not* allowed to go to D3hot
> because otherwise the hotplug interrupts will no longer come through.
> The algorithm in pci_bridge_d3_update() and pci_dev_check_d3cold()
> needs to be amended to take that into account. Hm, it's nontrivial
> I guess, allowing bridge_d3 = true for the lowest hotplug bridge in a
> hierarchy but not for anything above?"
> 
> Mika's reply was:
> "If we need to do things like that, it will get pretty complex and we
> still cannot be sure whether hotplug works. I think it is safer to go
> back to what I already had and disable runtime PM from such ports."

Indeed that seems to be the most reliable way.

In addition it may be possible to relax this a bit more like:

 - For ACPI hotplug bridges we disable runtime PM like we are doing
   currently.

 - For native PCIe ports supporting hotplug, enable runtime PM and call
   pci_d3cold_disable() in pcie_portdrv_probe() which allows the port to
   go into D3hot.

But I think it may be better to do that separately after v4.8-rc1 is
released :)

Re: [PATCH v5 4/4] PCI: Add runtime PM support for PCIe ports

[Bjorn Helgaas]

On Mon, Jun 20, 2016 at 11:10:57AM +0300, Mika Westerberg wrote:
> On Fri, Jun 17, 2016 at 11:32:09PM +0200, Lukas Wunner wrote:
> > On Fri, Jun 17, 2016 at 03:48:24PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Apr 29, 2016 at 11:51:59AM +0300, Mika Westerberg wrote:
> > [snip]
> > > > +
> > > > +	/*
> > > > +	 * 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.
> > > 
> > > Can we wordsmith this comment a bit?  I'm not sure what "BIOS hotplug
> > > SMI code" even is or why it is relevant.  And it seems x86-centric so
> > > may not apply to other arches.
> > 
> > The comment pertains to:
> > https://bugzilla.kernel.org/show_bug.cgi?id=53811
> > 
> > It was discussed on April 12/13:
> > https://patchwork.kernel.org/patch/8782801/
> > 
> > Executive summary:
> > On non-Macs, Thunderbolt is driven by the firmware in System Management
> > Mode, on hotplug the CPU receives a Sytem Management Interrupt (SMI) and
> > jumps to SMM code. This doesn't work reliably if the hotplug port has
> > been transitioned to D3hot, the SMM code tries to access config space
> > of hotplugged devices (not possible if port is in D3hot) and is apparently
> > too dumb to check if the port is in D3hot and wake it up.
> 
> That's the best guess what happens on non-Macs.
> 
> Bjorn, do you want me to update the comment to open up what SMI means
> and maybe add better explanation?

I withdraw my request.  I guess it's probably not worth doing anything
right now.  It's just a little out of place because this is
theoretically generic code, but the whole SMM thing is x86-specific.

Bjorn