Re: [PATCH] PM: domains: Prevent power off for parent unless child is in deepest state

[Ulf Hansson]

On Fri, 4 Feb 2022 at 20:10, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> 04.02.2022 12:43, Ulf Hansson пишет:
> > On Mon, 31 Jan 2022 at 19:29, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
> >>
> >> 31.01.2022 14:37, Ulf Hansson пишет:
> >>> A PM domain managed by genpd may support multiple idlestates. During
> >>> genpd_power_off() a genpd governor may be asked to select one of the
> >>> idlestates based upon the dev PM QoS constraints, for example.
> >>>
> >>> However, there is a problem with the behaviour around this in genpd. More
> >>> precisely, a parent-domain is allowed to be powered off, no matter of what
> >>> idlestate that has been selected for the child-domain.
> >>>
> >>> So far, we have not received any reports about errors, possibly because
> >>> there might not be platform with this hierarchical configuration, yet.
> >>> Nevertheless, it seems reasonable to change the behaviour into preventing
> >>> the parent-domain from being powered off, unless the deepest idlestate has
> >>> been selected for the child-domain, so let's do that.
> >>>
> >>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> >>> ---
> >>>  drivers/base/power/domain.c | 18 ++++++++++++++++++
> >>>  1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >>> index 5db704f02e71..7f97c5cabdc2 100644
> >>> --- a/drivers/base/power/domain.c
> >>> +++ b/drivers/base/power/domain.c
> >>> @@ -636,6 +636,17 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> >>>                       atomic_read(&genpd->sd_count) > 0)
> >>>               return -EBUSY;
> >>>
> >>> +     /*
> >>> +      * The children must be in their deepest 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 parent's lock to be acquired first.
> >>> +      */
> >>> +     list_for_each_entry(link, &genpd->parent_links, parent_node) {
> >>> +             struct generic_pm_domain *child = 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 +1084,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 state. */
> >>> +     list_for_each_entry(link, &genpd->parent_links, parent_node) {
> >>> +             struct generic_pm_domain *child = link->child;
> >>> +             if (child->state_idx < child->state_count - 1)
> >>> +                     return;
> >>> +     }
> >>> +
> >>>       /* Choose the deepest state when suspending */
> >>>       genpd->state_idx = genpd->state_count - 1;
> >>>       if (_genpd_power_off(genpd, false))
> >>
> >> Hello Ulf,
> >
> > Hi Dmitry,
> >
> >>
> >> Is this needed by a concrete SoC? It needs to be clarified in the commit
> >> message, otherwise looks like this patch wasn't tested and it's unclear
> >> whether this change is really needed.
> >
> > It's needed on a STMicro SoC that I have been working on. However,
> > it's difficult for me to test on that platform, as some SoC specific
> > pieces are missing upstream (the power domain deployment in
> > particular). Anyway, let me add some information about this in the
> > commit log for the next version.
> >
> > When it comes to testing, I am using a couple of local test dummy
> > drivers. One that manages devices that gets attached to a genpd,
> > mostly to execute runtime PM and dev PM QoS calls - and another that
> > manages the PM domains with genpd. I have been thinking of a way to
> > share these "tools" to let other people use them for testing too, but
> > I haven't just got to it yet.
> >
> > Besides the above, do you see any issues from Nvidia platforms point
> > of view with $subject patch?
>
> I've two main concerns:
>
> 1. This is a patch for something (STMicro SoC) that isn't fully
> supported by upstream kernel and it's not clear whether it will be ever
> supported at all.

The upstream work is ongoing, it's the stm32mp1 platform, which is
already supported upstream.

>
> 2. It's not clear why behaviour of a very specific SoC should be applied
> to all SoCs, especially given that the specific SoC itself isn't going
> to use to this feature right now. I guess it could be okay to put this
> behaviour into the core code until any other SoC will require a
> different behaviour, but the commit message doesn't clarify this.

The point with the commit message is to question the current default
behaviour. If we have a QoS constraint that causes the genpd governor
to select a shallow state for a child, it seems wrong to allow the
parent to be turned off, in my opinion.

If a platform with a PM domain hierarchy would need a different
behaviour from genpd, then we need to look into that, of course.
However, the current *uncontrolled* behaviour is most likely not going
to be suitable for any platform anyway.

>
> To my knowledge all NVIDIA Tegra SoCs are indifferent to this patch
> because they don't have such kind of dependency between power domains.

Great, thanks for confirming!

>
> In general, such changes usually are deferred from being upstreamed
> until there is a real user, otherwise there is a risk of cluttering the
> code with unused features. Do you have a time estimation in regards to
> when STMicro may start to benefit from this change?

The STMicro folkz are working on it right now, but I can't give you
any estimates for their work.

Moreover, I think the important point in this regard, is that the
$subject patch doesn't really hurt anything else, so then what's the
point of holding this back?

Kind regards
Uffe