linux-kernel - Re: [PATCH v2] opp: Power on (virtual) power domains managed by the OPP core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200831121457.2v6avendroclvrn5@vireshk-i7>
Date:   Mon, 31 Aug 2020 17:44:57 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Stephan Gerhold <stephan@...hold.net>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Nishanth Menon <nm@...com>, Stephen Boyd <sboyd@...nel.org>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Niklas Cassel <nks@...wful.org>
Subject: Re: [PATCH v2] opp: Power on (virtual) power domains managed by the
 OPP core

On 26-08-20, 11:33, Stephan Gerhold wrote:
> dev_pm_opp_attach_genpd() allows attaching an arbitrary number of
> power domains to an OPP table. In that case, the genpd core will
> create a virtual device for each of the power domains.
> 
> At the moment, the OPP core only calls
> dev_pm_genpd_set_performance_state() on these virtual devices.
> It does not attempt to power on the power domains. Therefore
> the required power domain might never get turned on.
> 
> So far, dev_pm_opp_attach_genpd() is only used in qcom-cpufreq-nvmem.c
> to attach the CPR power domain to the CPU OPP table. The CPR driver
> does not check if it was actually powered on so this did not cause
> any problems. However, other drivers (e.g. rpmpd) might ignore the
> performance state until the power domain is actually powered on.
> 
> Since these virtual devices are managed exclusively by the OPP core,
> I would say that it should also be responsible to ensure they are
> enabled.
> 
> This commit implements this similar to the non-virtual power domains;
> we create device links for each of attached power domains so that they
> are turned on whenever the consumer device is active.
> 
> Signed-off-by: Stephan Gerhold <stephan@...hold.net>
> ---

Applied with some changes, hope that is fine:

Signed-off-by: Stephan Gerhold <stephan@...hold.net>
[ Viresh: Rearranged the code to remove extra loop and minor cleanup ]
Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
---
 drivers/opp/core.c | 37 ++++++++++++++++++++++++++++++++++++-
 drivers/opp/opp.h  |  1 +
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e65174725a4d..b608b0253079 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 #include "opp.h"
@@ -1967,10 +1968,15 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
 		if (!opp_table->genpd_virt_devs[index])
 			continue;
 
+		if (opp_table->genpd_virt_links[index])
+			device_link_del(opp_table->genpd_virt_links[index]);
+
 		dev_pm_domain_detach(opp_table->genpd_virt_devs[index], false);
 		opp_table->genpd_virt_devs[index] = NULL;
 	}
 
+	kfree(opp_table->genpd_virt_links);
+	opp_table->genpd_virt_links = NULL;
 	kfree(opp_table->genpd_virt_devs);
 	opp_table->genpd_virt_devs = NULL;
 }
@@ -2002,8 +2008,10 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 {
 	struct opp_table *opp_table;
 	struct device *virt_dev;
+	struct device_link *dev_link;
 	int index = 0, ret = -EINVAL;
 	const char **name = names;
+	u32 flags;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
 	if (IS_ERR(opp_table))
@@ -2030,6 +2038,21 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 	if (!opp_table->genpd_virt_devs)
 		goto unlock;
 
+	opp_table->genpd_virt_links = kcalloc(opp_table->required_opp_count,
+					      sizeof(*opp_table->genpd_virt_links),
+					      GFP_KERNEL);
+	if (!opp_table->genpd_virt_links) {
+		kfree(opp_table->genpd_virt_devs);
+		opp_table->genpd_virt_devs = NULL;
+		goto unlock;
+	}
+
+	/* Turn on power domain initially if consumer is active */
+	pm_runtime_get_noresume(dev);
+	flags = DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS;
+	if (pm_runtime_active(dev))
+		flags |= DL_FLAG_RPM_ACTIVE;
+
 	while (*name) {
 		if (index >= opp_table->required_opp_count) {
 			dev_err(dev, "Index can't be greater than required-opp-count - 1, %s (%d : %d)\n",
@@ -2043,12 +2066,23 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 			dev_err(dev, "Couldn't attach to pm_domain: %d\n", ret);
 			goto err;
 		}
-
 		opp_table->genpd_virt_devs[index] = virt_dev;
+
+		/* Create device links to manage runtime PM */
+		dev_link = device_link_add(dev, opp_table->genpd_virt_devs[index],
+					   flags);
+		if (!dev_link) {
+			dev_err(dev, "Failed to create device link\n");
+			goto err;
+		}
+		opp_table->genpd_virt_links[index] = dev_link;
+
 		index++;
 		name++;
 	}
 
+	pm_runtime_put(dev);
+
 	if (virt_devs)
 		*virt_devs = opp_table->genpd_virt_devs;
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
@@ -2056,6 +2090,7 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 	return opp_table;
 
 err:
+	pm_runtime_put(dev);
 	_opp_detach_genpd(opp_table);
 unlock:
 	mutex_unlock(&opp_table->genpd_virt_dev_lock);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 78e876ec803e..be5526cdbdba 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -186,6 +186,7 @@ struct opp_table {
 
 	struct mutex genpd_virt_dev_lock;
 	struct device **genpd_virt_devs;
+	struct device_link **genpd_virt_links;
 	struct opp_table **required_opp_tables;
 	unsigned int required_opp_count;