[PATCH 0/4] PM / Sleep / ACPI: Some cleanups and clarifications

[PATCH 0/4] PM / Sleep / ACPI: Some cleanups and clarifications

[Ulf Hansson]

This series contains four patches which are re-submitted as a subset from an
earlier posted series [1]. These changes addresses cleanups/clarifications to
the code and do not include any functional changes.

[1]
https://www.spinics.net/lists/arm-kernel/msg603856.html

Kind regards
Uffe

Ulf Hansson (4):
  PM / ACPI: Restore acpi_subsys_complete()
  PM / Sleep: Remove pm_complete_with_resume_check()
  PM / ACPI: Split code validating need for runtime resume in
    ->prepare()
  PM / ACPI: Split acpi_lpss_suspend_late|resume_early()

 drivers/acpi/acpi_lpss.c         | 31 +++++++++++++++++-----
 drivers/acpi/device_pm.c         | 56 ++++++++++++++++++++++++++++++----------
 drivers/base/power/generic_ops.c | 23 -----------------
 include/linux/pm.h               |  1 -
 4 files changed, 66 insertions(+), 45 deletions(-)

-- 
2.7.4

[PATCH 1/4] PM / ACPI: Restore acpi_subsys_complete()

[Ulf Hansson]

The commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have
been reset by firmware"), made PCI's and ACPI's ->complete() callbacks to
be assigned to a new API called pm_complete_with_resume_check(), which was
introduced in the same change.

Later it turned out that using pm_complete_with_resume_check() isn't good
enough for PCI, as it needs additional PCI specific checks, before deciding
whether runtime resuming the device is needed when running the ->complete()
callback.

This leaves ACPI being the only user of pm_complete_with_resume_check().
Therefore let's restore ACPI's acpi_subsys_complete(), which was dropped in
commit 58a1fbbb2ee8 ("PM / PCI / ACPI: Kick devices that might have been
reset by firmware").

This enables us to remove the pm_complete_with_resume_check() API in a
following change, but it also enables ACPI to add more ACPI specific checks
in acpi_subsys_complete() if it turns out that is needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c |  2 +-
 drivers/acpi/device_pm.c | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 032ae44..0c84d15 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -894,7 +894,7 @@ static struct dev_pm_domain acpi_lpss_pm_domain = {
 #ifdef CONFIG_PM
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = pm_complete_with_resume_check,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_lpss_suspend_late,
 		.resume_early = acpi_lpss_resume_early,
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index fbcc73f..632f214 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1020,6 +1020,23 @@ int acpi_subsys_prepare(struct device *dev)
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
 /**
+ * acpi_subsys_complete - Finalize device's resume during system resume.
+ * @dev: Device to handle.
+ */
+void acpi_subsys_complete(struct device *dev)
+{
+	pm_generic_complete(dev);
+	/*
+	 * If the device had been runtime-suspended before the system went into
+	 * the sleep state it is going out of and it has never been resumed till
+	 * now, resume it in case the firmware powered it up.
+	 */
+	if (dev->power.direct_complete && pm_resume_via_firmware())
+		pm_request_resume(dev);
+}
+EXPORT_SYMBOL_GPL(acpi_subsys_complete);
+
+/**
  * acpi_subsys_suspend - Run the device driver's suspend callback.
  * @dev: Device to handle.
  *
@@ -1087,7 +1104,7 @@ static struct dev_pm_domain acpi_general_pm_domain = {
 		.runtime_resume = acpi_subsys_runtime_resume,
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
-		.complete = pm_complete_with_resume_check,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_subsys_suspend_late,
 		.resume_early = acpi_subsys_resume_early,
-- 
2.7.4

[PATCH 2/4] PM / Sleep: Remove pm_complete_with_resume_check()

[Ulf Hansson]

According to recent changes for ACPI, the are longer any users of
pm_complete_with_resume_check(), thus let's drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/generic_ops.c | 23 -----------------------
 include/linux/pm.h               |  1 -
 2 files changed, 24 deletions(-)

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 07c3c4a..b2ed606 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -9,7 +9,6 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/export.h>
-#include <linux/suspend.h>
 
 #ifdef CONFIG_PM
 /**
@@ -298,26 +297,4 @@ void pm_generic_complete(struct device *dev)
 	if (drv && drv->pm && drv->pm->complete)
 		drv->pm->complete(dev);
 }
-
-/**
- * pm_complete_with_resume_check - Complete a device power transition.
- * @dev: Device to handle.
- *
- * Complete a device power transition during a system-wide power transition and
- * optionally schedule a runtime resume of the device if the system resume in
- * progress has been initated by the platform firmware and the device had its
- * power.direct_complete flag set.
- */
-void pm_complete_with_resume_check(struct device *dev)
-{
-	pm_generic_complete(dev);
-	/*
-	 * If the device had been runtime-suspended before the system went into
-	 * the sleep state it is going out of and it has never been resumed till
-	 * now, resume it in case the firmware powered it up.
-	 */
-	if (dev->power.direct_complete && pm_resume_via_firmware())
-		pm_request_resume(dev);
-}
-EXPORT_SYMBOL_GPL(pm_complete_with_resume_check);
 #endif /* CONFIG_PM_SLEEP */
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 47ded8a..a0ceecc 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -736,7 +736,6 @@ extern int pm_generic_poweroff_noirq(struct device *dev);
 extern int pm_generic_poweroff_late(struct device *dev);
 extern int pm_generic_poweroff(struct device *dev);
 extern void pm_generic_complete(struct device *dev);
-extern void pm_complete_with_resume_check(struct device *dev);
 
 #else /* !CONFIG_PM_SLEEP */
 
-- 
2.7.4

[PATCH 3/4] PM / ACPI: Split code validating need for runtime resume in ->prepare()

[Ulf Hansson]

Move the code dealing with validation of whether runtime resuming the
device is needed during system suspend.

In this way it becomes more clear for what circumstances ACPI is prevented
from trying the direct_complete path.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 632f214..5181057 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -989,6 +989,27 @@ int acpi_dev_resume_early(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
 
+static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
+{
+	u32 sys_target = acpi_target_system_state();
+	int ret, state;
+
+	if (device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
+		return true;
+
+	if (sys_target == ACPI_STATE_S0)
+		return false;
+
+	if (adev->power.flags.dsw_present)
+		return true;
+
+	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
+	if (ret)
+		return true;
+
+	return state != adev->power.state;
+}
+
 /**
  * acpi_subsys_prepare - Prepare device for system transition to a sleep state.
  * @dev: Device to prepare.
@@ -996,26 +1017,16 @@ EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
 int acpi_subsys_prepare(struct device *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
-	u32 sys_target;
-	int ret, state;
+	int ret;
 
 	ret = pm_generic_prepare(dev);
 	if (ret < 0)
 		return ret;
 
-	if (!adev || !pm_runtime_suspended(dev)
-	    || device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
-		return 0;
-
-	sys_target = acpi_target_system_state();
-	if (sys_target == ACPI_STATE_S0)
-		return 1;
-
-	if (adev->power.flags.dsw_present)
+	if (!adev || !pm_runtime_suspended(dev))
 		return 0;
 
-	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
-	return !ret && state == adev->power.state;
+	return !acpi_dev_needs_resume(dev, adev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
-- 
2.7.4

[PATCH 4/4] PM / ACPI: Split acpi_lpss_suspend_late|resume_early()

[Ulf Hansson]

Move the code which is special to ACPI LPSS into separate functions. This
may clarify the code a bit, but the main purpose of this change, is instead
to prepare for additional changes on top. Ideally the following changes
should then become easier to review.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/acpi/acpi_lpss.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 0c84d15..e726173 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -717,22 +717,28 @@ static void acpi_lpss_dismiss(struct device *dev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int acpi_lpss_suspend_late(struct device *dev)
+static int lpss_suspend_late(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+
+	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
+		acpi_lpss_save_ctx(dev, pdata);
+
+	return acpi_dev_suspend_late(dev);
+}
+
+static int acpi_lpss_suspend_late(struct device *dev)
+{
 	int ret;
 
 	ret = pm_generic_suspend_late(dev);
 	if (ret)
 		return ret;
 
-	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
-		acpi_lpss_save_ctx(dev, pdata);
-
-	return acpi_dev_suspend_late(dev);
+	return lpss_suspend_late(dev);
 }
 
-static int acpi_lpss_resume_early(struct device *dev)
+static int lpss_resume_early(struct device *dev)
 {
 	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
 	int ret;
@@ -746,6 +752,17 @@ static int acpi_lpss_resume_early(struct device *dev)
 	if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
 		acpi_lpss_restore_ctx(dev, pdata);
 
+	return 0;
+}
+
+static int acpi_lpss_resume_early(struct device *dev)
+{
+	int ret;
+
+	ret = lpss_resume_early(dev);
+	if (ret)
+		return ret;
+
 	return pm_generic_resume_early(dev);
 }
 #endif /* CONFIG_PM_SLEEP */
-- 
2.7.4

Re: [PATCH 0/4] PM / Sleep / ACPI: Some cleanups and clarifications

[Rafael J. Wysocki]

On Tue, Oct 3, 2017 at 9:11 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> This series contains four patches which are re-submitted as a subset from an
> earlier posted series [1]. These changes addresses cleanups/clarifications to
> the code and do not include any functional changes.
>
> [1]
> https://www.spinics.net/lists/arm-kernel/msg603856.html

OK for patches [1-3/4], but I'm not sure about the last one.

Thanks,
Rafael

Re: [PATCH 4/4] PM / ACPI: Split acpi_lpss_suspend_late|resume_early()

[Rafael J. Wysocki]

On Tue, Oct 3, 2017 at 9:11 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Move the code which is special to ACPI LPSS into separate functions. This
> may clarify the code a bit, but the main purpose of this change, is instead
> to prepare for additional changes on top. Ideally the following changes
> should then become easier to review.

I'm not sure what changes this is talking about.

I have the device flags patches in the works, but I don't think they
will need this ATM.

Thanks,
Rafael

Re: [PATCH 4/4] PM / ACPI: Split acpi_lpss_suspend_late|resume_early()

[Ulf Hansson]

On 6 October 2017 at 14:59, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Oct 3, 2017 at 9:11 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Move the code which is special to ACPI LPSS into separate functions. This
>> may clarify the code a bit, but the main purpose of this change, is instead
>> to prepare for additional changes on top. Ideally the following changes
>> should then become easier to review.
>
> I'm not sure what changes this is talking about.

Right, I forgot to update the changelog.

>
> I have the device flags patches in the works, but I don't think they
> will need this ATM.

Okay, then let's just ignore this one.

Kind regards
Uffe