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

[PATCH v2] 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 from the current
behaviour. However, there is an STMicro platform that is being worked on,
which can't cope with this. Therefore, let's change genpd to prevent the
parent- domain from being powered off, unless the deepest idlestate has
been selected for the child-domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Clarified the commit message

---
 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 v2] PM: domains: Prevent power off for parent unless child is in deepest state

[Dmitry Osipenko]

04.02.2022 13:16, 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 from the current
> behaviour. However, there is an STMicro platform that is being worked on,
> which can't cope with this.

Could you please provide some technical info about why STMicro platform
can't cope with that?

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

[Ulf Hansson]

On Tue, 15 Feb 2022 at 22:49, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 04.02.2022 13:16, 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 from the current
> > behaviour. However, there is an STMicro platform that is being worked on,
> > which can't cope with this.
>
> Could you please provide some technical info about why STMicro platform
> can't cope with that?

There is a parent domain with one power-off state. The parent domain
has a few devices attached to it, which means they need to be managed
to together. The parent domain is controlling a shared power rail.

The child domain, which has multiple idle states to choose from, also
has some devices attached to it. The child domain is controlling a
system clock, but also relies on the shared power rail that is managed
by the parent domain.

Obviously, these idle states are managed by firmware.

I hope that made it a bit more clear?

Kind regards
Uffe

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

[Dmitry Osipenko]

16.02.2022 12:45, Ulf Hansson пишет:
> On Tue, 15 Feb 2022 at 22:49, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 04.02.2022 13:16, 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 from the current
>>> behaviour. However, there is an STMicro platform that is being worked on,
>>> which can't cope with this.
>>
>> Could you please provide some technical info about why STMicro platform
>> can't cope with that?
> 
> There is a parent domain with one power-off state. The parent domain
> has a few devices attached to it, which means they need to be managed
> to together. The parent domain is controlling a shared power rail.
> 
> The child domain, which has multiple idle states to choose from, also
> has some devices attached to it. The child domain is controlling a
> system clock, but also relies on the shared power rail that is managed
> by the parent domain.
> 
> Obviously, these idle states are managed by firmware.
> 
> I hope that made it a bit more clear?

Yes, I see now what this patch does.

So "deepest state" in the code's comment means powered off state. Maybe
the comment could clarify it better a tad.

- * The children must be in their deepest states to allow the parent to
+ * The children must be in their deepest (powered-off) states to allow
the parent to

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

[Rafael J. Wysocki]

On Mon, Feb 28, 2022 at 9:56 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 18 Feb 2022 at 00:11, Dmitry Osipenko <digetx@gmail.com> wrote:
> >
> > 17.02.2022 15:49, Ulf Hansson пишет:
> > > A PM domain managed by genpd may support multiple idlestates (power-off
> > > states). 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.
> > >
> > > For the stm32mp1 platform from STMicro, this behaviour doesn't play well.
> > > Instead, the parent-domain must not be powered off, unless the deepest
> > > idlestate has been selected for the child-domain. As the current behaviour
> > > 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 to
> > > 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_domain *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 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 +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) 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))
> >
> > 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?

Applied as 5.18 material, thanks!

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

[Ulf Hansson]

On Fri, 18 Feb 2022 at 00:11, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 17.02.2022 15:49, Ulf Hansson пишет:
> > A PM domain managed by genpd may support multiple idlestates (power-off
> > states). 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.
> >
> > For the stm32mp1 platform from STMicro, this behaviour doesn't play well.
> > Instead, the parent-domain must not be powered off, unless the deepest
> > idlestate has been selected for the child-domain. As the current behaviour
> > 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 to
> > 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_domain *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 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 +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) 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))
>
> 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

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

[Dmitry Osipenko]

17.02.2022 15:49, Ulf Hansson пишет:
> A PM domain managed by genpd may support multiple idlestates (power-off
> states). 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.
> 
> For the stm32mp1 platform from STMicro, this behaviour doesn't play well.
> Instead, the parent-domain must not be powered off, unless the deepest
> idlestate has been selected for the child-domain. As the current behaviour
> 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 to
> 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_domain *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 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 +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) 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))

Thank you, looks good. Although, this should be v3.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

[PATCH v2] 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 (power-off
states). 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.

For the stm32mp1 platform from STMicro, this behaviour doesn't play well.
Instead, the parent-domain must not be powered off, unless the deepest
idlestate has been selected for the child-domain. As the current behaviour
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 to
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_domain *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 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 +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) 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