Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 1 Mar 2022 15:56:31 +0100 | Subject | Re: [PATCH v2] PM: domains: Prevent power off for parent unless child is in deepest state |
| |
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!
| |