OPP: Fix support for required OPPs for multiple PM domains

It has turned out that having _set_required_opps() to recursively call
dev_pm_opp_set_opp() to set the required OPPs, doesn't really work as well
as we expected.

More precisely, at each recursive call to dev_pm_opp_set_opp() we are
changing an OPP for a required_dev that belongs to a required-OPP table.
The problem with this, is that we may have several devices sharing the same
required-OPP table, which leads to an incorrect behaviour in regards to
aggregating the per device votes.

To fix the problem for a required-OPP table belonging to a PM domain, which
is the only existing usecase for now, let's simply replace the call to
dev_pm_opp_set_opp() in _set_required_opps() by a call to _set_opp_level().

Moving forward we may potentially need to add support for other types of
required-OPP tables. In this case, the aggregation needs to be thought of.

Fixes: e37440e7e2c2 ("OPP: Call dev_pm_opp_set_opp() for required OPPs")
Cc: stable@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Link: https://lore.kernel.org/r/20240822224547.385095-2-ulf.hansson@linaro.org

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5f4598246a8740d68a9a3e1f5034e68025308aa7..494f8860220d97fc690ebab5ed3b7f5f04f22d73 100644 (file)
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1061,6 +1061,27 @@ static int _set_opp_bw(const struct opp_table *opp_table,
        return 0;
 }
 
+static int _set_opp_level(struct device *dev, struct dev_pm_opp *opp)
+{
+       unsigned int level = 0;
+       int ret = 0;
+
+       if (opp) {
+               if (opp->level == OPP_LEVEL_UNSET)
+                       return 0;
+
+               level = opp->level;
+       }
+
+       /* Request a new performance state through the device's PM domain. */
+       ret = dev_pm_domain_set_performance_state(dev, level);
+       if (ret)
+               dev_err(dev, "Failed to set performance state %u (%d)\n", level,
+                       ret);
+
+       return ret;
+}
+
 /* This is only called for PM domain for now */
 static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
                              struct dev_pm_opp *opp, bool up)
@@ -1091,7 +1112,7 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
                if (devs[index]) {
                        required_opp = opp ? opp->required_opps[index] : NULL;
 
-                       ret = dev_pm_opp_set_opp(devs[index], required_opp);
+                       ret = _set_opp_level(devs[index], required_opp);
                        if (ret)
                                return ret;
                }
@@ -1102,27 +1123,6 @@ static int _set_required_opps(struct device *dev, struct opp_table *opp_table,
        return 0;
 }
 
-static int _set_opp_level(struct device *dev, struct dev_pm_opp *opp)
-{
-       unsigned int level = 0;
-       int ret = 0;
-
-       if (opp) {
-               if (opp->level == OPP_LEVEL_UNSET)
-                       return 0;
-
-               level = opp->level;
-       }
-
-       /* Request a new performance state through the device's PM domain. */
-       ret = dev_pm_domain_set_performance_state(dev, level);
-       if (ret)
-               dev_err(dev, "Failed to set performance state %u (%d)\n", level,
-                       ret);
-
-       return ret;
-}
-
 static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 {
        struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
@@ -2457,18 +2457,6 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
                        }
                }
 
-               /*
-                * Add the virtual genpd device as a user of the OPP table, so
-                * we can call dev_pm_opp_set_opp() on it directly.
-                *
-                * This will be automatically removed when the OPP table is
-                * removed, don't need to handle that here.
-                */
-               if (!_add_opp_dev(virt_dev, opp_table->required_opp_tables[index])) {
-                       ret = -ENOMEM;
-                       goto err;
-               }
-
                opp_table->required_devs[index] = virt_dev;
                index++;
                name++;