| Message ID | 20221101024736.1509207-3-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Manage domain power state for hibernate freeze | expand |
On Tue, 1 Nov 2022 at 03:47, Shawn Guo <shawn.guo@linaro.org> wrote: > > Most of the logic between genpd_restore_noirq() and genpd_resume_noirq() > are same except GENPD_STATE_OFF status reset for hibernation restore. > The suspended_count decrement for restore should be the right thing to do > anyway, considering there is an increment in genpd_finish_suspend() for > hibernation. > > Consolidate genpd_restore_noirq() and genpd_resume_noirq() into > genpd_finish_resume() and handle GENPD_STATE_OFF status reset for > restore case specially. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> I have a comment, see more below. Nevertheless, please add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/domain.c | 70 ++++++++++++++++++------------------- > 1 file changed, 35 insertions(+), 35 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 54f6b0dd35fb..b81baeb38d81 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1247,12 +1247,14 @@ static int genpd_suspend_noirq(struct device *dev) > } > > /** > - * genpd_resume_noirq - Start of resume of device in an I/O PM domain. > + * genpd_finish_resume - Completion of resume of device in an I/O PM domain. > * @dev: Device to resume. > + * @resume_noirq: Generic resume_noirq callback. > * > * Restore power to the device's PM domain, if necessary, and start the device. > */ > -static int genpd_resume_noirq(struct device *dev) > +static int genpd_finish_resume(struct device *dev, > + int (*resume_noirq)(struct device *dev)) > { > struct generic_pm_domain *genpd; > int ret; > @@ -1264,9 +1266,25 @@ static int genpd_resume_noirq(struct device *dev) > return -EINVAL; > > if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd)) > - return pm_generic_resume_noirq(dev); > + return resume_noirq(dev); > > genpd_lock(genpd); > + > + /* > + * Special handling for hibernation restore: > + * At this point suspended_count == 0 means we are being run for the > + * first time for the given domain in the present cycle. > + */ > + if (resume_noirq == pm_generic_restore_noirq && > + 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; This has really never worked as intended. Resetting the status like this, needs more careful actions. For example, if the genpd->status was GENPD_STATE_ON, the parent domain's ->sd_count have been increased - so that needs to be adjusted too. By looking at patch3/3, I wonder if we shouldn't try to align the hibernation behaviors so the above hack can be dropped. Do you think that could work? > + } > + > genpd_sync_power_on(genpd, true, 0); > genpd->suspended_count--; > genpd_unlock(genpd); > @@ -1281,6 +1299,19 @@ static int genpd_resume_noirq(struct device *dev) > return pm_generic_resume_noirq(dev); > } > > +/** > + * genpd_resume_noirq - Start of resume of device in an I/O PM domain. > + * @dev: Device to resume. > + * > + * Restore power to the device's PM domain, if necessary, and start the device. > + */ > +static int genpd_resume_noirq(struct device *dev) > +{ > + dev_dbg(dev, "%s()\n", __func__); > + > + return genpd_finish_resume(dev, pm_generic_resume_noirq); > +} > + > /** > * genpd_freeze_noirq - Completion of freezing a device in an I/O PM domain. > * @dev: Device to freeze. > @@ -1366,40 +1397,9 @@ static int genpd_poweroff_noirq(struct device *dev) > */ > static int genpd_restore_noirq(struct device *dev) > { > - struct generic_pm_domain *genpd; > - int ret = 0; > - > dev_dbg(dev, "%s()\n", __func__); > > - genpd = dev_to_genpd(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); > - > - if (genpd->dev_ops.stop && genpd->dev_ops.start && > - !pm_runtime_status_suspended(dev)) { > - ret = genpd_start_dev(genpd, dev); > - if (ret) > - return ret; > - } > - > - return pm_generic_restore_noirq(dev); > + return genpd_finish_resume(dev, pm_generic_restore_noirq); > } > > /** Kind regards Uffe
On Tue, Nov 01, 2022 at 11:23:45AM +0100, Ulf Hansson wrote: > On Tue, 1 Nov 2022 at 03:47, Shawn Guo <shawn.guo@linaro.org> wrote: > > > > Most of the logic between genpd_restore_noirq() and genpd_resume_noirq() > > are same except GENPD_STATE_OFF status reset for hibernation restore. > > The suspended_count decrement for restore should be the right thing to do > > anyway, considering there is an increment in genpd_finish_suspend() for > > hibernation. > > > > Consolidate genpd_restore_noirq() and genpd_resume_noirq() into > > genpd_finish_resume() and handle GENPD_STATE_OFF status reset for > > restore case specially. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > I have a comment, see more below. > > Nevertheless, please add: > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > drivers/base/power/domain.c | 70 ++++++++++++++++++------------------- > > 1 file changed, 35 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 54f6b0dd35fb..b81baeb38d81 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -1247,12 +1247,14 @@ static int genpd_suspend_noirq(struct device *dev) > > } > > > > /** > > - * genpd_resume_noirq - Start of resume of device in an I/O PM domain. > > + * genpd_finish_resume - Completion of resume of device in an I/O PM domain. > > * @dev: Device to resume. > > + * @resume_noirq: Generic resume_noirq callback. > > * > > * Restore power to the device's PM domain, if necessary, and start the device. > > */ > > -static int genpd_resume_noirq(struct device *dev) > > +static int genpd_finish_resume(struct device *dev, > > + int (*resume_noirq)(struct device *dev)) > > { > > struct generic_pm_domain *genpd; > > int ret; > > @@ -1264,9 +1266,25 @@ static int genpd_resume_noirq(struct device *dev) > > return -EINVAL; > > > > if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd)) > > - return pm_generic_resume_noirq(dev); > > + return resume_noirq(dev); > > > > genpd_lock(genpd); > > + > > + /* > > + * Special handling for hibernation restore: > > + * At this point suspended_count == 0 means we are being run for the > > + * first time for the given domain in the present cycle. > > + */ > > + if (resume_noirq == pm_generic_restore_noirq && > > + 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; > > This has really never worked as intended. Resetting the status like > this, needs more careful actions. > > For example, if the genpd->status was GENPD_STATE_ON, the parent > domain's ->sd_count have been increased - so that needs to be adjusted > too. > > By looking at patch3/3, I wonder if we shouldn't try to align the > hibernation behaviors so the above hack can be dropped. Do you think > that could work? To be honest, I found this piece of code suspicious when I was fixing my problem. To be on the safe side, I chose to leave it there because I'm not sure if it's handling any special cases or platform quirks. I tested on my platform with dropping the code. Worked perfectly fine. So I will repost the series by starting with this cleanup. Shawn
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 54f6b0dd35fb..b81baeb38d81 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1247,12 +1247,14 @@ static int genpd_suspend_noirq(struct device *dev) } /** - * genpd_resume_noirq - Start of resume of device in an I/O PM domain. + * genpd_finish_resume - Completion of resume of device in an I/O PM domain. * @dev: Device to resume. + * @resume_noirq: Generic resume_noirq callback. * * Restore power to the device's PM domain, if necessary, and start the device. */ -static int genpd_resume_noirq(struct device *dev) +static int genpd_finish_resume(struct device *dev, + int (*resume_noirq)(struct device *dev)) { struct generic_pm_domain *genpd; int ret; @@ -1264,9 +1266,25 @@ static int genpd_resume_noirq(struct device *dev) return -EINVAL; if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd)) - return pm_generic_resume_noirq(dev); + return resume_noirq(dev); genpd_lock(genpd); + + /* + * Special handling for hibernation restore: + * At this point suspended_count == 0 means we are being run for the + * first time for the given domain in the present cycle. + */ + if (resume_noirq == pm_generic_restore_noirq && + 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->suspended_count--; genpd_unlock(genpd); @@ -1281,6 +1299,19 @@ static int genpd_resume_noirq(struct device *dev) return pm_generic_resume_noirq(dev); } +/** + * genpd_resume_noirq - Start of resume of device in an I/O PM domain. + * @dev: Device to resume. + * + * Restore power to the device's PM domain, if necessary, and start the device. + */ +static int genpd_resume_noirq(struct device *dev) +{ + dev_dbg(dev, "%s()\n", __func__); + + return genpd_finish_resume(dev, pm_generic_resume_noirq); +} + /** * genpd_freeze_noirq - Completion of freezing a device in an I/O PM domain. * @dev: Device to freeze. @@ -1366,40 +1397,9 @@ static int genpd_poweroff_noirq(struct device *dev) */ static int genpd_restore_noirq(struct device *dev) { - struct generic_pm_domain *genpd; - int ret = 0; - dev_dbg(dev, "%s()\n", __func__); - genpd = dev_to_genpd(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); - - if (genpd->dev_ops.stop && genpd->dev_ops.start && - !pm_runtime_status_suspended(dev)) { - ret = genpd_start_dev(genpd, dev); - if (ret) - return ret; - } - - return pm_generic_restore_noirq(dev); + return genpd_finish_resume(dev, pm_generic_restore_noirq); } /**
Most of the logic between genpd_restore_noirq() and genpd_resume_noirq() are same except GENPD_STATE_OFF status reset for hibernation restore. The suspended_count decrement for restore should be the right thing to do anyway, considering there is an increment in genpd_finish_suspend() for hibernation. Consolidate genpd_restore_noirq() and genpd_resume_noirq() into genpd_finish_resume() and handle GENPD_STATE_OFF status reset for restore case specially. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/base/power/domain.c | 70 ++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 35 deletions(-)