[PATCH 0/6] PM: PCI/ACPI: Hibernation handling fixes

[PATCH 0/6] PM: PCI/ACPI: Hibernation handling fixes

[Rafael J. Wysocki]

Hi All,

This series of patches addresses a few issues related to the handling of
hibernation in the PCI bus type and the ACPI PM domain and ACPI LPSS driver.

First of all, all of the runtime-suspended PCI devices and devices in the ACPI PM and LPSS
PM domains will be resumed during hibernation (first patch).  This appears to be the
only way to avoid weird corner cases and the benefit from avoiding to resume those
devices during hibernation is questionable.

That change allows the the hibernation callbacks in all of the involved subsystems to be
simplified (patches 2 and 3).

While at it, there is a subtle issue in the LPSS suspend callbacks which is addressed
by patch 4.

Moreover, reusing bus-level suspend callbacks for the "poweroff" transition during
hibernation (which is the case for the ACPI PM domain and LPSS) is incorrect, so patch 5
fixes that.

Finally, there are some leftover items in linux/acpi.h that can be dropped (patch 6).

Thanks,
Rafael

[PATCH 1/6] PM: ACPI/PCI: Resume all devices during hibernation

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Both the PCI bus type and the ACPI PM domain avoid resuming
runtime-suspended devices with DPM_FLAG_SMART_SUSPEND set during
hibernation (before creating the snapshot image of system memory),
but that turns out to be a mistake.  It leads to functional issues
and adds complexity that's hard to justify.

For this reason, resume all runtime-suspended PCI devices and all
devices in the ACPI PM domains before creating a snapshot image of
system memory during hibernation.

Fixes: 05087360fd7a (ACPI / PM: Take SMART_SUSPEND driver flag into account)
Fixes: c4b65157aeef (PCI / PM: Take SMART_SUSPEND driver flag into account)
Link: https://lore.kernel.org/linux-acpi/917d4399-2e22-67b1-9d54-808561f9083f@uwyo.edu/T/#maf065fe6e4974f2a9d79f332ab99dfaba635f64c
Reported-by: Robert R. Howell <RHowell@uwyo.edu>
Tested-by: Robert R. Howell <RHowell@uwyo.edu>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   13 +++++++------
 drivers/pci/pci-driver.c |   16 ++++++++--------
 2 files changed, 15 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -1155,13 +1155,14 @@ EXPORT_SYMBOL_GPL(acpi_subsys_resume_ear
 int acpi_subsys_freeze(struct device *dev)
 {
 	/*
-	 * This used to be done in acpi_subsys_prepare() for all devices and
-	 * some drivers may depend on it, so do it here.  Ideally, however,
-	 * runtime-suspended devices should not be touched during freeze/thaw
-	 * transitions.
+	 * Resume all runtime-suspended devices before creating a snapshot
+	 * image of system memory, because the restore kernel generally cannot
+	 * be expected to always handle them consistently and they need to be
+	 * put into the runtime-active metastate during system resume anyway,
+	 * so it is better to ensure that the state saved in the image will be
+	 * alwyas consistent with that.
 	 */
-	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
-		pm_runtime_resume(dev);
+	pm_runtime_resume(dev);
 
 	return pm_generic_freeze(dev);
 }
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1012,15 +1012,15 @@ static int pci_pm_freeze(struct device *
 	}
 
 	/*
-	 * This used to be done in pci_pm_prepare() for all devices and some
-	 * drivers may depend on it, so do it here.  Ideally, runtime-suspended
-	 * devices should not be touched during freeze/thaw transitions,
-	 * however.
+	 * Resume all runtime-suspended devices before creating a snapshot
+	 * image of system memory, because the restore kernel generally cannot
+	 * be expected to always handle them consistently and they need to be
+	 * put into the runtime-active metastate during system resume anyway,
+	 * so it is better to ensure that the state saved in the image will be
+	 * alwyas consistent with that.
 	 */
-	if (!dev_pm_smart_suspend_and_suspended(dev)) {
-		pm_runtime_resume(dev);
-		pci_dev->state_saved = false;
-	}
+	pm_runtime_resume(dev);
+	pci_dev->state_saved = false;
 
 	if (pm->freeze) {
 		int error;

[PATCH 2/6] PCI: PM: Simplify bus-level hibernation callbacks

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After a previous change causing all runtime-suspended PCI devices
to be resumed before creating a snapshot image of memory during
hibernation, it is not necessary to worry about the case in which
them might be left in runtime-suspend any more, so get rid of the
code related to that from bus-level PCI hibernation callbacks.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |   27 ---------------------------
 1 file changed, 27 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1034,22 +1034,11 @@ static int pci_pm_freeze(struct device *
 	return 0;
 }
 
-static int pci_pm_freeze_late(struct device *dev)
-{
-	if (dev_pm_smart_suspend_and_suspended(dev))
-		return 0;
-
-	return pm_generic_freeze_late(dev);
-}
-
 static int pci_pm_freeze_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct device_driver *drv = dev->driver;
 
-	if (dev_pm_smart_suspend_and_suspended(dev))
-		return 0;
-
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend_late(dev, PMSG_FREEZE);
 
@@ -1079,16 +1068,6 @@ static int pci_pm_thaw_noirq(struct devi
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	/*
-	 * If the device is in runtime suspend, the code below may not work
-	 * correctly with it, so skip that code and make the PM core skip all of
-	 * the subsequent "thaw" callbacks for the device.
-	 */
-	if (dev_pm_smart_suspend_and_suspended(dev)) {
-		dev_pm_skip_next_resume_phases(dev);
-		return 0;
-	}
-
 	if (pcibios_pm_ops.thaw_noirq) {
 		error = pcibios_pm_ops.thaw_noirq(dev);
 		if (error)
@@ -1226,10 +1205,6 @@ static int pci_pm_restore_noirq(struct d
 	struct device_driver *drv = dev->driver;
 	int error = 0;
 
-	/* This is analogous to the pci_pm_resume_noirq() case. */
-	if (dev_pm_smart_suspend_and_suspended(dev))
-		pm_runtime_set_active(dev);
-
 	if (pcibios_pm_ops.restore_noirq) {
 		error = pcibios_pm_ops.restore_noirq(dev);
 		if (error)
@@ -1279,7 +1254,6 @@ static int pci_pm_restore(struct device
 #else /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #define pci_pm_freeze		NULL
-#define pci_pm_freeze_late	NULL
 #define pci_pm_freeze_noirq	NULL
 #define pci_pm_thaw		NULL
 #define pci_pm_thaw_noirq	NULL
@@ -1405,7 +1379,6 @@ static const struct dev_pm_ops pci_dev_p
 	.suspend_late = pci_pm_suspend_late,
 	.resume = pci_pm_resume,
 	.freeze = pci_pm_freeze,
-	.freeze_late = pci_pm_freeze_late,
 	.thaw = pci_pm_thaw,
 	.poweroff = pci_pm_poweroff,
 	.poweroff_late = pci_pm_poweroff_late,

[PATCH 3/6] ACPI: PM: Simplify and fix PM domain hibernation callbacks

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

First, after a previous change causing all runtime-suspended devices
in the ACPI PM domain (and ACPI LPSS devices) to be resumed before
creating a snapshot image of memory during hibernation, it is not
necessary to worry about the case in which them might be left in
runtime-suspend any more, so get rid of the code related to that from
ACPI PM domain and ACPI LPSS hibernation callbacks.

Second, it is not correct to use pm_generic_resume_early() and
acpi_subsys_resume_noirq() in hibernation "restore" callbacks (which
currently happens in the ACPI PM domain and ACPI LPSS), so introduce
proper _restore_late and _restore_noirq callbacks for the ACPI PM
domain and ACPI LPSS.

Fixes: 05087360fd7a (ACPI / PM: Take SMART_SUSPEND driver flag into account)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_lpss.c |   59 +++++++++++++++++++++++++++++++++------------
 drivers/acpi/device_pm.c |   61 ++++++-----------------------------------------
 include/linux/acpi.h     |   10 -------
 3 files changed, 52 insertions(+), 78 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -1069,38 +1069,68 @@ static int acpi_lpss_suspend_noirq(struc
 	return acpi_subsys_suspend_noirq(dev);
 }
 
-static int acpi_lpss_do_resume_early(struct device *dev)
+static int acpi_lpss_resume_noirq(struct device *dev)
 {
-	int ret = acpi_lpss_resume(dev);
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+	int ret;
+
+	/* Follow acpi_subsys_resune_noirq(). */
+	if (dev_pm_may_skip_resume(dev))
+		return 0;
 
-	return ret ? ret : pm_generic_resume_early(dev);
+	if (dev_pm_smart_suspend_and_suspended(dev))
+		pm_runtime_set_active(dev);
+
+	ret = pm_generic_resume_noirq(dev);
+	if (ret)
+		return ret;
+
+	if (pdata->dev_desc->resume_from_noirq)
+		return acpi_lpss_resume(dev);
+
+	return 0;
 }
 
 static int acpi_lpss_resume_early(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 
-	if (pdata->dev_desc->resume_from_noirq)
-		return 0;
+	if (!pdata->dev_desc->resume_from_noirq) {
+		int ret = acpi_lpss_resume(dev);
+		if (ret)
+			return ret;
+	}
 
-	return acpi_lpss_do_resume_early(dev);
+	return pm_generic_resume_early(dev);
 }
 
-static int acpi_lpss_resume_noirq(struct device *dev)
+static int acpi_lpss_restore_noirq(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
 
-	ret = acpi_subsys_resume_noirq(dev);
+	ret = pm_generic_restore_noirq(dev);
 	if (ret)
 		return ret;
 
-	if (!dev_pm_may_skip_resume(dev) && pdata->dev_desc->resume_from_noirq)
-		ret = acpi_lpss_do_resume_early(dev);
+	if (pdata->dev_desc->resume_from_noirq)
+		return acpi_lpss_resume(dev);
 
-	return ret;
+	return 0;
 }
 
+static int acpi_lpss_restore_early(struct device *dev)
+{
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (!pdata->dev_desc->resume_from_noirq) {
+		int ret = acpi_lpss_resume(dev);
+		if (ret)
+			return ret;
+	}
+
+	return pm_generic_restore_early(dev);
+}
 #endif /* CONFIG_PM_SLEEP */
 
 static int acpi_lpss_runtime_suspend(struct device *dev)
@@ -1134,14 +1164,11 @@ static struct dev_pm_domain acpi_lpss_pm
 		.resume_noirq = acpi_lpss_resume_noirq,
 		.resume_early = acpi_lpss_resume_early,
 		.freeze = acpi_subsys_freeze,
-		.freeze_late = acpi_subsys_freeze_late,
-		.freeze_noirq = acpi_subsys_freeze_noirq,
-		.thaw_noirq = acpi_subsys_thaw_noirq,
 		.poweroff = acpi_subsys_suspend,
 		.poweroff_late = acpi_lpss_suspend_late,
 		.poweroff_noirq = acpi_lpss_suspend_noirq,
-		.restore_noirq = acpi_lpss_resume_noirq,
-		.restore_early = acpi_lpss_resume_early,
+		.restore_noirq = acpi_lpss_restore_noirq,
+		.restore_early = acpi_lpss_restore_early,
 #endif
 		.runtime_suspend = acpi_lpss_runtime_suspend,
 		.runtime_resume = acpi_lpss_runtime_resume,
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -1116,7 +1116,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend_no
  * acpi_subsys_resume_noirq - Run the device driver's "noirq" resume callback.
  * @dev: Device to handle.
  */
-int acpi_subsys_resume_noirq(struct device *dev)
+static int acpi_subsys_resume_noirq(struct device *dev)
 {
 	if (dev_pm_may_skip_resume(dev))
 		return 0;
@@ -1131,7 +1131,6 @@ int acpi_subsys_resume_noirq(struct devi
 
 	return pm_generic_resume_noirq(dev);
 }
-EXPORT_SYMBOL_GPL(acpi_subsys_resume_noirq);
 
 /**
  * acpi_subsys_resume_early - Resume device using ACPI.
@@ -1141,12 +1140,11 @@ EXPORT_SYMBOL_GPL(acpi_subsys_resume_noi
  * generic early resume procedure for it during system transition into the
  * working state.
  */
-int acpi_subsys_resume_early(struct device *dev)
+static int acpi_subsys_resume_early(struct device *dev)
 {
 	int ret = acpi_dev_resume(dev);
 	return ret ? ret : pm_generic_resume_early(dev);
 }
-EXPORT_SYMBOL_GPL(acpi_subsys_resume_early);
 
 /**
  * acpi_subsys_freeze - Run the device driver's freeze callback.
@@ -1169,52 +1167,15 @@ int acpi_subsys_freeze(struct device *de
 EXPORT_SYMBOL_GPL(acpi_subsys_freeze);
 
 /**
- * acpi_subsys_freeze_late - Run the device driver's "late" freeze callback.
- * @dev: Device to handle.
- */
-int acpi_subsys_freeze_late(struct device *dev)
-{
-
-	if (dev_pm_smart_suspend_and_suspended(dev))
-		return 0;
-
-	return pm_generic_freeze_late(dev);
-}
-EXPORT_SYMBOL_GPL(acpi_subsys_freeze_late);
-
-/**
- * acpi_subsys_freeze_noirq - Run the device driver's "noirq" freeze callback.
- * @dev: Device to handle.
- */
-int acpi_subsys_freeze_noirq(struct device *dev)
-{
-
-	if (dev_pm_smart_suspend_and_suspended(dev))
-		return 0;
-
-	return pm_generic_freeze_noirq(dev);
-}
-EXPORT_SYMBOL_GPL(acpi_subsys_freeze_noirq);
-
-/**
- * acpi_subsys_thaw_noirq - Run the device driver's "noirq" thaw callback.
- * @dev: Device to handle.
+ * acpi_subsys_restore_early - Restore device using ACPI.
+ * @dev: Device to restore.
  */
-int acpi_subsys_thaw_noirq(struct device *dev)
+int acpi_subsys_restore_early(struct device *dev)
 {
-	/*
-	 * If the device is in runtime suspend, the "thaw" code may not work
-	 * correctly with it, so skip the driver callback and make the PM core
-	 * skip all of the subsequent "thaw" callbacks for the device.
-	 */
-	if (dev_pm_smart_suspend_and_suspended(dev)) {
-		dev_pm_skip_next_resume_phases(dev);
-		return 0;
-	}
-
-	return pm_generic_thaw_noirq(dev);
+	int ret = acpi_dev_resume(dev);
+	return ret ? ret : pm_generic_restore_early(dev);
 }
-EXPORT_SYMBOL_GPL(acpi_subsys_thaw_noirq);
+EXPORT_SYMBOL_GPL(acpi_subsys_restore_early);
 #endif /* CONFIG_PM_SLEEP */
 
 static struct dev_pm_domain acpi_general_pm_domain = {
@@ -1230,14 +1191,10 @@ static struct dev_pm_domain acpi_general
 		.resume_noirq = acpi_subsys_resume_noirq,
 		.resume_early = acpi_subsys_resume_early,
 		.freeze = acpi_subsys_freeze,
-		.freeze_late = acpi_subsys_freeze_late,
-		.freeze_noirq = acpi_subsys_freeze_noirq,
-		.thaw_noirq = acpi_subsys_thaw_noirq,
 		.poweroff = acpi_subsys_suspend,
 		.poweroff_late = acpi_subsys_suspend_late,
 		.poweroff_noirq = acpi_subsys_suspend_noirq,
-		.restore_noirq = acpi_subsys_resume_noirq,
-		.restore_early = acpi_subsys_resume_early,
+		.restore_early = acpi_subsys_restore_early,
 #endif
 	},
 };
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -918,26 +918,16 @@ int acpi_subsys_prepare(struct device *d
 void acpi_subsys_complete(struct device *dev);
 int acpi_subsys_suspend_late(struct device *dev);
 int acpi_subsys_suspend_noirq(struct device *dev);
-int acpi_subsys_resume_noirq(struct device *dev);
-int acpi_subsys_resume_early(struct device *dev);
 int acpi_subsys_suspend(struct device *dev);
 int acpi_subsys_freeze(struct device *dev);
-int acpi_subsys_freeze_late(struct device *dev);
-int acpi_subsys_freeze_noirq(struct device *dev);
-int acpi_subsys_thaw_noirq(struct device *dev);
 #else
 static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
 static inline void acpi_subsys_complete(struct device *dev) {}
 static inline int acpi_subsys_suspend_late(struct device *dev) { return 0; }
 static inline int acpi_subsys_suspend_noirq(struct device *dev) { return 0; }
-static inline int acpi_subsys_resume_noirq(struct device *dev) { return 0; }
-static inline int acpi_subsys_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_suspend(struct device *dev) { return 0; }
 static inline int acpi_subsys_freeze(struct device *dev) { return 0; }
-static inline int acpi_subsys_freeze_late(struct device *dev) { return 0; }
-static inline int acpi_subsys_freeze_noirq(struct device *dev) { return 0; }
-static inline int acpi_subsys_thaw_noirq(struct device *dev) { return 0; }
 #endif
 
 #ifdef CONFIG_ACPI

[PATCH 4/6] ACPI: LPSS: Fix ->suspend_late callbacks handling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If the resume_from_noirq flag is set in dev_desc, the ->suspend_late
callback provided by the device driver will be invoked at the "noirq"
stage of system suspend, via acpi_lpss_do_suspend_late(), which is
incorrect.

To fix that, drop acpi_lpss_do_suspend_late() and rearrange
acpi_lpss_suspend_late() to call pm_generic_suspend_late()
directly, before calling acpi_lpss_suspend(), in analogy with
acpi_subsys_suspend_late().

Also notice that acpi_subsys_suspend_late() is not used outside
of the file where it is defined, so make it static and drop its
header for the header file containing it.

Fixes: 48402cee6889 (ACPI / LPSS: Resume BYT/CHT I2C controllers from resume_noirq)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_lpss.c |   23 ++++++++++-------------
 drivers/acpi/device_pm.c |    3 +--
 include/linux/acpi.h     |    2 --
 3 files changed, 11 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -1034,34 +1034,31 @@ static int acpi_lpss_resume(struct devic
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int acpi_lpss_do_suspend_late(struct device *dev)
+static int acpi_lpss_suspend_late(struct device *dev)
 {
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
 
 	if (dev_pm_smart_suspend_and_suspended(dev))
 		return 0;
 
 	ret = pm_generic_suspend_late(dev);
-	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
-}
+	if (ret)
+		return ret;
 
-static int acpi_lpss_suspend_late(struct device *dev)
-{
-	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
-
-	if (pdata->dev_desc->resume_from_noirq)
-		return 0;
+	if (!pdata->dev_desc->resume_from_noirq)
+		return acpi_lpss_suspend(dev, device_may_wakeup(dev));
 
-	return acpi_lpss_do_suspend_late(dev);
+	return 0;
 }
 
 static int acpi_lpss_suspend_noirq(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
-	int ret;
 
-	if (pdata->dev_desc->resume_from_noirq) {
-		ret = acpi_lpss_do_suspend_late(dev);
+	if (!dev_pm_smart_suspend_and_suspended(dev) &&
+	    pdata->dev_desc->resume_from_noirq) {
+		int ret = acpi_lpss_suspend(dev, device_may_wakeup(dev));
 		if (ret)
 			return ret;
 	}
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -1069,7 +1069,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
  * Carry out the generic late suspend procedure for @dev and use ACPI to put
  * it into a low-power state during system transition into a sleep state.
  */
-int acpi_subsys_suspend_late(struct device *dev)
+static int acpi_subsys_suspend_late(struct device *dev)
 {
 	int ret;
 
@@ -1079,7 +1079,6 @@ int acpi_subsys_suspend_late(struct devi
 	ret = pm_generic_suspend_late(dev);
 	return ret ? ret : acpi_dev_suspend(dev, device_may_wakeup(dev));
 }
-EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late);
 
 /**
  * acpi_subsys_suspend_noirq - Run the device driver's "noirq" suspend callback.
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -916,7 +916,6 @@ static inline int acpi_dev_pm_attach(str
 int acpi_dev_suspend_late(struct device *dev);
 int acpi_subsys_prepare(struct device *dev);
 void acpi_subsys_complete(struct device *dev);
-int acpi_subsys_suspend_late(struct device *dev);
 int acpi_subsys_suspend_noirq(struct device *dev);
 int acpi_subsys_suspend(struct device *dev);
 int acpi_subsys_freeze(struct device *dev);
@@ -924,7 +923,6 @@ int acpi_subsys_freeze(struct device *de
 static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
 static inline void acpi_subsys_complete(struct device *dev) {}
-static inline int acpi_subsys_suspend_late(struct device *dev) { return 0; }
 static inline int acpi_subsys_suspend_noirq(struct device *dev) { return 0; }
 static inline int acpi_subsys_suspend(struct device *dev) { return 0; }
 static inline int acpi_subsys_freeze(struct device *dev) { return 0; }

Re: [PATCH 4/6] ACPI: LPSS: Fix ->suspend_late callbacks handling

[Hans de Goede]

Hi Rafael,

On 29-06-19 11:50, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If the resume_from_noirq flag is set in dev_desc, the ->suspend_late
> callback provided by the device driver will be invoked at the "noirq"
> stage of system suspend, via acpi_lpss_do_suspend_late(), which is
> incorrect.
> 
> To fix that, drop acpi_lpss_do_suspend_late() and rearrange
> acpi_lpss_suspend_late() to call pm_generic_suspend_late()
> directly, before calling acpi_lpss_suspend(), in analogy with
> acpi_subsys_suspend_late().

Ah now I see the logic in your previous test-patch.

I'm afraid that this is going to break things though, the calling
of the device-driver's suspend-late method at noirq time is
*intentional* !

The resume_from_noirq flag is only set for i2c controllers which
use: drivers/i2c/busses/i2c-designware-platdrv.c as driver.

This driver's suspend late method looks like this:

static int dw_i2c_plat_suspend(struct device *dev)
{
         struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);

         i_dev->suspended = true;

         if (i_dev->shared_with_punit)
                 return 0;

         i_dev->disable(i_dev);
         i2c_dw_prepare_clk(i_dev, false);

         return 0;
}

The i_dev->disable(i_dev) and i2c_dw_prepare_clk(i_dev, false) calls here
will make the i2c controller non functional. But (some of) these i2c
controllers are used by code in the _PS0  / _PS3 methods of some PCI
devices and the PCI core calls _PS0 / _PS3 at *noirq* time, so as explained
in the commit message which introduced acpi_lpss_do_suspend_late():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=48402cee6889fb3fce58e95fea1471626286dc63

We must not only make sure that the suspending of the i2c controller is
ordered so that it happens after these PCI devices are suspended, we must
also make sure that the i2c controller stays functional until the
i2c-controller is put in the suspend-noirq state.

If you really want to go this route, we must duplicate the resume_from_noirq
flag inside drivers/i2c/busses/i2c-designware-platdrv.c, setting it
only for acpi_lpss enumerated devices (the driver handles a whole lot more
devices) ans then make the driver's suspend_late method a no-op and instead
to the suspend from its suspend_noirq callback.

Since pm_generic_suspend_late() is just a wrapper to call dev->driver->pm->suspend_late
duplicating the resume_from_noirq flag inside i2c-designware-platdrv.c seems
unproductive.

Note that we have the same thing going on in acpi_lpss.c with resume_early vs
resume_noirq, we call the resume_early callback from acpi_lpss_resume_noirq
if the resume_from_noirq flag is set.

TL;DR: the behavior you are trying to fix here is intentional and
IMHO this patch should be dropped.

I guess we could / should do a patch adding a comment that the calling
the drivers' suspend_late / resume_early callback at noirq time is intentional
to avoid this confusing people in the future.

###

Patches 1-3 and 6 look good to me and you may add my:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
to those.

The comments from this patch also impact patch 5 "ACPI: PM: Introduce "poweroff" callbacks for ACPI PM domain and LPSS"
that should be rewritten to call the device's poweroff_late method at noirq
time if the resume_from_noirq flag is set.

I wonder if patch 5 is necessary though, the acpi_lpss code only deals with a small set
of devices, at least i2c-designware-platdrv.c uses SET_LATE_SYSTEM_SLEEP_PM_OPS so the
poweroff and suspend methods are 1 and the same. If this is true for all involved drivers
(I did not check) then patch 5 is not really necessary.

I guess if fixed to deal with resume_from_noirq the same way the suspend methods
do this patch cannot hurt and if we every add separate power_off methods to the
device drivers later we need this.

Regards,

Hans




> Also notice that acpi_subsys_suspend_late() is not used outside
> of the file where it is defined, so make it static and drop its
> header for the header file containing it.
> 
> Fixes: 48402cee6889 (ACPI / LPSS: Resume BYT/CHT I2C controllers from resume_noirq)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/acpi_lpss.c |   23 ++++++++++-------------
>   drivers/acpi/device_pm.c |    3 +--
>   include/linux/acpi.h     |    2 --
>   3 files changed, 11 insertions(+), 17 deletions(-)
> 
> Index: linux-pm/drivers/acpi/acpi_lpss.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_lpss.c
> +++ linux-pm/drivers/acpi/acpi_lpss.c
> @@ -1034,34 +1034,31 @@ static int acpi_lpss_resume(struct devic
>   }
>   
>   #ifdef CONFIG_PM_SLEEP
> -static int acpi_lpss_do_suspend_late(struct device *dev)
> +static int acpi_lpss_suspend_late(struct device *dev)
>   {
> +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
>   	int ret;
>   
>   	if (dev_pm_smart_suspend_and_suspended(dev))
>   		return 0;
>   
>   	ret = pm_generic_suspend_late(dev);
> -	return ret ? ret : acpi_lpss_suspend(dev, device_may_wakeup(dev));
> -}
> +	if (ret)
> +		return ret;
>   
> -static int acpi_lpss_suspend_late(struct device *dev)
> -{
> -	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> -
> -	if (pdata->dev_desc->resume_from_noirq)
> -		return 0;
> +	if (!pdata->dev_desc->resume_from_noirq)
> +		return acpi_lpss_suspend(dev, device_may_wakeup(dev));
>   
> -	return acpi_lpss_do_suspend_late(dev);
> +	return 0;
>   }
>   
>   static int acpi_lpss_suspend_noirq(struct device *dev)
>   {
>   	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> -	int ret;
>   
> -	if (pdata->dev_desc->resume_from_noirq) {
> -		ret = acpi_lpss_do_suspend_late(dev);
> +	if (!dev_pm_smart_suspend_and_suspended(dev) &&
> +	    pdata->dev_desc->resume_from_noirq) {
> +		int ret = acpi_lpss_suspend(dev, device_may_wakeup(dev));
>   		if (ret)
>   			return ret;
>   	}
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -1069,7 +1069,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
>    * Carry out the generic late suspend procedure for @dev and use ACPI to put
>    * it into a low-power state during system transition into a sleep state.
>    */
> -int acpi_subsys_suspend_late(struct device *dev)
> +static int acpi_subsys_suspend_late(struct device *dev)
>   {
>   	int ret;
>   
> @@ -1079,7 +1079,6 @@ int acpi_subsys_suspend_late(struct devi
>   	ret = pm_generic_suspend_late(dev);
>   	return ret ? ret : acpi_dev_suspend(dev, device_may_wakeup(dev));
>   }
> -EXPORT_SYMBOL_GPL(acpi_subsys_suspend_late);
>   
>   /**
>    * acpi_subsys_suspend_noirq - Run the device driver's "noirq" suspend callback.
> Index: linux-pm/include/linux/acpi.h
> ===================================================================
> --- linux-pm.orig/include/linux/acpi.h
> +++ linux-pm/include/linux/acpi.h
> @@ -916,7 +916,6 @@ static inline int acpi_dev_pm_attach(str
>   int acpi_dev_suspend_late(struct device *dev);
>   int acpi_subsys_prepare(struct device *dev);
>   void acpi_subsys_complete(struct device *dev);
> -int acpi_subsys_suspend_late(struct device *dev);
>   int acpi_subsys_suspend_noirq(struct device *dev);
>   int acpi_subsys_suspend(struct device *dev);
>   int acpi_subsys_freeze(struct device *dev);
> @@ -924,7 +923,6 @@ int acpi_subsys_freeze(struct device *de
>   static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
>   static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
>   static inline void acpi_subsys_complete(struct device *dev) {}
> -static inline int acpi_subsys_suspend_late(struct device *dev) { return 0; }
>   static inline int acpi_subsys_suspend_noirq(struct device *dev) { return 0; }
>   static inline int acpi_subsys_suspend(struct device *dev) { return 0; }
>   static inline int acpi_subsys_freeze(struct device *dev) { return 0; }
> 
> 
> 
> 

Re: [PATCH 4/6] ACPI: LPSS: Fix ->suspend_late callbacks handling

[Rafael J. Wysocki]

On Sat, Jun 29, 2019 at 1:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Rafael,
>
> On 29-06-19 11:50, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If the resume_from_noirq flag is set in dev_desc, the ->suspend_late
> > callback provided by the device driver will be invoked at the "noirq"
> > stage of system suspend, via acpi_lpss_do_suspend_late(), which is
> > incorrect.
> >
> > To fix that, drop acpi_lpss_do_suspend_late() and rearrange
> > acpi_lpss_suspend_late() to call pm_generic_suspend_late()
> > directly, before calling acpi_lpss_suspend(), in analogy with
> > acpi_subsys_suspend_late().
>
> Ah now I see the logic in your previous test-patch.
>
> I'm afraid that this is going to break things though, the calling
> of the device-driver's suspend-late method at noirq time is
> *intentional* !

But it is a bug too.

> The resume_from_noirq flag is only set for i2c controllers which
> use: drivers/i2c/busses/i2c-designware-platdrv.c as driver.
>
> This driver's suspend late method looks like this:
>
> static int dw_i2c_plat_suspend(struct device *dev)
> {
>          struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>
>          i_dev->suspended = true;
>
>          if (i_dev->shared_with_punit)
>                  return 0;
>
>          i_dev->disable(i_dev);
>          i2c_dw_prepare_clk(i_dev, false);
>
>          return 0;
> }
>
> The i_dev->disable(i_dev) and i2c_dw_prepare_clk(i_dev, false) calls here
> will make the i2c controller non functional. But (some of) these i2c
> controllers are used by code in the _PS0  / _PS3 methods of some PCI
> devices and the PCI core calls _PS0 / _PS3 at *noirq* time, so as explained
> in the commit message which introduced acpi_lpss_do_suspend_late():
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=48402cee6889fb3fce58e95fea1471626286dc63
>
> We must not only make sure that the suspending of the i2c controller is
> ordered so that it happens after these PCI devices are suspended, we must
> also make sure that the i2c controller stays functional until the
> i2c-controller is put in the suspend-noirq state.
>
> If you really want to go this route, we must duplicate the resume_from_noirq
> flag inside drivers/i2c/busses/i2c-designware-platdrv.c, setting it
> only for acpi_lpss enumerated devices (the driver handles a whole lot more
> devices) ans then make the driver's suspend_late method a no-op and instead
> to the suspend from its suspend_noirq callback.
>
> Since pm_generic_suspend_late() is just a wrapper to call dev->driver->pm->suspend_late
> duplicating the resume_from_noirq flag inside i2c-designware-platdrv.c seems
> unproductive.
>
> Note that we have the same thing going on in acpi_lpss.c with resume_early vs
> resume_noirq, we call the resume_early callback from acpi_lpss_resume_noirq
> if the resume_from_noirq flag is set.
>
> TL;DR: the behavior you are trying to fix here is intentional and
> IMHO this patch should be dropped.

I can drop the patch, but the current code is simply incorrect.

If the driver provided a ->suspend_late callback, it wanted that
callback to be invoked during the "late" stage of suspend.  Calling it
later simply papers over a driver bug.  If invoking that callback
during the "late" stage doesn't work, the driver should have provided
a "noirq" callback instead.

> I guess we could / should do a patch adding a comment that the calling
> the drivers' suspend_late / resume_early callback at noirq time is intentional
> to avoid this confusing people in the future.

No.  We need to fix drivers doing wrong things.

Thanks!

Re: [PATCH 4/6] ACPI: LPSS: Fix ->suspend_late callbacks handling

[Rafael J. Wysocki]

On Sun, Jun 30, 2019 at 12:02 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Jun 29, 2019 at 1:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi Rafael,
> >
> > On 29-06-19 11:50, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > If the resume_from_noirq flag is set in dev_desc, the ->suspend_late
> > > callback provided by the device driver will be invoked at the "noirq"
> > > stage of system suspend, via acpi_lpss_do_suspend_late(), which is
> > > incorrect.
> > >
> > > To fix that, drop acpi_lpss_do_suspend_late() and rearrange
> > > acpi_lpss_suspend_late() to call pm_generic_suspend_late()
> > > directly, before calling acpi_lpss_suspend(), in analogy with
> > > acpi_subsys_suspend_late().
> >
> > Ah now I see the logic in your previous test-patch.
> >
> > I'm afraid that this is going to break things though, the calling
> > of the device-driver's suspend-late method at noirq time is
> > *intentional* !
>
> But it is a bug too.

Well, not strictly a bug, but a departure from a clearly established convention.

What happens is that the driver provides ->suspend_late and
->resume_early, because the upper layer normally would remove power
and power up the device at those stages, respectively.  However, due
to dependencies between devices, the upper layer kind of works around
its own limitation.

That is not straightforward at all.

I will retain this setup in the current patch series, but going
forward it would be good to clean it up.  I wonder if using
non-PM-runtime device links to represent the dependencies would work
in this case.

[PATCH 5/6] ACPI: PM: Introduce "poweroff" callbacks for ACPI PM domain and LPSS

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In general, it is not correct to call pm_generic_suspend(),
pm_generic_suspend_late() and pm_generic_suspend_noirq() during the
hibernation's "poweroff" transition, because device drivers may
provide special callbacks to be invoked then and the wrappers in
question cause system suspend callbacks to be run.  Unfortunately,
that happens in the ACPI PM domain and ACPI LPSS.

To address this potential issue, introduce "poweroff" callbacks
for the ACPI PM and LPSS that will use pm_generic_poweroff(),
pm_generic_poweroff_late() and pm_generic_poweroff_noirq() as
appropriate.

Fixes: 05087360fd7a (ACPI / PM: Take SMART_SUSPEND driver flag into account)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_lpss.c |   40 +++++++++++++++++++++++++++++---
 drivers/acpi/device_pm.c |   58 ++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/acpi.h     |    2 +
 3 files changed, 94 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -1128,6 +1128,40 @@ static int acpi_lpss_restore_early(struc
 
 	return pm_generic_restore_early(dev);
 }
+
+static int acpi_lpss_poweroff_late(struct device *dev)
+{
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+	int ret;
+
+	if (dev_pm_smart_suspend_and_suspended(dev))
+		return 0;
+
+	ret = pm_generic_poweroff_late(dev);
+	if (ret)
+		return ret;
+
+	if (!pdata->dev_desc->resume_from_noirq)
+		return acpi_lpss_suspend(dev, device_may_wakeup(dev));
+
+	return 0;
+}
+
+static int acpi_lpss_poweroff_noirq(struct device *dev)
+{
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (dev_pm_smart_suspend_and_suspended(dev))
+		return 0;
+
+	if (pdata->dev_desc->resume_from_noirq) {
+		int ret = acpi_lpss_suspend(dev, device_may_wakeup(dev));
+		if (ret)
+			return ret;
+	}
+
+	return pm_generic_poweroff_noirq(dev);
+}
 #endif /* CONFIG_PM_SLEEP */
 
 static int acpi_lpss_runtime_suspend(struct device *dev)
@@ -1161,9 +1195,9 @@ static struct dev_pm_domain acpi_lpss_pm
 		.resume_noirq = acpi_lpss_resume_noirq,
 		.resume_early = acpi_lpss_resume_early,
 		.freeze = acpi_subsys_freeze,
-		.poweroff = acpi_subsys_suspend,
-		.poweroff_late = acpi_lpss_suspend_late,
-		.poweroff_noirq = acpi_lpss_suspend_noirq,
+		.poweroff = acpi_subsys_poweroff,
+		.poweroff_late = acpi_lpss_poweroff_late,
+		.poweroff_noirq = acpi_lpss_poweroff_noirq,
 		.restore_noirq = acpi_lpss_restore_noirq,
 		.restore_early = acpi_lpss_restore_early,
 #endif
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -1175,6 +1175,58 @@ int acpi_subsys_restore_early(struct dev
 	return ret ? ret : pm_generic_restore_early(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_restore_early);
+
+/**
+ * acpi_subsys_poweroff - Run the device driver's poweroff callback.
+ * @dev: Device to handle.
+ *
+ * Follow PCI and resume devices from runtime suspend before running their
+ * system poweroff callbacks, unless the driver can cope with runtime-suspended
+ * devices during system suspend and there are no ACPI-specific reasons for
+ * resuming them.
+ */
+int acpi_subsys_poweroff(struct device *dev)
+{
+	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
+	    acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
+		pm_runtime_resume(dev);
+
+	return pm_generic_poweroff(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_subsys_poweroff);
+
+/**
+ * acpi_subsys_poweroff_late - Run the device driver's poweroff callback.
+ * @dev: Device to handle.
+ *
+ * Carry out the generic late poweroff procedure for @dev and use ACPI to put
+ * it into a low-power state during system transition into a sleep state.
+ */
+static int acpi_subsys_poweroff_late(struct device *dev)
+{
+	int ret;
+
+	if (dev_pm_smart_suspend_and_suspended(dev))
+		return 0;
+
+	ret = pm_generic_poweroff_late(dev);
+	if (ret)
+		return ret;
+
+	return acpi_dev_suspend(dev, device_may_wakeup(dev));
+}
+
+/**
+ * acpi_subsys_poweroff_noirq - Run the driver's "noirq" poweroff callback.
+ * @dev: Device to suspend.
+ */
+static int acpi_subsys_poweroff_noirq(struct device *dev)
+{
+	if (dev_pm_smart_suspend_and_suspended(dev))
+		return 0;
+
+	return pm_generic_poweroff_noirq(dev);
+}
 #endif /* CONFIG_PM_SLEEP */
 
 static struct dev_pm_domain acpi_general_pm_domain = {
@@ -1190,9 +1242,9 @@ static struct dev_pm_domain acpi_general
 		.resume_noirq = acpi_subsys_resume_noirq,
 		.resume_early = acpi_subsys_resume_early,
 		.freeze = acpi_subsys_freeze,
-		.poweroff = acpi_subsys_suspend,
-		.poweroff_late = acpi_subsys_suspend_late,
-		.poweroff_noirq = acpi_subsys_suspend_noirq,
+		.poweroff = acpi_subsys_poweroff,
+		.poweroff_late = acpi_subsys_poweroff_late,
+		.poweroff_noirq = acpi_subsys_poweroff_noirq,
 		.restore_early = acpi_subsys_restore_early,
 #endif
 	},
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -919,6 +919,7 @@ void acpi_subsys_complete(struct device
 int acpi_subsys_suspend_noirq(struct device *dev);
 int acpi_subsys_suspend(struct device *dev);
 int acpi_subsys_freeze(struct device *dev);
+int acpi_subsys_poweroff(struct device *dev);
 #else
 static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
@@ -926,6 +927,7 @@ static inline void acpi_subsys_complete(
 static inline int acpi_subsys_suspend_noirq(struct device *dev) { return 0; }
 static inline int acpi_subsys_suspend(struct device *dev) { return 0; }
 static inline int acpi_subsys_freeze(struct device *dev) { return 0; }
+static inline int acpi_subsys_poweroff(struct device *dev) { return 0; }
 #endif
 
 #ifdef CONFIG_ACPI

[PATCH 6/6] ACPI: PM: Drop unused function and function header

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Remove a leftover function header and a static inline stub with no
users from the ACPI header file.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/acpi.h |    2 --
 1 file changed, 2 deletions(-)

Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -913,7 +913,6 @@ static inline int acpi_dev_pm_attach(str
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
-int acpi_dev_suspend_late(struct device *dev);
 int acpi_subsys_prepare(struct device *dev);
 void acpi_subsys_complete(struct device *dev);
 int acpi_subsys_suspend_noirq(struct device *dev);
@@ -921,7 +920,6 @@ int acpi_subsys_suspend(struct device *d
 int acpi_subsys_freeze(struct device *dev);
 int acpi_subsys_poweroff(struct device *dev);
 #else
-static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
 static inline void acpi_subsys_complete(struct device *dev) {}
 static inline int acpi_subsys_suspend_noirq(struct device *dev) { return 0; }