Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221102142104.2006554-1-shawn.guo@linaro.org> <20221102142104.2006554-2-shawn.guo@linaro.org>
In-Reply-To: <20221102142104.2006554-2-shawn.guo@linaro.org>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri, 4 Nov 2022 16:32:16 +0100
Message-ID: <CAPDyKFouo8LCrDe2tLjO=LXM3P7NDhTY2zoDzkdou+gVp8jtGQ@mail.gmail.com>
Subject: Re: [PATCH v3 1/4] PM: domains: Drop genpd status manipulation for
 hibernate restore
To:     Shawn Guo <shawn.guo@linaro.org>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Kevin Hilman <khilman@kernel.org>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 2 Nov 2022 at 15:21, Shawn Guo <shawn.guo@linaro.org> 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 <ulf.hansson@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

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 <ulf.hansson@linaro.org>

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
>