Received-SPF: pass (google.com: domain of linux-kernel+bounces-33576-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240103-genpd-perf-order-v2-1-eeecfc55624b@gerhold.net>
In-Reply-To: <20240103-genpd-perf-order-v2-1-eeecfc55624b@gerhold.net>
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Mon, 22 Jan 2024 16:21:06 +0100
Message-ID: <CAPDyKFr3TRWM9UOtKkxSXKCAFN5wuvWLNcUfQuqGD91Ut1Ys1g@mail.gmail.com>
Subject: Re: [PATCH v2] PM: domains: Scale down parent performance states in
 reverse order
To: Stephan Gerhold <stephan@gerhold.net>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Viresh Kumar <viresh.kumar@linaro.org>
Content-Type: text/plain; charset="UTF-8"

On Wed, 3 Jan 2024 at 22:10, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> Power domains might have parent domains assigned that are automatically
> managed by the PM domain core. In particular, parent domains are
> automatically powered on/off and setting performance states on child
> domains is translated to parent domains (e.g. using an OPP table from
> the device tree).
>
> Currently parent performance states are always adjusted before the
> performance state of the child domain is changed.
>
> However, typically a parent/child relationship between two power
> domains with performance states models the requirement to keep the
> parent domain at a performance state equal or higher to the child
> domain. When scaling down there is a brief moment where the parent
> domain will end up having a lower performance state than required by
> the child domain.
>
> To avoid this, we need to differentiate between scaling up/down and
> adjust the order of operations:
>
>  - When scaling up, parent domains should be adjusted before the child
>    domain. In case of an error, the rollback happens in reverse order.
>
>  - When scaling down, parent domains should be adjusted after the child
>    domain, in reverse order, just as if we would rollback scaling up.
>    In case of an error, the rollback happens in normal order (just as
>    if we would normally scale up).
>
> Implement this by moving the existing functionality of
> _genpd_set_performance_state() to two separate functions that are
> called in the proper iteration order.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

I have tested this with my local set of test drivers - and things are
now working as they should. Thanks a lot for fixing this!

I have applied the patch for next, but it's probably a good idea to
add a fixes/stable tag to it and queue it as a fix instead. What do
you think?

Note that, I took the liberty of slightly updating the commit message
a bit, to try to make things even more clear.

Kind regards
Uffe


> ---
> Changes in v2:
> - Rebase to adjust for move of drivers/base/power/domain.c
>   to drivers/pmdomain/core.c
> - Regenerate CC list
> - No code changes
> - Link to v1: https://lore.kernel.org/r/20231205-genpd-perf-order-v1-1-6597cc69a729@gerhold.net
> ---
> Related discussion: https://lore.kernel.org/linux-pm/ZWXgFNKgm9QaFuzx@gerhold.net/
> ---
>  drivers/pmdomain/core.c | 124 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 77 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index a1f6cba3ae6c..fec9dc6ab828 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -311,72 +311,102 @@ static int genpd_xlate_performance_state(struct generic_pm_domain *genpd,
>  }
>
>  static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
> -                                       unsigned int state, int depth)
> +                                       unsigned int state, int depth);
> +
> +static void _genpd_rollback_parent_state(struct gpd_link *link, int depth)
>  {
> -       struct generic_pm_domain *parent;
> -       struct gpd_link *link;
> -       int parent_state, ret;
> +       struct generic_pm_domain *parent = link->parent;
> +       int parent_state;
>
> -       if (state == genpd->performance_state)
> -               return 0;
> +       genpd_lock_nested(parent, depth + 1);
>
> -       /* Propagate to parents of genpd */
> -       list_for_each_entry(link, &genpd->child_links, child_node) {
> -               parent = link->parent;
> +       parent_state = link->prev_performance_state;
> +       link->performance_state = parent_state;
>
> -               /* Find parent's performance state */
> -               ret = genpd_xlate_performance_state(genpd, parent, state);
> -               if (unlikely(ret < 0))
> -                       goto err;
> +       parent_state = _genpd_reeval_performance_state(parent, parent_state);
> +       if (_genpd_set_performance_state(parent, parent_state, depth + 1)) {
> +               pr_err("%s: Failed to roll back to %d performance state\n",
> +                      parent->name, parent_state);
> +       }
>
> -               parent_state = ret;
> +       genpd_unlock(parent);
> +}
>
> -               genpd_lock_nested(parent, depth + 1);
> +static int _genpd_set_parent_state(struct generic_pm_domain *genpd,
> +                                  struct gpd_link *link,
> +                                  unsigned int state, int depth)
> +{
> +       struct generic_pm_domain *parent = link->parent;
> +       int parent_state, ret;
>
> -               link->prev_performance_state = link->performance_state;
> -               link->performance_state = parent_state;
> -               parent_state = _genpd_reeval_performance_state(parent,
> -                                               parent_state);
> -               ret = _genpd_set_performance_state(parent, parent_state, depth + 1);
> -               if (ret)
> -                       link->performance_state = link->prev_performance_state;
> +       /* Find parent's performance state */
> +       ret = genpd_xlate_performance_state(genpd, parent, state);
> +       if (unlikely(ret < 0))
> +               return ret;
>
> -               genpd_unlock(parent);
> +       parent_state = ret;
>
> -               if (ret)
> -                       goto err;
> -       }
> +       genpd_lock_nested(parent, depth + 1);
>
> -       if (genpd->set_performance_state) {
> -               ret = genpd->set_performance_state(genpd, state);
> -               if (ret)
> -                       goto err;
> -       }
> +       link->prev_performance_state = link->performance_state;
> +       link->performance_state = parent_state;
>
> -       genpd->performance_state = state;
> -       return 0;
> +       parent_state = _genpd_reeval_performance_state(parent, parent_state);
> +       ret = _genpd_set_performance_state(parent, parent_state, depth + 1);
> +       if (ret)
> +               link->performance_state = link->prev_performance_state;
>
> -err:
> -       /* Encountered an error, lets rollback */
> -       list_for_each_entry_continue_reverse(link, &genpd->child_links,
> -                                            child_node) {
> -               parent = link->parent;
> +       genpd_unlock(parent);
>
> -               genpd_lock_nested(parent, depth + 1);
> +       return ret;
> +}
> +
> +static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
> +                                       unsigned int state, int depth)
> +{
> +       struct gpd_link *link = NULL;
> +       int ret;
> +
> +       if (state == genpd->performance_state)
> +               return 0;
>
> -               parent_state = link->prev_performance_state;
> -               link->performance_state = parent_state;
> +       /* When scaling up, propagate to parents first in normal order */
> +       if (state > genpd->performance_state) {
> +               list_for_each_entry(link, &genpd->child_links, child_node) {
> +                       ret = _genpd_set_parent_state(genpd, link, state, depth);
> +                       if (ret)
> +                               goto rollback_parents_up;
> +               }
> +       }
>
> -               parent_state = _genpd_reeval_performance_state(parent,
> -                                               parent_state);
> -               if (_genpd_set_performance_state(parent, parent_state, depth + 1)) {
> -                       pr_err("%s: Failed to roll back to %d performance state\n",
> -                              parent->name, parent_state);
> +       if (genpd->set_performance_state) {
> +               ret = genpd->set_performance_state(genpd, state);
> +               if (ret) {
> +                       if (link)
> +                               goto rollback_parents_up;
> +                       return ret;
>                 }
> +       }
>
> -               genpd_unlock(parent);
> +       /* When scaling down, propagate to parents last in reverse order */
> +       if (state < genpd->performance_state) {
> +               list_for_each_entry_reverse(link, &genpd->child_links, child_node) {
> +                       ret = _genpd_set_parent_state(genpd, link, state, depth);
> +                       if (ret)
> +                               goto rollback_parents_down;
> +               }
>         }
>
> +       genpd->performance_state = state;
> +       return 0;
> +
> +rollback_parents_up:
> +       list_for_each_entry_continue_reverse(link, &genpd->child_links, child_node)
> +               _genpd_rollback_parent_state(link, depth);
> +       return ret;
> +rollback_parents_down:
> +       list_for_each_entry_continue(link, &genpd->child_links, child_node)
> +               _genpd_rollback_parent_state(link, depth);
>         return ret;
>  }
>
>
> ---
> base-commit: 0fef202ac2f8e6d9ad21aead648278f1226b9053
> change-id: 20231205-genpd-perf-order-bf33029c25ac
>
> Best regards,
> --
> Stephan Gerhold <stephan@gerhold.net>
>