Re: [PATCH] OPP: don't drop performance constraint on OPP table removal

[Ulf Hansson <ulf.hansson@linaro.org>]

On Wed, 14 Jun 2023 at 09:57, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> This code was added (long back) by commit 009acd196fc8 ("PM / OPP:
> Support updating performance state of device's power domain") and at
> that time the `opp->pstate` field was used to store the performance
> state required by a device's OPP.
>
> Over time that changed and the `->pstate` field is now used only for
> genpd devices and consumer devices access that via the required-opps
> instead.
>
> Because of all these changes, _opp_table_kref_release() now drops the
> constraint only when the genpd's OPP table gets freed and not the
> device's. Which is definitely not what we wanted. And dropping the
> constraint doesn't have much meaning as the genpd itself is going away.
>
> Moreover, if we want to drop constraints here, then just dropping the
> performance constraint alone isn't sufficient as there are other
> resource constraints like clk, regulator, etc. too, which must be
> handled.
>
> Probably the right thing to do here is to leave this decision to the
> consumers, which can call `dev_pm_opp_set_rate(dev, 0)` or similar APIs
> to drop all constraints properly. Which many of the consumers already
> do.
>
> Remove the special code, which is broken anyway.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

One good step to clean up the mess. Thanks!

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

> ---
>  drivers/opp/core.c | 10 +---------
>  drivers/opp/of.c   |  8 --------
>  drivers/opp/opp.h  |  2 --
>  3 files changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 9f918077cd62..7290168ec806 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1522,16 +1522,8 @@ static void _opp_table_kref_release(struct kref *kref)
>
>         WARN_ON(!list_empty(&opp_table->opp_list));
>
> -       list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node) {
> -               /*
> -                * The OPP table is getting removed, drop the performance state
> -                * constraints.
> -                */
> -               if (opp_table->genpd_performance_state)
> -                       dev_pm_genpd_set_performance_state((struct device *)(opp_dev->dev), 0);
> -
> +       list_for_each_entry_safe(opp_dev, temp, &opp_table->dev_list, node)
>                 _remove_opp_dev(opp_dev, opp_table);
> -       }
>
>         mutex_destroy(&opp_table->genpd_virt_dev_lock);
>         mutex_destroy(&opp_table->lock);
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index ac2179d5da4c..943c7fb7402b 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1034,14 +1034,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
>                 goto remove_static_opp;
>         }
>
> -       list_for_each_entry(opp, &opp_table->opp_list, node) {
> -               /* Any non-zero performance state would enable the feature */
> -               if (opp->pstate) {
> -                       opp_table->genpd_performance_state = true;
> -                       break;
> -               }
> -       }
> -
>         lazy_link_required_opp_table(opp_table);
>
>         return 0;
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index eb71385d96c1..3805b92a6100 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -182,7 +182,6 @@ enum opp_table_access {
>   * @paths: Interconnect path handles
>   * @path_count: Number of interconnect paths
>   * @enabled: Set to true if the device's resources are enabled/configured.
> - * @genpd_performance_state: Device's power domain support performance state.
>   * @is_genpd: Marks if the OPP table belongs to a genpd.
>   * @set_required_opps: Helper responsible to set required OPPs.
>   * @dentry:    debugfs dentry pointer of the real device directory (not links).
> @@ -233,7 +232,6 @@ struct opp_table {
>         struct icc_path **paths;
>         unsigned int path_count;
>         bool enabled;
> -       bool genpd_performance_state;
>         bool is_genpd;
>         int (*set_required_opps)(struct device *dev,
>                 struct opp_table *opp_table, struct dev_pm_opp *opp, bool scaling_down);
> --
> 2.31.1.272.g89b43f80a514
>