PCI PM refinements

PATCH Applied Lukas Wunner 4 participants 15 messages Sep 18, 2016 – Oct 5, 2016

Subsystems (2)

ACPI PCI

lore

Share & tools

lore

Lukas Wunner

author

2016-09-18 03:39 UTC #0 lore raw

v2 cover PCI PM refinements

Here's the second installment of this series, based on feedback from Rafael. (Thanks!)

To reiterate, these refinements are meant to speed up and reduce power consumption when entering and leaving system sleep and when shutting down the system. For details, please refer to the cover letter of v1:
http://www.spinics.net/lists/linux-pci/msg53702.html

As usual I've pushed the patches to GitHub in case anyone prefers reviewing in a GUI:
https://github.com/l1k/linux/commits/pci_pm_refinements_v2

Changes since v1:

* Patch [1/5] now sports an ack by Rafael.

* Patch [2/5]:
* In acpi_pci_get_power_state(), only check for ACPI_STATE_UNKNOWN instead of ACPI_STATE_D0 and ACPI_STATE_D3_COLD.
* Move the change to pci_update_current_state() to new patch [3/5].

* Patch [3/5]:
* Instead of solely relying on the platform firmware to report D3cold, also probe the vendor ID and assume D3cold if it can't be read. This should ensure proper detection of D3cold on pre-ACPI 4.0 platforms (which will never report anything deeper than D3hot) as well as for devices with nonstandard PM mechanisms.
* The two existing workarounds for D3cold are removed from pci_update_current_state(), as explained in the commit message.

* Patch [5/5]:
* Disable runtime PM on the device to prevent it from being runtime resumed during the remainder of the shutdown process.

Thanks,

Lukas

Lukas Wunner (5):
PCI: Afford direct-complete to devices with nonstandard PM
PCI: Query platform firmware for device power state
PCI: Recognize D3cold in pci_update_current_state()
PCI: Avoid unnecessary resume after direct-complete
PCI: Avoid unnecessary resume on shutdown

 drivers/pci/pci-acpi.c   | 22 ++++++++++++++++++++
 drivers/pci/pci-driver.c | 24 ++++++++++++++++++++--
 drivers/pci/pci.c        | 52 ++++++++++++++++++++++++++++++++----------------
 drivers/pci/pci.h        |  3 +++
 4 files changed, 82 insertions(+), 19 deletions(-)

Signature · 1 line

2.9.3

Lukas Wunner

author

2016-09-18 03:39 UTC #1 lore raw

PATCH v2 1/5 PCI: Afford direct-complete to devices with nonstandard PM

There are devices not power-manageable by the platform, but still able to runtime suspend to D3cold with a nonstandard mechanism. One example is laptop hybrid graphics where the discrete GPU and its built-in HDA controller are power-managed either with a _DSM (AMD PowerXpress, Nvidia Optimus) or a separate gmux controller (MacBook Pro). Another example is Thunderbolt on Macs which is power-managed with custom ACPI methods.

When putting the system to sleep, we currently handle such devices improperly by transitioning them from D3cold to D3hot (the default power state defined at the top of pci_target_state()). This wastes energy and prolongs the suspend sequence (powering up the Thunderbolt controller takes 2 seconds).

Avoid that by assuming that a nonstandard PM mechanism is at work if the device is not platform-power-manageable but currently in D3cold.

If the device is wakeup enabled, we might still have to wake it up from D3cold if PME cannot be signaled from that power state.

The check for devices without PM capability comes before the check for D3cold since such devices could in theory also be powered down by nonstandard means and should then be afforded direct-complete as well.

Signed-off-by Lukas Wunner <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

---
 drivers/pci/pci.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

drivers/pci/pci.c +15 -2

index aab9d51..72a9d3a 100644
@@ -1959,9 +1959,22 @@ static pci_power_t pci_target_state(struct pci_dev *dev)
 		default:
 			target_state = state;
 		}
-	} else if (!dev->pm_cap) {
+
+		return target_state;
+	}
+
+	if (!dev->pm_cap)
 		target_state = PCI_D0;
-	} else if (device_may_wakeup(&dev->dev)) {
+
+	/*
+	 * If the device is in D3cold even though it's not power-manageable by
+	 * the platform, it may have been powered down by nonstandard means.
+	 * Best to let it slumber.
+	 */
+	if (dev->current_state == PCI_D3cold)
+		target_state = PCI_D3cold;
+
+	if (device_may_wakeup(&dev->dev)) {
 		/*
 		 * Find the deepest state from which the device can generate
 		 * wake-up events, make it the target state and enable device

Signature · 1 line

2.9.3

Lukas Wunner

author

2016-09-18 03:39 UTC #2 lore raw

PATCH v2 2/5 PCI: Query platform firmware for device power state

In reply to Lukas Wunner

Usually the most accurate way to determine a PCI device's power state is to read its PM Control & Status Register. There are two cases however when this is not an option: If the device doesn't have the PM capability at all, or if it is in D3cold (in which case its config space is inaccessible).

In both cases, we can alternatively query the platform firmware for its opinion on the device's power state. To facilitate this, augment struct pci_platform_pm_ops with a ->get_power callback and implement it for acpi_pci_platform_pm (the only pci_platform_pm_ops existing so far).

It is used by a forthcoming commit to let pci_update_current_state() recognize D3cold.

Cc Rafael J. Wysocki <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---

Changes since v1:
* In acpi_pci_get_power_state(), only check for ACPI_STATE_UNKNOWN instead of ACPI_STATE_D0 and ACPI_STATE_D3_COLD.
* Move the change to pci_update_current_state() to new patch [3/5].

 drivers/pci/pci-acpi.c | 22 ++++++++++++++++++++++
 drivers/pci/pci.c      | 10 ++++++++--
 drivers/pci/pci.h      |  3 +++
 3 files changed, 33 insertions(+), 2 deletions(-)

drivers/pci/pci-acpi.c +22 -0

index 9a033e8..d966d47 100644
@@ -452,6 +452,27 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	return error;
 }
 
+static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
+	static const pci_power_t state_conv[] = {
+		[ACPI_STATE_D0]      = PCI_D0,
+		[ACPI_STATE_D1]      = PCI_D1,
+		[ACPI_STATE_D2]      = PCI_D2,
+		[ACPI_STATE_D3_HOT]  = PCI_D3hot,
+		[ACPI_STATE_D3_COLD] = PCI_D3cold,
+	};
+	int state;
+
+	if (!adev || !acpi_device_power_manageable(adev))
+		return PCI_UNKNOWN;
+
+	if (acpi_device_get_power(adev, &state) || state == ACPI_STATE_UNKNOWN)
+		return PCI_UNKNOWN;
+
+	return state_conv[state];
+}
+
 static bool acpi_pci_can_wakeup(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);

@@ -534,6 +555,7 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)
 static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
 	.is_manageable = acpi_pci_power_manageable,
 	.set_state = acpi_pci_set_power_state,
+	.get_state = acpi_pci_get_power_state,
 	.choose_state = acpi_pci_choose_state,
 	.sleep_wake = acpi_pci_sleep_wake,
 	.run_wake = acpi_pci_run_wake,

drivers/pci/pci.c +8 -2

index 72a9d3a..6ea0d2d 100644
@@ -552,8 +552,9 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
 
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
-	if (!ops->is_manageable || !ops->set_state || !ops->choose_state ||
-	    !ops->sleep_wake || !ops->run_wake || !ops->need_resume)
+	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
+	    !ops->choose_state  || !ops->sleep_wake || !ops->run_wake  ||
+	    !ops->need_resume)
 		return -EINVAL;
 	pci_platform_pm = ops;
 	return 0;

@@ -570,6 +571,11 @@ static inline int platform_pci_set_power_state(struct pci_dev *dev,
 	return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS;
 }
 
+static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
+{
+	return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN;
+}
+
 static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
 {
 	return pci_platform_pm ?

drivers/pci/pci.h +3 -0

index 194521b..4518562 100644
@@ -42,6 +42,8 @@ int pci_probe_reset_function(struct pci_dev *dev);
  *
  * @set_state: invokes the platform firmware to set the device's power state
  *
+ * @get_state: queries the platform firmware for a device's current power state
+ *
  * @choose_state: returns PCI power state of given device preferred by the
  *                platform; to be used during system-wide transitions from a
  *                sleeping state to the working state and vice versa

@@ -62,6 +64,7 @@ int pci_probe_reset_function(struct pci_dev *dev);
 struct pci_platform_pm_ops {
 	bool (*is_manageable)(struct pci_dev *dev);
 	int (*set_state)(struct pci_dev *dev, pci_power_t state);
+	pci_power_t (*get_state)(struct pci_dev *dev);
 	pci_power_t (*choose_state)(struct pci_dev *dev);
 	int (*sleep_wake)(struct pci_dev *dev, bool enable);
 	int (*run_wake)(struct pci_dev *dev, bool enable);

Signature · 1 line

2.9.3

Rafael J. Wysocki

Acked-by on v2 2/5

2016-09-24 00:46 UTC #3 lore raw

Re: [PATCH v2 2/5] PCI: Query platform firmware for device power state

Quoted 11 lines · Lukas Wunner

Usually the most accurate way to determine a PCI device's power state is to read its PM Control & Status Register. There are two cases however when this is not an option: If the device doesn't have the PM capability at all, or if it is in D3cold (in which case its config space is inaccessible).
In both cases, we can alternatively query the platform firmware for its opinion on the device's power state. To facilitate this, augment struct pci_platform_pm_ops with a ->get_power callback and implement it for acpi_pci_platform_pm (the only pci_platform_pm_ops existing so far).

Quoted 5 lines

It is used by a forthcoming commit to let pci_update_current_state() recognize D3cold.
Cc Rafael J. Wysocki <[email protected]>
Signed-off-by Lukas Wunner <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Quoted 102 lines

---

 drivers/pci/pci-acpi.c | 22 ++++++++++++++++++++++
 drivers/pci/pci.c      | 10 ++++++++--
 drivers/pci/pci.h      |  3 +++
 3 files changed, 33 insertions(+), 2 deletions(-)

drivers/pci/pci-acpi.c +22 -0

index 9a033e8..d966d47 100644
@@ -452,6 +452,27 @@ static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	return error;
 }
 
+static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
+	static const pci_power_t state_conv[] = {
+		[ACPI_STATE_D0]      = PCI_D0,
+		[ACPI_STATE_D1]      = PCI_D1,
+		[ACPI_STATE_D2]      = PCI_D2,
+		[ACPI_STATE_D3_HOT]  = PCI_D3hot,
+		[ACPI_STATE_D3_COLD] = PCI_D3cold,
+	};
+	int state;
+
+	if (!adev || !acpi_device_power_manageable(adev))
+		return PCI_UNKNOWN;
+
+	if (acpi_device_get_power(adev, &state) || state == ACPI_STATE_UNKNOWN)
+		return PCI_UNKNOWN;
+
+	return state_conv[state];
+}
+
 static bool acpi_pci_can_wakeup(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);

@@ -534,6 +555,7 @@ static bool acpi_pci_need_resume(struct pci_dev *dev)
 static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
 	.is_manageable = acpi_pci_power_manageable,
 	.set_state = acpi_pci_set_power_state,
+	.get_state = acpi_pci_get_power_state,
 	.choose_state = acpi_pci_choose_state,
 	.sleep_wake = acpi_pci_sleep_wake,
 	.run_wake = acpi_pci_run_wake,

drivers/pci/pci.c +8 -2

index 72a9d3a..6ea0d2d 100644
@@ -552,8 +552,9 @@ static const struct pci_platform_pm_ops *pci_platform_pm;
 
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
 {
-	if (!ops->is_manageable || !ops->set_state || !ops->choose_state ||
-	    !ops->sleep_wake || !ops->run_wake || !ops->need_resume)
+	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
+	    !ops->choose_state  || !ops->sleep_wake || !ops->run_wake  ||
+	    !ops->need_resume)
 		return -EINVAL;
 	pci_platform_pm = ops;
 	return 0;

@@ -570,6 +571,11 @@ static inline int platform_pci_set_power_state(struct pci_dev *dev,
 	return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS;
 }
 
+static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
+{
+	return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN;
+}
+
 static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
 {
 	return pci_platform_pm ?

drivers/pci/pci.h +3 -0

index 194521b..4518562 100644
@@ -42,6 +42,8 @@ int pci_probe_reset_function(struct pci_dev *dev);
  *
  * @set_state: invokes the platform firmware to set the device's power state
  *
+ * @get_state: queries the platform firmware for a device's current power state
+ *
  * @choose_state: returns PCI power state of given device preferred by the
  *                platform; to be used during system-wide transitions from a
  *                sleeping state to the working state and vice versa

@@ -62,6 +64,7 @@ int pci_probe_reset_function(struct pci_dev *dev);
 struct pci_platform_pm_ops {
 	bool (*is_manageable)(struct pci_dev *dev);
 	int (*set_state)(struct pci_dev *dev, pci_power_t state);
+	pci_power_t (*get_state)(struct pci_dev *dev);
 	pci_power_t (*choose_state)(struct pci_dev *dev);
 	int (*sleep_wake)(struct pci_dev *dev, bool enable);
 	int (*run_wake)(struct pci_dev *dev, bool enable);

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Lukas Wunner

author

2016-09-18 03:39 UTC #4 lore raw

PATCH v2 3/5 PCI: Recognize D3cold in pci_update_current_state()

In reply to Lukas Wunner

Whenever a device is resumed or its power state is changed using the platform, its new power state is read from the PM Control & Status Register and cached in pci_dev->current_state by calling pci_update_current_state().

If the device is in D3cold, reading from config space typically results in a fabricated "all ones" response. But if it's in D3hot, the two bits representing the power state in the PMCSR are *also* set to 1. Thus D3hot and D3cold are not discernible by just reading the PMCSR.

To account for this, pci_update_current_state() uses two workarounds:

- When transitioning to D3cold using pci_platform_power_transition(), the new power state is set blindly by pci_update_current_state(), i.e. without verifying that the device actually *is* in D3cold. This is achieved by setting the "state" argument to PCI_D3cold. The "state" argument was originally intended to convey the new state in case the device doesn't have the PM capability. It is *also* used to convey the device state if the PM capability is present and the new state is D3cold, but this was never explained in the kerneldoc.

- Once the current_state is set to D3cold, further invocations of pci_update_current_state() will blindly assume that the device is still in D3cold and leave the current_state unmodified. To get out of this impasse, the current_state has to be set directly, typically by calling pci_raw_set_power_state() or pci_enable_device().

It would be desirable if pci_update_current_state() could reliably detect D3cold by itself. That would allow us to do away with these workarounds, and it would allow for a smarter, more energy conserving runtime resume strategy after system sleep: Currently devices which utilize direct_complete are mandatorily runtime resumed in their ->complete stage. This can be avoided if their power state after system sleep is the same as before, but it requires a mechanism to detect the power state reliably.

We've just gained the ability to query the platform firmware for its opinion on the device's power state. On platforms conforming to ACPI 4.0 or newer, this allows recognition of D3cold. Pre-4.0 platforms lack _PR3 and therefore the deepest power state that will ever be reported is D3hot, even though the device may actually be in D3cold. To detect D3cold in those cases, accessibility of the vendor ID in config space is probed using pci_device_is_present(). This also works for devices which are not platform-power-manageable at all, but can be suspended to D3cold using a nonstandard mechanism (e.g. some hybrid graphics laptops or Thunderbolt on the Mac).

Cc Rafael J. Wysocki <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---

Changes since v1:
* Instead of solely relying on the platform firmware to report D3cold, also probe the vendor ID and assume D3cold if it can't be read. This should ensure proper detection of D3cold on pre-ACPI 4.0 platforms (which will never report anything deeper than D3hot) as well as for devices with nonstandard PM mechanisms.
* The two existing workarounds for D3cold are removed from pci_update_current_state(), as explained in the commit message.

 drivers/pci/pci.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/pci/pci.c +12 ~~-13~~

index 6ea0d2d..7d3188b 100644
@@ -707,26 +707,25 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 }
 
 /**
- * pci_update_current_state - Read PCI power state of given device from its
- *                            PCI PM registers and cache it
+ * pci_update_current_state - Read power state of given device and cache it
  * @dev: PCI device to handle.
  * @state: State to cache in case the device doesn't have the PM capability
+ *
+ * The power state is read from the PMCSR register, which however is
+ * inaccessible in D3cold.  The platform firmware is therefore queried first
+ * to detect accessibility of the register.  In case the platform firmware
+ * reports an incorrect state or the device isn't power manageable by the
+ * platform at all, we try to detect D3cold by testing accessibility of the
+ * vendor ID in config space.
  */
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
 {
-	if (dev->pm_cap) {
+	if (platform_pci_get_power_state(dev) == PCI_D3cold ||
+	    !pci_device_is_present(dev)) {
+		dev->current_state = PCI_D3cold;
+	} else if (dev->pm_cap) {
 		u16 pmcsr;
 
-		/*
-		 * Configuration space is not accessible for device in
-		 * D3cold, so just keep or set D3cold for safety
-		 */
-		if (dev->current_state == PCI_D3cold)
-			return;
-		if (state == PCI_D3cold) {
-			dev->current_state = PCI_D3cold;
-			return;
-		}
 		pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 		dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
 	} else {

Signature · 1 line

2.9.3

Rafael J. Wysocki

Acked-by on v2 3/5

2016-09-24 00:40 UTC #5 lore raw

Re: [PATCH v2 3/5] PCI: Recognize D3cold in pci_update_current_state()

Quoted 47 lines · Lukas Wunner

Whenever a device is resumed or its power state is changed using the platform, its new power state is read from the PM Control & Status Register and cached in pci_dev->current_state by calling pci_update_current_state().
If the device is in D3cold, reading from config space typically results in a fabricated "all ones" response. But if it's in D3hot, the two bits representing the power state in the PMCSR are *also* set to 1. Thus D3hot and D3cold are not discernible by just reading the PMCSR.
To account for this, pci_update_current_state() uses two workarounds:
- When transitioning to D3cold using pci_platform_power_transition(), the new power state is set blindly by pci_update_current_state(), i.e. without verifying that the device actually *is* in D3cold. This is achieved by setting the "state" argument to PCI_D3cold. The "state" argument was originally intended to convey the new state in case the device doesn't have the PM capability. It is *also* used to convey the device state if the PM capability is present and the new state is D3cold, but this was never explained in the kerneldoc.
- Once the current_state is set to D3cold, further invocations of pci_update_current_state() will blindly assume that the device is still in D3cold and leave the current_state unmodified. To get out of this impasse, the current_state has to be set directly, typically by calling pci_raw_set_power_state() or pci_enable_device().
It would be desirable if pci_update_current_state() could reliably detect D3cold by itself. That would allow us to do away with these workarounds, and it would allow for a smarter, more energy conserving runtime resume strategy after system sleep: Currently devices which utilize direct_complete are mandatorily runtime resumed in their ->complete stage. This can be avoided if their power state after system sleep is the same as before, but it requires a mechanism to detect the power state reliably.
We've just gained the ability to query the platform firmware for its opinion on the device's power state. On platforms conforming to ACPI 4.0 or newer, this allows recognition of D3cold. Pre-4.0 platforms lack _PR3 and therefore the deepest power state that will ever be reported is D3hot, even though the device may actually be in D3cold. To detect D3cold in those cases, accessibility of the vendor ID in config space is probed using pci_device_is_present(). This also works for devices which are not platform-power-manageable at all, but can be suspended to D3cold using a nonstandard mechanism (e.g. some hybrid graphics laptops or Thunderbolt on the Mac).

Quoted 2 lines

Cc Rafael J. Wysocki <[email protected]>
Signed-off-by Lukas Wunner <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Quoted 58 lines

---

 drivers/pci/pci.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/pci/pci.c +12 ~~-13~~

index 6ea0d2d..7d3188b 100644
@@ -707,26 +707,25 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 }
 
 /**
- * pci_update_current_state - Read PCI power state of given device from its
- *                            PCI PM registers and cache it
+ * pci_update_current_state - Read power state of given device and cache it
  * @dev: PCI device to handle.
  * @state: State to cache in case the device doesn't have the PM capability
+ *
+ * The power state is read from the PMCSR register, which however is
+ * inaccessible in D3cold.  The platform firmware is therefore queried first
+ * to detect accessibility of the register.  In case the platform firmware
+ * reports an incorrect state or the device isn't power manageable by the
+ * platform at all, we try to detect D3cold by testing accessibility of the
+ * vendor ID in config space.
  */
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
 {
-	if (dev->pm_cap) {
+	if (platform_pci_get_power_state(dev) == PCI_D3cold ||
+	    !pci_device_is_present(dev)) {
+		dev->current_state = PCI_D3cold;
+	} else if (dev->pm_cap) {
 		u16 pmcsr;
 
-		/*
-		 * Configuration space is not accessible for device in
-		 * D3cold, so just keep or set D3cold for safety
-		 */
-		if (dev->current_state == PCI_D3cold)
-			return;
-		if (state == PCI_D3cold) {
-			dev->current_state = PCI_D3cold;
-			return;
-		}
 		pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 		dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
 	} else {

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Lukas Wunner

author

2016-09-18 03:39 UTC #6 lore raw

PATCH v2 4/5 PCI: Avoid unnecessary resume after direct-complete

In reply to Lukas Wunner

Commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have been reset by firmware") added a runtime resume for devices that were runtime suspended when the system entered sleep.

The motivation was that devices might be in a reset-power-on state after waking from system sleep, so their power state as perceived by Linux (stored in pci_dev->current_state) would no longer reflect reality. By resuming such devices, we allow them to return to a low-power state via autosuspend and also bring their current_state in sync with reality.

However for devices that are *not* in a reset-power-on state, doing an unconditional resume wastes energy. A more refined approach is called for which issues a runtime resume only if the power state after direct-complete is shallower than it was before. To achieve this, update the device's current_state and compare it to its pre-sleep value.

Cc Rafael J. Wysocki <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/pci/pci-driver.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

drivers/pci/pci-driver.c +13 -2

index e39a67c..fd4b9c4 100644
@@ -684,8 +684,19 @@ static int pci_pm_prepare(struct device *dev)
 
 static void pci_pm_complete(struct device *dev)
 {
-	pci_dev_complete_resume(to_pci_dev(dev));
-	pm_complete_with_resume_check(dev);
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
+	pci_dev_complete_resume(pci_dev);
+	pm_generic_complete(dev);
+
+	/* Resume device if platform firmware has put it in reset-power-on */
+	if (dev->power.direct_complete && pm_resume_via_firmware()) {
+		pci_power_t pre_sleep_state = pci_dev->current_state;
+
+		pci_update_current_state(pci_dev, pci_dev->current_state);
+		if (pci_dev->current_state < pre_sleep_state)
+			pm_request_resume(dev);
+	}
 }
 
 #else /* !CONFIG_PM_SLEEP */

Signature · 1 line

2.9.3

Rafael J. Wysocki

Acked-by on v2 4/5

2016-09-24 00:40 UTC #7 lore raw

Re: [PATCH v2 4/5] PCI: Avoid unnecessary resume after direct-complete

Quoted 10 lines · Lukas Wunner

Commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have been reset by firmware") added a runtime resume for devices that were runtime suspended when the system entered sleep.
The motivation was that devices might be in a reset-power-on state after waking from system sleep, so their power state as perceived by Linux (stored in pci_dev->current_state) would no longer reflect reality. By resuming such devices, we allow them to return to a low-power state via autosuspend and also bring their current_state in sync with reality.

Quoted 8 lines

However for devices that are *not* in a reset-power-on state, doing an unconditional resume wastes energy. A more refined approach is called for which issues a runtime resume only if the power state after direct-complete is shallower than it was before. To achieve this, update the device's current_state and compare it to its pre-sleep value.
Cc Rafael J. Wysocki <[email protected]>
Signed-off-by Lukas Wunner <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Quoted 31 lines

---
 drivers/pci/pci-driver.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

drivers/pci/pci-driver.c +13 -2

index e39a67c..fd4b9c4 100644
@@ -684,8 +684,19 @@ static int pci_pm_prepare(struct device *dev)
 
 static void pci_pm_complete(struct device *dev)
 {
-	pci_dev_complete_resume(to_pci_dev(dev));
-	pm_complete_with_resume_check(dev);
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
+	pci_dev_complete_resume(pci_dev);
+	pm_generic_complete(dev);
+
+	/* Resume device if platform firmware has put it in reset-power-on */
+	if (dev->power.direct_complete && pm_resume_via_firmware()) {
+		pci_power_t pre_sleep_state = pci_dev->current_state;
+
+		pci_update_current_state(pci_dev, pci_dev->current_state);
+		if (pci_dev->current_state < pre_sleep_state)
+			pm_request_resume(dev);
+	}
 }
 
 #else /* !CONFIG_PM_SLEEP */

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Lukas Wunner

author

2016-09-18 03:39 UTC #8 lore raw

PATCH v2 5/5 PCI: Avoid unnecessary resume on shutdown

In reply to Lukas Wunner

We currently perform a mandatory runtime resume of all PCI devices on ->shutdown. However it is pointless to wake devices only to immediately power them down afterwards. (Or have the firmware reset them, in case of a reboot.)

It seems there are only two cases when a runtime resume is actually
necessary: If the driver has declared a ->shutdown callback or if kexec is in progress.

Constrain resume of a device to these cases and let it slumber otherwise, thereby conserving energy and speeding up shutdown.

To prevent the device from being runtime resumed during the remainder of the shutdown process, disable runtime PM for it.

Cc Rafael J. Wysocki <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---

Changes since v1:
* Disable runtime PM on the device to prevent it from being runtime resumed during the remainder of the shutdown process.

 drivers/pci/pci-driver.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/pci/pci-driver.c +9 -0

index fd4b9c4..f21c620 100644
@@ -459,6 +459,15 @@ static void pci_device_shutdown(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct pci_driver *drv = pci_dev->driver;
 
+	/* Fast path for suspended devices */
+	if (pm_runtime_status_suspended(dev) && (!drv || !drv->shutdown) &&
+	    !kexec_in_progress) {
+		pm_runtime_disable(dev);
+		if (pm_runtime_status_suspended(dev))
+			return;
+		pm_runtime_enable(dev);
+	}
+
 	pm_runtime_resume(dev);
 
 	if (drv && drv->shutdown)

Signature · 1 line

2.9.3

Oliver Neukum

2016-09-19 09:12 UTC #9 lore raw

Re: [PATCH v2 5/5] PCI: Avoid unnecessary resume on shutdown

Quoted 11 lines · Lukas Wunner

We currently perform a mandatory runtime resume of all PCI devices on ->shutdown. However it is pointless to wake devices only to immediately power them down afterwards. (Or have the firmware reset them, in case of a reboot.)
It seems there are only two cases when a runtime resume is actually
necessary: If the driver has declared a ->shutdown callback or if kexec is in progress.
Constrain resume of a device to these cases and let it slumber otherwise, thereby conserving energy and speeding up shutdown.

What happens if you get a wakeup event while going down? Will it lead to a reboot when a shutdown has been requested?

Regards
Oliver

Lukas Wunner

author

2016-09-19 10:13 UTC #10 lore raw

Re: [PATCH v2 5/5] PCI: Avoid unnecessary resume on shutdown

Quoted 6 lines · Oliver Neukum

Quoted 5 lines · Lukas Wunner
We currently perform a mandatory runtime resume of all PCI devices on ->shutdown. However it is pointless to wake devices only to immediately power them down afterwards. (Or have the firmware reset them, in case of a reboot.)

Quoted 8 lines

Quoted 6 lines
It seems there are only two cases when a runtime resume is actually
necessary: If the driver has declared a ->shutdown callback or if kexec is in progress.
Constrain resume of a device to these cases and let it slumber otherwise, thereby conserving energy and speeding up shutdown.
What happens if you get a wakeup event while going down?

Hm, I should probably disable that with

	__pci_enable_wake(pci_dev, pci_dev->current_state, true, false);

Quoted 1 line

Will it lead to a reboot when a shutdown has been requested?

That would depend on the platform, my guess is no, but only someone with extensive real world experience with such corner cases can answer this. (Rafael?)

Thanks,

Lukas

Rafael J. Wysocki

2016-09-24 00:43 UTC #11 lore raw

Re: [PATCH v2 5/5] PCI: Avoid unnecessary resume on shutdown

Quoted 11 lines · Lukas Wunner

Quoted 10 lines · Oliver Neukum
Quoted 9 lines · Lukas Wunner
We currently perform a mandatory runtime resume of all PCI devices on ->shutdown. However it is pointless to wake devices only to immediately power them down afterwards. (Or have the firmware reset them, in case of a reboot.)
It seems there are only two cases when a runtime resume is actually
necessary: If the driver has declared a ->shutdown callback or if kexec is in progress.

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
Constrain resume of a device to these cases and let it slumber otherwise, thereby conserving energy and speeding up shutdown.
What happens if you get a wakeup event while going down?
Hm, I should probably disable that with
	__pci_enable_wake(pci_dev, pci_dev->current_state, true, false);

What if the user wants, say, WoL to power on the system?

Quoted 6 lines

Quoted 2 lines
Will it lead to a reboot when a shutdown has been requested?
That would depend on the platform, my guess is no, but only someone with extensive real world experience with such corner cases can answer this. (Rafael?)

That wakeup will be silently dropped on the floor as a rule AFAICS.

But there's another theoretical issue here.

On systems with ACPI some devices may need to be reconfigured for wakeup from S5 even though they have been configured for signaling wakeup while in S0. Those would still need to be resumed.

Thanks,
Rafael

Lukas Wunner

author

2016-10-05 12:32 UTC #12 lore raw

Re: [PATCH v2 5/5] PCI: Avoid unnecessary resume on shutdown

Quoted 15 lines · Rafael J. Wysocki

Quoted 14 lines · Lukas Wunner
Quoted 13 lines · Oliver Neukum
Quoted 11 lines · Lukas Wunner
We currently perform a mandatory runtime resume of all PCI devices on ->shutdown. However it is pointless to wake devices only to immediately power them down afterwards. (Or have the firmware reset them, in case of a reboot.)
It seems there are only two cases when a runtime resume is actually
necessary: If the driver has declared a ->shutdown callback or if kexec is in progress.
Constrain resume of a device to these cases and let it slumber otherwise, thereby conserving energy and speeding up shutdown.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
What happens if you get a wakeup event while going down?
Hm, I should probably disable that with
	__pci_enable_wake(pci_dev, pci_dev->current_state, true, false);
What if the user wants, say, WoL to power on the system?

When the PCI device is runtime resumed, the function call above will be executed by pci_pm_runtime_resume(), so WoL will be disabled as well.

There are a few drivers which re-enable WoL from their ->shutdown hook, but that isn't impacted by this patch because the fast path isn't taken if the driver has a ->shutdown hook.

Quoted 5 lines

But there's another theoretical issue here.
On systems with ACPI some devices may need to be reconfigured for wakeup from S5 even though they have been configured for signaling wakeup while in S0. Those would still need to be resumed.

Hm, why does the device have to be resumed for that? The ACPI 6.0 spec says for _DSW:

"This control method can only access Operation Regions that are either always available while in a system working state or that are available when the Power Resources referenced by the _PRW object are all ON."

Before _DSW is executed, all power resources are turned on, so whatever _DSW does, the device should be powered up and accessible. Does OSPM really have to runtime resume the device for _DSW?

Another potential issue that crossed my mind is I/O virtualization:
If a PCI device is passed through to a guest VM, the VM may enable PME, put the device in D3, etc. If the VM is shut down, can anything go wrong if the fast path is taken for runtime suspended devices? (To be fair, if the VM crashes and doesn't shut down passed through devices orderly, we'd be in the same situation.) PME would be disabled as described above, and leaving the device in D3hot should be fine as well. If the host or another guest VM wants to use the PCI device afterwards, it will resume it to D0 with pci_enable_device() in the driver's ->probe hook. Let me know if you find flaws in this line of thought.

Thanks!

Lukas

Bjorn Helgaas

Applied

2016-09-28 16:54 UTC #13 lore raw

In reply to Lukas Wunner

Quoted 37 lines · Lukas Wunner

Here's the second installment of this series, based on feedback from Rafael. (Thanks!)
To reiterate, these refinements are meant to speed up and reduce power consumption when entering and leaving system sleep and when shutting down the system. For details, please refer to the cover letter of v1:
http://www.spinics.net/lists/linux-pci/msg53702.html
As usual I've pushed the patches to GitHub in case anyone prefers reviewing in a GUI:
https://github.com/l1k/linux/commits/pci_pm_refinements_v2
Changes since v1:
* Patch [1/5] now sports an ack by Rafael.
* Patch [2/5]:
* In acpi_pci_get_power_state(), only check for ACPI_STATE_UNKNOWN instead of ACPI_STATE_D0 and ACPI_STATE_D3_COLD.
* Move the change to pci_update_current_state() to new patch [3/5].
* Patch [3/5]:
* Instead of solely relying on the platform firmware to report D3cold, also probe the vendor ID and assume D3cold if it can't be read. This should ensure proper detection of D3cold on pre-ACPI 4.0 platforms (which will never report anything deeper than D3hot) as well as for devices with nonstandard PM mechanisms.
* The two existing workarounds for D3cold are removed from pci_update_current_state(), as explained in the commit message.
* Patch [5/5]:
* Disable runtime PM on the device to prevent it from being runtime resumed during the remainder of the shutdown process.
Thanks,

Quoted 8 lines

Lukas
Lukas Wunner (5):
PCI: Afford direct-complete to devices with nonstandard PM
PCI: Query platform firmware for device power state
PCI: Recognize D3cold in pci_update_current_state()
PCI: Avoid unnecessary resume after direct-complete

I applied the patches above with Rafael's acks to pci/pm for v4.9, thanks, Lukas! I particularly appreciate your changelogs -- they're very readable, complete, and consistent in style.

Quoted 1 line

PCI: Avoid unnecessary resume on shutdown

Sounds like there's still a little discussion on this, so I haven't applied this one yet.

Quoted 13 lines

 drivers/pci/pci-acpi.c   | 22 ++++++++++++++++++++
 drivers/pci/pci-driver.c | 24 ++++++++++++++++++++--
 drivers/pci/pci.c        | 52 ++++++++++++++++++++++++++++++++----------------
 drivers/pci/pci.h        |  3 +++
 4 files changed, 82 insertions(+), 19 deletions(-)

Signature · 6 lines

2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lukas Wunner

author

2016-09-29 12:10 UTC #14 lore raw

Quoted 9 lines · Bjorn Helgaas

Quoted 4 lines · Lukas Wunner
PCI: Afford direct-complete to devices with nonstandard PM
PCI: Query platform firmware for device power state
PCI: Recognize D3cold in pci_update_current_state()
PCI: Avoid unnecessary resume after direct-complete
I applied the patches above with Rafael's acks to pci/pm for v4.9, thanks, Lukas! I particularly appreciate your changelogs -- they're very readable, complete, and consistent in style.

As a non-native English speaker I cannot imagine a bigger compliment, thanks so much!

Quoted 4 lines

Quoted 1 line
PCI: Avoid unnecessary resume on shutdown
Sounds like there's still a little discussion on this, so I haven't applied this one yet.

Yes, indeed I need more time with this one.

Thanks!

Lukas