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

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

[Viresh Kumar]

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>
---
 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

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

[Ulf Hansson]

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
>