diff mbox series

PCI: PM: Avoid skipping bus-level PM on platforms without ACPI

Message ID 14605632.7Eqku7tdey@kreacher (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI: PM: Avoid skipping bus-level PM on platforms without ACPI | expand

Commit Message

Rafael J. Wysocki June 25, 2019, 10:20 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There are platforms that do not call pm_set_suspend_via_firmware(),
so pm_suspend_via_firmware() returns 'false' on them, but the power
states of PCI devices (PCIe ports in particular) are changed as a
result of powering down core platform components during system-wide
suspend.  Thus the pm_suspend_via_firmware() checks in
pci_pm_suspend_noirq() and pci_pm_resume_noirq() introduced by
commit 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-
idle") are not sufficient to determine that devices left in D0
during suspend will remain in D0 during resume and so the bus-level
power management can be skipped for them.

For this reason, introduce a new global suspend flag,
PM_SUSPEND_FLAG_NO_PLATFORM, set it for suspend-to-idle only
and replace the pm_suspend_via_firmware() checks mentioned above
with checks against this flag.

Fixes: 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |    8 ++++----
 include/linux/suspend.h  |   26 ++++++++++++++++++++++++--
 kernel/power/suspend.c   |    3 +++
 3 files changed, 31 insertions(+), 6 deletions(-)

Comments

Mika Westerberg June 26, 2019, 12:56 p.m. UTC | #1
On Wed, Jun 26, 2019 at 12:20:23AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There are platforms that do not call pm_set_suspend_via_firmware(),
> so pm_suspend_via_firmware() returns 'false' on them, but the power
> states of PCI devices (PCIe ports in particular) are changed as a
> result of powering down core platform components during system-wide
> suspend.  Thus the pm_suspend_via_firmware() checks in
> pci_pm_suspend_noirq() and pci_pm_resume_noirq() introduced by
> commit 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-
> idle") are not sufficient to determine that devices left in D0
> during suspend will remain in D0 during resume and so the bus-level
> power management can be skipped for them.
> 
> For this reason, introduce a new global suspend flag,
> PM_SUSPEND_FLAG_NO_PLATFORM, set it for suspend-to-idle only
> and replace the pm_suspend_via_firmware() checks mentioned above
> with checks against this flag.
> 
> Fixes: 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I tested this patch on top of your (and mine) previous patches touching
the ACPI/PCI PM and did not see any issues over several suspend-to-idle
cycles with and without TBT device connected.

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Rafael J. Wysocki June 26, 2019, 9:52 p.m. UTC | #2
On Wed, Jun 26, 2019 at 2:56 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Wed, Jun 26, 2019 at 12:20:23AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There are platforms that do not call pm_set_suspend_via_firmware(),
> > so pm_suspend_via_firmware() returns 'false' on them, but the power
> > states of PCI devices (PCIe ports in particular) are changed as a
> > result of powering down core platform components during system-wide
> > suspend.  Thus the pm_suspend_via_firmware() checks in
> > pci_pm_suspend_noirq() and pci_pm_resume_noirq() introduced by
> > commit 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-
> > idle") are not sufficient to determine that devices left in D0
> > during suspend will remain in D0 during resume and so the bus-level
> > power management can be skipped for them.
> >
> > For this reason, introduce a new global suspend flag,
> > PM_SUSPEND_FLAG_NO_PLATFORM, set it for suspend-to-idle only
> > and replace the pm_suspend_via_firmware() checks mentioned above
> > with checks against this flag.
> >
> > Fixes: 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")
> > Reported-by: Jon Hunter <jonathanh@nvidia.com>
> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> I tested this patch on top of your (and mine) previous patches touching
> the ACPI/PCI PM and did not see any issues over several suspend-to-idle
> cycles with and without TBT device connected.
>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!
diff mbox series

Patch

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -209,8 +209,9 @@  extern int suspend_valid_only_mem(suspen
 
 extern unsigned int pm_suspend_global_flags;
 
-#define PM_SUSPEND_FLAG_FW_SUSPEND	(1 << 0)
-#define PM_SUSPEND_FLAG_FW_RESUME	(1 << 1)
+#define PM_SUSPEND_FLAG_FW_SUSPEND	BIT(0)
+#define PM_SUSPEND_FLAG_FW_RESUME	BIT(1)
+#define PM_SUSPEND_FLAG_NO_PLATFORM	BIT(2)
 
 static inline void pm_suspend_clear_flags(void)
 {
@@ -227,6 +228,11 @@  static inline void pm_set_resume_via_fir
 	pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME;
 }
 
+static inline void pm_set_suspend_no_platform(void)
+{
+	pm_suspend_global_flags |= PM_SUSPEND_FLAG_NO_PLATFORM;
+}
+
 /**
  * pm_suspend_via_firmware - Check if platform firmware will suspend the system.
  *
@@ -268,6 +274,22 @@  static inline bool pm_resume_via_firmwar
 	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME);
 }
 
+/**
+ * pm_suspend_no_platform - Check if platform may change device power states.
+ *
+ * To be called during system-wide power management transitions to sleep states
+ * or during the subsequent system-wide transitions back to the working state.
+ *
+ * Return 'true' if the power states of devices remain under full control of the
+ * kernel throughout the system-wide suspend and resume cycle in progress (that
+ * is, if a device is put into a certain power state during suspend, it can be
+ * expected to remain in that state during resume).
+ */
+static inline bool pm_suspend_no_platform(void)
+{
+	return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_NO_PLATFORM);
+}
+
 /* Suspend-to-idle state machnine. */
 enum s2idle_states {
 	S2IDLE_STATE_NONE,      /* Not suspended/suspending. */
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -493,6 +493,9 @@  int suspend_devices_and_enter(suspend_st
 
 	pm_suspend_target_state = state;
 
+	if (state == PM_SUSPEND_TO_IDLE)
+		pm_set_suspend_no_platform();
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -877,7 +877,7 @@  static int pci_pm_suspend_noirq(struct d
 			pci_dev->bus->self->skip_bus_pm = true;
 	}
 
-	if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) {
+	if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
 		dev_dbg(dev, "PCI PM: Skipped\n");
 		goto Fixup;
 	}
@@ -932,10 +932,10 @@  static int pci_pm_resume_noirq(struct de
 	/*
 	 * In the suspend-to-idle case, devices left in D0 during suspend will
 	 * stay in D0, so it is not necessary to restore or update their
-	 * configuration here and attempting to put them into D0 again may
-	 * confuse some firmware, so avoid doing that.
+	 * configuration here and attempting to put them into D0 again is
+	 * pointless, so avoid doing that.
 	 */
-	if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware())
+	if (!(pci_dev->skip_bus_pm && pm_suspend_no_platform()))
 		pci_pm_default_resume_early(pci_dev);
 
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);