[v2,2/3] PM: domains: Consolidate genpd_restore_noirq() and genpd_resume_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(-)
  

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