pciehp runtime PM
cover pciehp runtime PM
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(-)
Signature · 1 line
2.11.0
PATCH 1/5 PCI: pciehp: Resume to D0 on board addition/removal
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().
--- drivers/pci/hotplug/pciehp_ctrl.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/pci/hotplug/pciehp_ctrl.c +11 -2
@@ -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)
Signature · 1 line
2.11.0
PATCH 2/5 PCI: pciehp: Remain in D0 while awaiting command completion
In reply to Lukas WunnerHotplug 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).
--- drivers/pci/hotplug/pciehp_hpc.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/pci/hotplug/pciehp_hpc.c +8 -1
@@ -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);
Signature · 1 line
2.11.0
PATCH 3/5 PCI: pciehp: Resume to D0 on sysfs read access
In reply to Lukas WunnerEnsure 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().
--- drivers/pci/hotplug/pciehp_hpc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/pci/hotplug/pciehp_hpc.c +10 -0
@@ -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 ); }
Signature · 1 line
2.11.0
PATCH 4/5 PCI: pciehp: Remain in D0 if poll mode is enabled
In reply to Lukas WunnerThe 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.
--- drivers/pci/hotplug/pciehp_hpc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/pci/hotplug/pciehp_hpc.c +4 -2
@@ -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); }
Signature · 1 line
2.11.0
PATCH 5/5 PCI: Whitelist native hotplug ports for runtime PM
In reply to Lukas WunnerPreviously 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.
--- drivers/pci/pci.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/pci/pci.c +2 -5
@@ -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)
Signature · 1 line
2.11.0