PCIe port PM: high gloss polish & hotplug support
v2 cover PCIe port PM: high gloss polish & hotplug support
Respin of this series to polish the runtime PM support for PCIe ports that was added with v4.8, and extend it to native hotplug ports:
- All patches have been reviewed by Rafael, patches 1 to 8 have been tested by Mika. (Thanks a lot!)
- Patch 6 ("PCI: Unfold conditions to block runtime PM on PCIe ports") contains a minor change relative to v1 wherein the function returns as soon as a single condition evaluates to true. (Instead of needlessly evaluating all the remaining conditions.)
- Patch 9 was rebased to accommodate to this change but is otherwise the same as before.
As usual I've pushed the series to GitHub to ease reviewing/fetching:
https://github.com/l1k/linux/commits/pcie_port_pm_v2
Link to v1:
http://www.spinics.net/lists/linux-pci/msg55347.html
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 | 100 ++++++++++++++++++------------------- 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, 88 insertions(+), 96 deletions(-)
Signature · 1 line
2.9.3
PATCH v2 1/9 PCI: Don't acquire ref on parent in pci_bridge_d3_update()
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.
--- drivers/pci/pci.c | 3 --- 1 file changed, 3 deletions(-)
drivers/pci/pci.c +0 -3
@@ -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); }/**
Signature · 1 line
2.9.3
PATCH v2 2/9 PCI: Autosense device removal in pci_bridge_d3_update()
In reply to Lukas WunnerThe 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.
--- 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(-)
drivers/pci/bus.c +1 -1
@@ -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);
drivers/pci/pci.c +5 -30
@@ -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);
drivers/pci/pci.h +1 -2
@@ -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) {
drivers/pci/remove.c +1 -1
@@ -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); }
Signature · 1 line
2.9.3
PATCH v2 3/9 PCI: Speed up algorithm in pci_bridge_d3_update()
In reply to Lukas WunnerAfter 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.
--- drivers/pci/pci.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
drivers/pci/pci.c +20 -8
@@ -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;
Signature · 1 line
2.9.3
PATCH v2 4/9 PCI: Activate runtime PM on a PCIe port only if it can suspend
In reply to Lukas WunnerCurrently 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.
--- drivers/pci/pci.c | 2 +- drivers/pci/pci.h | 1 + drivers/pci/pcie/portdrv_pci.c | 5 +++-- 3 files changed, 5 insertions(+), 3 deletions(-)
drivers/pci/pci.c +1 -1
@@ -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;
drivers/pci/pci.h +1 -0
@@ -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)
drivers/pci/pcie/portdrv_pci.c +3 -2
@@ -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);
Signature · 1 line
2.9.3
PATCH v2 5/9 PCI: Consolidate conditions to allow runtime PM on PCIe ports
In reply to Lukas WunnerThe 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.
--- drivers/pci/pci.c | 11 +++++++++++ drivers/pci/pcie/portdrv_pci.c | 12 ++---------- 2 files changed, 13 insertions(+), 10 deletions(-)
drivers/pci/pci.c +11 -0
@@ -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;
drivers/pci/pcie/portdrv_pci.c +2 -10
@@ -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);
Signature · 1 line
2.9.3
PATCH v2 6/9 PCI: Unfold conditions to block runtime PM on PCIe ports
In reply to Lukas WunnerThe 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.
--- drivers/pci/pci.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
drivers/pci/pci.c +11 -10
@@ -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); +if (/* The device needs to be allowed to go D3cold ... */ + dev->no_d3cold || !dev->d3cold_allowed || + +/* ... and if it is wakeup capable to do so from D3cold. */ + (device_may_wakeup (&dev->dev) && + !pci_pme_capable (dev, PCI_D3cold)) || + +/* If it is a bridge it must be allowed to go to D3. */ + !pci_power_manageable (dev)) - *d3cold_ok = !no_d3cold; + *d3cold_ok = false; -return no_d3cold; +return !*d3cold_ok; }/*
Signature · 1 line
2.9.3
PATCH v2 7/9 ACPI / hotplug / PCI: Use cached copy of PCI_EXP_SLTCAP_HPC bit
In reply to Lukas WunnerWe 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.
--- drivers/pci/hotplug/acpiphp_glue.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/pci/hotplug/acpiphp_glue.c +1 -4
@@ -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 , ®32)) -return false; -if (!(reg32 &PCI_EXP_SLTCAP_HPC )) +if (!pdev->is_hotplug_bridge)return false;/*
Signature · 1 line
2.9.3
PATCH v2 8/9 ACPI / hotplug / PCI: Make device_is_managed_by_native_pciehp() public
In reply to Lukas WunnerWe'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.
--- 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(-)
drivers/pci/hotplug/acpiphp_glue.c +1 -27
@@ -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;
drivers/pci/pci-acpi.c +24 -0
@@ -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. */
include/linux/pci_hotplug.h +2 -0
@@ -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
Signature · 1 line
2.9.3
PATCH v2 9/9 PCI: pciehp: Add runtime PM support for PCIe hotplug ports
In reply to Lukas WunnerLinux 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
--- drivers/pci/hotplug/pciehp_ctrl.c | 6 ++++++ drivers/pci/pci.c | 10 +++++----- 2 files changed, 11 insertions(+), 5 deletions(-)
drivers/pci/hotplug/pciehp_ctrl.c +6 -0
@@ -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;
drivers/pci/pci.c +5 -5
@@ -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)
@@ -2279,7 +2276,10 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data) !pci_pme_capable(dev, PCI_D3cold)) ||/* If it is a bridge it must be allowed to go to D3. */ - !pci_power_manageable(dev)) + !pci_power_manageable (dev) || + +/* Hotplug interrupts cannot be delivered if the link is down. */ + dev->is_hotplug_bridge) *d3cold_ok = false;
Signature · 1 line
2.9.3
Bjorn Helgaas
AppliedQuoted 36 lines · Lukas Wunner
Respin of this series to polish the runtime PM support for PCIe ports that was added with v4.8, and extend it to native hotplug ports:
- All patches have been reviewed by Rafael, patches 1 to 8 have been tested by Mika. (Thanks a lot!)
- Patch 6 ("PCI: Unfold conditions to block runtime PM on PCIe ports") contains a minor change relative to v1 wherein the function returns as soon as a single condition evaluates to true. (Instead of needlessly evaluating all the remaining conditions.)
- Patch 9 was rebased to accommodate to this change but is otherwise the same as before.
As usual I've pushed the series to GitHub to ease reviewing/fetching:
https://github.com/l1k/linux/commits/pcie_port_pm_v2Link to v1:
http://www.spinics.net/lists/linux-pci/msg55347.htmlThanks,
Lukas
Lukas Wunner (9):
PCI: Don't acquire ref on parent inpci_bridge_d3_update()
PCI: Autosense device removal inpci_bridge_d3_update()
PCI: Speed up algorithm inpci_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 ofPCI_EXP_SLTCAP_HPCbit ACPI / hotplug / PCI: Makedevice_is_managed_by_native_pciehp()public
PCI: pciehp: Add runtime PM support for PCIe hotplug ports
Quoted 10 lines
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 | 100 ++++++++++++++++++------------------- 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, 88 insertions(+), 96 deletions(-)
I applied these to pci/pm for v4.10, thanks!
I did make two changes to the changelog of the last patch: you mentioned the "subordinate" bus, and I changed it to "secondary". We have often used "subordinate" to refer to the immediately-downstream bus, but that's slightly confusing because in the bridge spec, "subordinate" means the highest bus number downstream from the bridge, while "secondary" refers to the immediately-downstream bridge. Am I making sense or confusing things more?
Bjorn
Quoted 3 lines · Bjorn Helgaas
Quoted 2 lines · Lukas Wunner
Respin of this series to polish the runtime PM support for PCIe ports that was added with v4.8, and extend it to native hotplug ports:
[snip]
Quoted 9 lines
I applied these to pci/pm for v4.10, thanks!
I did make two changes to the changelog of the last patch: you mentioned the "subordinate" bus, and I changed it to "secondary". We have often used "subordinate" to refer to the immediately-downstream bus, but that's slightly confusing because in the bridge spec, "subordinate" means the highest bus number downstream from the bridge, while "secondary" refers to the immediately-downstream bridge. Am I making sense or confusing things more?
Indeed the secondary bus was meant here and I used "subordinate" only because the attribute in struct pci_dev is named like this. :-)
Thanks,
Lukas
Quoted 15 lines · Lukas Wunner
Quoted 3 lines · Bjorn Helgaas
Quoted 2 lines · Lukas Wunner
Respin of this series to polish the runtime PM support for PCIe ports that was added with v4.8, and extend it to native hotplug ports:
[snip]
Quoted 9 lines
I applied these to pci/pm for v4.10, thanks!
I did make two changes to the changelog of the last patch: you mentioned the "subordinate" bus, and I changed it to "secondary". We have often used "subordinate" to refer to the immediately-downstream bus, but that's slightly confusing because in the bridge spec, "subordinate" means the highest bus number downstream from the bridge, while "secondary" refers to the immediately-downstream bridge. Am I making sense or confusing things more?
Quoted 2 lines
Indeed the secondary bus was meant here and I used "subordinate" only because the attribute in struct
pci_devis named like this. :-)
Yeah, I wish it weren't named that way, but I guess we're stuck with that one.