[PATCH 0/5] pciehp runtime PM

[PATCH 0/5] pciehp runtime PM

[Lukas Wunner]

Introduce runtime PM to pciehp in a more thorough and comprehensive way
than the reverted 68db9bc81436, taking into account the regressions
reported against it.

@Yinghai Lu & Intel/Facebook engineers, I would be grateful if you
could test this on as many systems as you can get your hands on,
in particular the Skylake servers that were broken by 68db9bc81436.
I guess those were Skylake-SP Xeon systems (Purley platform).
They're still unreleased and not available outside Intel and their
partners.  Their release date has been pushed further back to
mid-summer.  Myself I've only got a puny, 7 years old "Light Ridge"
Thunderbolt controller to test with and it doesn't even fully adhere
to the PCIe spec.

I'd also be interested to learn if you can measure a drop in power usage
on systems with unoccupied hotplug ports.

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

Thanks!

Lukas


Lukas Wunner (5):
  PCI: pciehp: Resume to D0 on board addition/removal
  PCI: pciehp: Remain in D0 while awaiting command completion
  PCI: pciehp: Resume to D0 on sysfs read access
  PCI: pciehp: Remain in D0 if poll mode is enabled
  PCI: Whitelist native hotplug ports for runtime PM

 drivers/pci/hotplug/pciehp_ctrl.c | 13 +++++++++++--
 drivers/pci/hotplug/pciehp_hpc.c  | 25 ++++++++++++++++++++++---
 drivers/pci/pci.c                 |  7 ++-----
 3 files changed, 35 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH 1/5] PCI: pciehp: Resume to D0 on board addition/removal

[Lukas Wunner]

Hotplug ports need to be in D0 to access devices on their secondary bus.
pciehp itself does this in
    pciehp_check_link_status(),
    pciehp_configure_device() (both called from board_added())
and
    pciehp_unconfigure_device() (called from remove_board()).

In addition, Yinghai Lu discovered that some Skylake server CPUs or PCHs
feature a Power Controller for their PCIe hotplug ports (PCIe r3.1, sec
6.7.1.8) which requires the port to be in D0 when invoking
    pciehp_power_on_slot() (likewise called from board_added()).

The spec is silent about such a requirement, but it seems prudent to
assume that any hotplug port with a Power Controller may need this.

Thus, acquire a runtime PM ref for the invocation of board_added() and
remove_board().

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Erik Veijola <erik.veijola@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Krishna Dhulipala <krishnad@fb.com>
Cc: Wei Zhang <wzhang@fb.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index ec0b4c11ccd9..d071aa63dac9 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"
@@ -390,6 +391,7 @@ int pciehp_enable_slot(struct slot *p_slot)
 {
 	u8 getstatus = 0;
 	struct controller *ctrl = p_slot->ctrl;
+	int retval;
 
 	pciehp_get_adapter_status(p_slot, &getstatus);
 	if (!getstatus) {
@@ -414,7 +416,10 @@ int pciehp_enable_slot(struct slot *p_slot)
 		}
 	}
 
-	return board_added(p_slot);
+	pm_runtime_get_sync(&ctrl->pcie->port->dev);
+	retval = board_added(p_slot);
+	pm_runtime_put(&ctrl->pcie->port->dev);
+	return retval;
 }
 
 /*
@@ -424,6 +429,7 @@ int pciehp_disable_slot(struct slot *p_slot)
 {
 	u8 getstatus = 0;
 	struct controller *ctrl = p_slot->ctrl;
+	int retval;
 
 	if (!p_slot->ctrl)
 		return 1;
@@ -437,7 +443,10 @@ int pciehp_disable_slot(struct slot *p_slot)
 		}
 	}
 
-	return remove_board(p_slot);
+	pm_runtime_get_sync(&ctrl->pcie->port->dev);
+	retval = remove_board(p_slot);
+	pm_runtime_put(&ctrl->pcie->port->dev);
+	return retval;
 }
 
 int pciehp_sysfs_enable_slot(struct slot *p_slot)
-- 
2.11.0

[PATCH 2/5] PCI: pciehp: Remain in D0 while awaiting command completion

[Lukas Wunner]

Hotplug controllers may signal an interrupt after a command written to
the Slot Control register completes (PCIe r3.1, sec 6.7.3.2).

If the controller happens to be in D3hot and is PME capable, this
Command Complete interrupt triggers a wakeup to D0 because:

(a) PME is the only valid operation that can be initiated by the
    function when in D3hot (PCI PM r1.2, table 5-4, though Thunderbolt
    controllers do not adhere to this rule).

(b) Hotplug events (including command completion) occurring in D3hot
    cause "generation of a wakeup event (using the PME mechanism)"
    (PCIe r3.1, sec 6.7.3.4).

When writing to the Slot Control register, we've resumed the controller
to D0 (see the call sites of pcie_write_cmd and pcie_write_cmd_nowait).
It's silly to let it go to D3hot afterwards if we know that it will be
resumed to D0 once the command completes.  Thus, hold a runtime PM ref
while awaiting command completion.  In the "nowait" case, schedule
runtime suspend after 1 second, which is the time limit for execution of
commands (PCIe r3.1, sec 6.7.3.2).

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Erik Veijola <erik.veijola@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Krishna Dhulipala <krishnad@fb.com>
Cc: Wei Zhang <wzhang@fb.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/pciehp_hpc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 026830a138ae..143e2143d62e 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -34,6 +34,7 @@
 #include <linux/jiffies.h>
 #include <linux/timer.h>
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 #include <linux/interrupt.h>
 #include <linux/time.h>
 #include <linux/slab.h>
@@ -201,6 +202,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
 	slot_ctrl |= (cmd & mask);
 	ctrl->cmd_busy = 1;
 	smp_mb();
+	pm_runtime_get_sync(&pdev->dev);
 	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
 	ctrl->cmd_started = jiffies;
 	ctrl->slot_ctrl = slot_ctrl;
@@ -209,8 +211,13 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
 	 * Optionally wait for the hardware to be ready for a new command,
 	 * indicating completion of the above issued command.
 	 */
-	if (wait)
+	if (wait) {
 		pcie_wait_cmd(ctrl);
+		pm_runtime_put(&pdev->dev);
+	} else {
+		pm_runtime_put_noidle(&pdev->dev);
+		pm_schedule_suspend(&pdev->dev, 1000);
+	}
 
 out:
 	mutex_unlock(&ctrl->ctrl_lock);
-- 
2.11.0

[PATCH 4/5] PCI: pciehp: Remain in D0 if poll mode is enabled

[Lukas Wunner]

The slot status is polled from a timer, i.e. in softirq context, and
pm_runtime_get_sync() may sleep.  Hence it's not an option to runtime
resume the port whenever its slot status is polled.  It would also be
silly to continuously switch back and forth between D0 and D3hot, so
keep it runtime active if poll mode is enabled.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Erik Veijola <erik.veijola@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Krishna Dhulipala <krishnad@fb.com>
Cc: Wei Zhang <wzhang@fb.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/pciehp_hpc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 70dd9ae4c097..20170bcf9862 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -86,6 +86,7 @@ static inline int pciehp_request_irq(struct controller *ctrl)
 	if (pciehp_poll_mode) {
 		init_timer(&ctrl->poll_timer);
 		start_int_poll_timer(ctrl, 10);
+		pm_runtime_get(&ctrl->pcie->port->dev);
 		return 0;
 	}
 
@@ -99,9 +100,10 @@ static inline int pciehp_request_irq(struct controller *ctrl)
 
 static inline void pciehp_free_irq(struct controller *ctrl)
 {
-	if (pciehp_poll_mode)
+	if (pciehp_poll_mode) {
 		del_timer_sync(&ctrl->poll_timer);
-	else
+		pm_runtime_put(&ctrl->pcie->port->dev);
+	} else
 		free_irq(ctrl->pcie->irq, ctrl);
 }
 
-- 
2.11.0

[PATCH 3/5] PCI: pciehp: Resume to D0 on sysfs read access

[Lukas Wunner]

Ensure accessibility of config space when it is read via sysfs.

Accessibility on write is already covered by the runtime resume in
pcie_do_write_cmd().

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Erik Veijola <erik.veijola@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Krishna Dhulipala <krishnad@fb.com>
Cc: Wei Zhang <wzhang@fb.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/pciehp_hpc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 143e2143d62e..70dd9ae4c097 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -369,7 +369,9 @@ int pciehp_get_raw_indicator_status(struct hotplug_slot *hotplug_slot,
 	struct pci_dev *pdev = ctrl_dev(slot->ctrl);
 	u16 slot_ctrl;
 
+	pm_runtime_get_sync(&pdev->dev);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+	pm_runtime_put(&pdev->dev);
 	*status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
 	return 0;
 }
@@ -380,7 +382,9 @@ void pciehp_get_attention_status(struct slot *slot, u8 *status)
 	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 slot_ctrl;
 
+	pm_runtime_get_sync(&pdev->dev);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+	pm_runtime_put(&pdev->dev);
 	ctrl_dbg(ctrl, "%s: SLOTCTRL %x, value read %x\n", __func__,
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
 
@@ -406,7 +410,9 @@ void pciehp_get_power_status(struct slot *slot, u8 *status)
 	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 slot_ctrl;
 
+	pm_runtime_get_sync(&pdev->dev);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+	pm_runtime_put(&pdev->dev);
 	ctrl_dbg(ctrl, "%s: SLOTCTRL %x value read %x\n", __func__,
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
 
@@ -428,7 +434,9 @@ void pciehp_get_latch_status(struct slot *slot, u8 *status)
 	struct pci_dev *pdev = ctrl_dev(slot->ctrl);
 	u16 slot_status;
 
+	pm_runtime_get_sync(&pdev->dev);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+	pm_runtime_put(&pdev->dev);
 	*status = !!(slot_status & PCI_EXP_SLTSTA_MRLSS);
 }
 
@@ -437,7 +445,9 @@ void pciehp_get_adapter_status(struct slot *slot, u8 *status)
 	struct pci_dev *pdev = ctrl_dev(slot->ctrl);
 	u16 slot_status;
 
+	pm_runtime_get_sync(&pdev->dev);
 	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+	pm_runtime_put(&pdev->dev);
 	*status = !!(slot_status & PCI_EXP_SLTSTA_PDS);
 }
 
-- 
2.11.0

[PATCH 5/5] PCI: Whitelist native hotplug ports for runtime PM

[Lukas Wunner]

Previously we blacklisted PCIe hotplug ports for runtime PM because:

(a) Ports handled by the firmware must not be transitioned to D3hot by
    OSPM behind the firmware's back:
    https://bugzilla.kernel.org/show_bug.cgi?id=53811

(b) For ports handled natively by OSPM we lacked proper runtime PM
    support in the pciehp driver.

We've just rectified the latter, so stop blacklisting those.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Erik Veijola <erik.veijola@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Krishna Dhulipala <krishnad@fb.com>
Cc: Wei Zhang <wzhang@fb.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5bba8e6..7567e575d536 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2244,13 +2244,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
+		 * 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)
-- 
2.11.0