On Mon, Apr 25, 2022 at 8:33 PM David E. Box
[off-list ref] wrote:

On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:

quoted

On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:

quoted

quoted

-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:

quoted

On 3/25/2022 8:50 PM, Rajvi Jingar wrote:

quoted

For the PCIe devices (like nvme) that do not go into D3 state still
need to
disable PTM on PCIe root ports to allow the port to enter a lower-
power PM
state and the SoC to reach a lower-power idle state as a whole. Move
the
pci_disable_ptm() out of pci_prepare_to_sleep() as this code path is
not
followed for devices that do not go into D3. This patch fixes the
issue
seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530 with
Coffee
Lake CPU platforms to get improved residency in low power idle states.

Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
Signed-off-by: Rajvi Jingar <redacted>
Suggested-by: David E. Box <david.e.box@linux.intel.com>
---
  drivers/pci/pci-driver.c | 10 ++++++++++
  drivers/pci/pci.c        | 10 ----------
  2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 8b55a90126a2..ab733374a260 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c

@@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device

*dev)
      if (!pci_dev->state_saved) {
              pci_save_state(pci_dev);
+             /*
+              * There are systems (for example, Intel mobile chips
since

Coffee

quoted

quoted

+              * Lake) where the power drawn while suspended can be

significantly

quoted

quoted

+              * reduced by disabling PTM on PCIe root ports as this
allows the
+              * port to enter a lower-power PM state and the SoC to
reach a
+              * lower-power idle state as a whole.
+              */
+             if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
+                     pci_disable_ptm(pci_dev);

Why is disabling PTM dependent on pci_dev->state_saved?  The point of
this is to change the behavior of the device, and it seems like we
want to do that regardless of whether the driver has used
pci_save_state().

Because we use the saved state to restore PTM on the root port.
And it's under this condition that the root port state gets saved.

Yes, I understand that pci_restore_ptm_state() depends on a previous
call to pci_save_ptm_state().

The point I'm trying to make is that pci_disable_ptm() changes the
state of the device, and that state change should not depend on
whether the driver has used pci_save_state().

We do it here because D3 depends on whether the device state was saved by the
driver.

        if (!pci_dev->state_saved) {
                pci_save_state(pci_dev);

                /* disable PTM here */

                if (pci_power_manageable(pci_dev))
                        pci_prepare_to_sleep(pci_dev);
        }


If we disable PTM before the check, we will have saved "PTM disabled" as the
restore state. And we can't do it after the check as the device will be in D3.

As to disabling PTM on all devices, I see no problem with this, but the
reasoning is different. We disabled the root port PTM for power savings.

Right.  As per the comment explaining why it is disabled.

quoted

When we're putting a device into a low-power state, I think we want to
disable PTM *always*, no matter what the driver did.  And I think we
want to do it for all devices, not just Root Ports.

Bjorn