The series tries to fix a hardware lockup issue caused by missing domain
power state management from hibernate .freeze[thaw]_noirq hooks. The
detailed description of the issue can be found in commit log of patch #4.
Changes for v3:
- Add one patch to drop genpd status manipulation for hibernate restore
- Add Ulf's Reviewed-by tag (Thanks Ulf!)
Changes for v2:
- Fix kernel doc of genpd_finish_resume() to use the new function name
Shawn Guo (4):
PM: domains: Drop genpd status manipulation for hibernate restore
PM: domains: Pass generic PM noirq hooks to genpd_finish_suspend()
PM: domains: Consolidate genpd_restore_noirq() and
genpd_resume_noirq()
PM: domains: Power off[on] domain in hibernate .freeze[thaw]_noirq
hook
drivers/base/power/domain.c | 114 ++++++++++++------------------------
1 file changed, 36 insertions(+), 78 deletions(-)
--
2.25.1
While argument `poweroff` works fine for genpd_finish_suspend() to handle
distinction between suspend and poweroff, it won't scale if we want to
use it for freeze as well. Pass generic PM noirq hooks as arguments
instead, so that the function can possibly cover freeze case too.
Signed-off-by: Shawn Guo <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
---
drivers/base/power/domain.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 97deae1d4e77..f18b8b1bc17a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1189,12 +1189,15 @@ static int genpd_prepare(struct device *dev)
* genpd_finish_suspend - Completion of suspend or hibernation of device in an
* I/O pm domain.
* @dev: Device to suspend.
- * @poweroff: Specifies if this is a poweroff_noirq or suspend_noirq callback.
+ * @suspend_noirq: Generic suspend_noirq callback.
+ * @resume_noirq: Generic resume_noirq callback.
*
* Stop the device and remove power from the domain if all devices in it have
* been stopped.
*/
-static int genpd_finish_suspend(struct device *dev, bool poweroff)
+static int genpd_finish_suspend(struct device *dev,
+ int (*suspend_noirq)(struct device *dev),
+ int (*resume_noirq)(struct device *dev))
{
struct generic_pm_domain *genpd;
int ret = 0;
@@ -1203,10 +1206,7 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff)
if (IS_ERR(genpd))
return -EINVAL;
- if (poweroff)
- ret = pm_generic_poweroff_noirq(dev);
- else
- ret = pm_generic_suspend_noirq(dev);
+ ret = suspend_noirq(dev);
if (ret)
return ret;
@@ -1217,10 +1217,7 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff)
!pm_runtime_status_suspended(dev)) {
ret = genpd_stop_dev(genpd, dev);
if (ret) {
- if (poweroff)
- pm_generic_restore_noirq(dev);
- else
- pm_generic_resume_noirq(dev);
+ resume_noirq(dev);
return ret;
}
}
@@ -1244,7 +1241,9 @@ static int genpd_suspend_noirq(struct device *dev)
{
dev_dbg(dev, "%s()\n", __func__);
- return genpd_finish_suspend(dev, false);
+ return genpd_finish_suspend(dev,
+ pm_generic_suspend_noirq,
+ pm_generic_resume_noirq);
}
/**
@@ -1353,7 +1352,9 @@ static int genpd_poweroff_noirq(struct device *dev)
{
dev_dbg(dev, "%s()\n", __func__);
- return genpd_finish_suspend(dev, true);
+ return genpd_finish_suspend(dev,
+ pm_generic_poweroff_noirq,
+ pm_generic_restore_noirq);
}
/**
--
2.25.1
The genpd status manipulation for hibernate restore has really never
worked as intended. For example, if the genpd->status was GENPD_STATE_ON,
the parent domain's `sd_count` must have been increased, so it needs to
be adjusted too. So drop this status manipulation.
Suggested-by: Ulf Hansson <[email protected]>
Signed-off-by: Shawn Guo <[email protected]>
---
drivers/base/power/domain.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 6471b559230e..97deae1d4e77 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1374,20 +1374,7 @@ static int genpd_restore_noirq(struct device *dev)
if (IS_ERR(genpd))
return -EINVAL;
- /*
- * At this point suspended_count == 0 means we are being run for the
- * first time for the given domain in the present cycle.
- */
genpd_lock(genpd);
- if (genpd->suspended_count++ == 0) {
- /*
- * The boot kernel might put the domain into arbitrary state,
- * so make it appear as powered off to genpd_sync_power_on(),
- * so that it tries to power it on in case it was really off.
- */
- genpd->status = GENPD_STATE_OFF;
- }
-
genpd_sync_power_on(genpd, true, 0);
genpd_unlock(genpd);
--
2.25.1
On Wed, 2 Nov 2022 at 15:21, Shawn Guo <[email protected]> wrote:
>
> The genpd status manipulation for hibernate restore has really never
> worked as intended. For example, if the genpd->status was GENPD_STATE_ON,
> the parent domain's `sd_count` must have been increased, so it needs to
> be adjusted too. So drop this status manipulation.
>
> Suggested-by: Ulf Hansson <[email protected]>
> Signed-off-by: Shawn Guo <[email protected]>
Note that, I was trying to understand a little more about the
background to the below code, although it's not entirely easy to
browse the git history around this.
Unless Rafael thinks there are reasons to keep the code as is, I
wouldn't mind seeing it go away. So:
Reviewed-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
> ---
> drivers/base/power/domain.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 6471b559230e..97deae1d4e77 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1374,20 +1374,7 @@ static int genpd_restore_noirq(struct device *dev)
> if (IS_ERR(genpd))
> return -EINVAL;
>
> - /*
> - * At this point suspended_count == 0 means we are being run for the
> - * first time for the given domain in the present cycle.
> - */
> genpd_lock(genpd);
> - if (genpd->suspended_count++ == 0) {
> - /*
> - * The boot kernel might put the domain into arbitrary state,
> - * so make it appear as powered off to genpd_sync_power_on(),
> - * so that it tries to power it on in case it was really off.
> - */
> - genpd->status = GENPD_STATE_OFF;
> - }
> -
> genpd_sync_power_on(genpd, true, 0);
> genpd_unlock(genpd);
>
> --
> 2.25.1
>