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: <20220217124950.211354-1-ulf.hansson@linaro.org> <a869b705-c10e-ed0b-4119-35ef0a028311@gmail.com>
In-Reply-To: <a869b705-c10e-ed0b-4119-35ef0a028311@gmail.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Mon, 28 Feb 2022 09:56:00 +0100
Message-ID: <CAPDyKFrBxVry=yh8m+OSaq+xnzDmVqhFXQ0QN1F9WR3KB=2MGg@mail.gmail.com>
Subject: Re: [PATCH v2] PM: domains: Prevent power off for parent unless child
 is in deepest state
To:     Dmitry Osipenko <digetx@gmail.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>
Cc:     linux-pm@vger.kernel.org, Kevin Hilman <khilman@kernel.org>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Dong Aisheng <aisheng.dong@nxp.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 18 Feb 2022 at 00:11, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 17.02.2022 15:49, Ulf Hansson =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
> > A PM domain managed by genpd may support multiple idlestates (power-off
> > states). During genpd_power_off() a genpd governor may be asked to sele=
ct
> > one of the idlestates based upon the dev PM QoS constraints, for exampl=
e.
> >
> > However, there is a problem with the behaviour around this in genpd. Mo=
re
> > precisely, a parent-domain is allowed to be powered off, no matter of w=
hat
> > idlestate that has been selected for the child-domain.
> >
> > For the stm32mp1 platform from STMicro, this behaviour doesn't play wel=
l.
> > Instead, the parent-domain must not be powered off, unless the deepest
> > idlestate has been selected for the child-domain. As the current behavi=
our
> > in genpd is quite questionable anyway, let's simply change it into what=
 is
> > needed by the stm32mp1 platform.
> >
> > If it surprisingly turns out that other platforms may need a different
> > behaviour from genpd, then we will have to revisit this to find a way t=
o
> > make it configurable.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v2:
> >       - Clarified commit message - based upon discussions with Dmitry.
> >       - Updated a comment in the code, suggested by Dmitry.
> >
> > ---
> >  drivers/base/power/domain.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 5db704f02e71..c87588c21700 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -636,6 +636,18 @@ static int genpd_power_off(struct generic_pm_domai=
n *genpd, bool one_dev_on,
> >                       atomic_read(&genpd->sd_count) > 0)
> >               return -EBUSY;
> >
> > +     /*
> > +      * The children must be in their deepest (powered-off) states to =
allow
> > +      * the parent to be powered off. Note that, there's no need for
> > +      * additional locking, as powering on a child, requires the paren=
t's
> > +      * lock to be acquired first.
> > +      */
> > +     list_for_each_entry(link, &genpd->parent_links, parent_node) {
> > +             struct generic_pm_domain *child =3D link->child;
> > +             if (child->state_idx < child->state_count - 1)
> > +                     return -EBUSY;
> > +     }
> > +
> >       list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> >               enum pm_qos_flags_status stat;
> >
> > @@ -1073,6 +1085,13 @@ static void genpd_sync_power_off(struct generic_=
pm_domain *genpd, bool use_lock,
> >           || atomic_read(&genpd->sd_count) > 0)
> >               return;
> >
> > +     /* Check that the children are in their deepest (powered-off) sta=
te. */
> > +     list_for_each_entry(link, &genpd->parent_links, parent_node) {
> > +             struct generic_pm_domain *child =3D link->child;
> > +             if (child->state_idx < child->state_count - 1)
> > +                     return;
> > +     }
> > +
> >       /* Choose the deepest state when suspending */
> >       genpd->state_idx =3D genpd->state_count - 1;
> >       if (_genpd_power_off(genpd, false))
>
> Thank you, looks good. Although, this should be v3.
>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

Thanks Dmitry! I think v2 should be correct. At least I haven't sent a
v2 before. :-)

Rafael, I think this is ready to go, can please pick it up?

Kind regards
Uffe