Messages in this thread | | | From | Ulf Hansson <> | Date | Tue, 1 Nov 2022 11:23:45 +0100 | Subject | Re: [PATCH v2 2/3] PM: domains: Consolidate genpd_restore_noirq() and genpd_resume_noirq() |
| |
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
| |