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

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

[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@linaro.org>
---
 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))
-- 
2.25.1

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

[Dmitry Osipenko]

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

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.

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

[Ulf Hansson]

On Mon, 31 Jan 2022 at 19:29, Dmitry Osipenko <digetx@gmail.com> 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@linaro.org>
> > ---
> >  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?

Kind regards
Uffe

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

[Dmitry Osipenko]

04.02.2022 12:43, Ulf Hansson пишет:
> On Mon, 31 Jan 2022 at 19:29, Dmitry Osipenko <digetx@gmail.com> 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@linaro.org>
>>> ---
>>>  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.

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.

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

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?

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@gmail.com> wrote:
>
> 04.02.2022 12:43, Ulf Hansson пишет:
> > On Mon, 31 Jan 2022 at 19:29, Dmitry Osipenko <digetx@gmail.com> 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@linaro.org>
> >>> ---
> >>>  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

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

[Dmitry Osipenko]

07.02.2022 11:43, Ulf Hansson пишет:
>> 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?

If that work will never pan out, will you remove the unused code?

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

[Ulf Hansson]

On Sun, 13 Feb 2022 at 13:14, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 07.02.2022 11:43, Ulf Hansson пишет:
> >> 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?
>
> If that work will never pan out, will you remove the unused code?

Sure, I will continue to monitor the situation, which is what I have
been doing for many years by now.

In the past we have agreed to add new things to genpd, even if those
didn't have in-tree users when the changes went in. The current
dev_pm_genpd_set_next_wakeup() inteface, for example, is still lacking
a user upstream. It's a balance, because I certainly agree with you,
that we don't want to carry around dead code in the kernel - unless we
have reasons to believe it's an intermediate step before there a user
turning up.

Kind regards
Uffe

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

[Dmitry Osipenko]

14.02.2022 12:22, Ulf Hansson пишет:
> On Sun, 13 Feb 2022 at 13:14, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 07.02.2022 11:43, Ulf Hansson пишет:
>>>> 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?
>>
>> If that work will never pan out, will you remove the unused code?
> 
> Sure, I will continue to monitor the situation, which is what I have
> been doing for many years by now.
> 
> In the past we have agreed to add new things to genpd, even if those
> didn't have in-tree users when the changes went in. The current
> dev_pm_genpd_set_next_wakeup() inteface, for example, is still lacking
> a user upstream. It's a balance, because I certainly agree with you,
> that we don't want to carry around dead code in the kernel - unless we
> have reasons to believe it's an intermediate step before there a user
> turning up.

I've seen enough of dead code while was doing a tree-wide changes, we
don't need more :)

Oh, well. Sounds like you're working closely with the STMicro people, so
should be fine.