lkml.org 
[lkml]   [2022]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 2/3] PM: domains: Consolidate genpd_restore_noirq() and genpd_resume_noirq()
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

\
 
 \ /
  Last update: 2022-11-02 10:31    [W:0.053 / U:1.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site