linux-kernel - Re: [PATCH] OPP: Fix support for required OPPs for multiple PM domains

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]

Date: Mon, 1 Jul 2024 17:17:48 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
	Stephen Boyd <sboyd@...nel.org>, Nikunj Kela <nkela@...cinc.com>,
	Prasad Sodagudi <psodagud@...cinc.com>, linux-pm@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-tegra@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] OPP: Fix support for required OPPs for multiple PM
 domains

On 29-06-24, 11:09, Ulf Hansson wrote:
> I get your point, but I am not sure I agree with it.
> 
> For the required-opps, the only existing use case is power/perf
> domains with performance-states, so why make the code more complicated
> than it needs to be?

That is a fair argument generally, i.e. keep things as simple as we
can, but this is a bit different. We are talking about setting the
(required) OPP for a device (parent genpd) here and it should follow
the full path.

Even in case of genpds we may want to configure more properties and
not just vote, like bandwidth, regulator, clk, etc. And so I would
really like to set the OPP in a standard way, no matter what.

> No, that's not correct. Let me try to elaborate on my setup, which is
> very similar to a use case on a Tegra platform.

Thanks, I wasn't thinking about this setup earlier.

> pd_perf0: pd-perf0 {
>     #power-domain-cells = <0>;
>     operating-points-v2 = <&opp_table_pd_perf0>;
> };
> 
> //Note: no opp-table
> pd_power4: pd-power4 {
>     #power-domain-cells = <0>;
>      power-domains = <&pd_perf0>;
> };
> 
> //Note: no opp-table
> pd_power5: pd-power5 {
>      #power-domain-cells = <0>;
>      power-domains = <&pd_perf0>;
> };
> 
> //Note: The opp_table_pm_test10 are having required-opps pointing to
> pd_perf0's opp-table.
> pm_test10 {
>     ...
>     power-domains = <&pd_power4>, <&pd_power5>;
>     power-domain-names = "perf4", "perf5";
>     operating-points-v2 = <&opp_table_pm_test10>;
> };


> In the use case above, we end up never voting on pd_power5.
 
> The DT parsing of the required-opps is already complicated and there
> seems to be endless new corner-cases showing up. Maybe we can fix this
> too, but perhaps we should simply take a step back and go for
> simplifications instead?

I truly believe that keeping a standard way of updating OPPs is the
right way to go and that will only prevent complicated corner cases
coming later on.

What about this patch instead ?

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5f4598246a87..2086292f8355 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1091,7 +1091,8 @@ 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);
+			/* Set required OPPs forcefully */
+			ret = dev_pm_opp_set_opp_forced(devs[index], required_opp, true);
 			if (ret)
 				return ret;
 		}
@@ -1365,17 +1366,8 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
 
-/**
- * dev_pm_opp_set_opp() - Configure device for OPP
- * @dev: device for which we do this operation
- * @opp: OPP to set to
- *
- * This configures the device based on the properties of the OPP passed to this
- * routine.
- *
- * Return: 0 on success, a negative error number otherwise.
- */
-int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
+static int dev_pm_opp_set_opp_forced(struct device *dev, struct dev_pm_opp *opp,
+				     bool forced)
 {
 	struct opp_table *opp_table;
 	int ret;
@@ -1386,11 +1378,25 @@ int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
 		return PTR_ERR(opp_table);
 	}
 
-	ret = _set_opp(dev, opp_table, opp, NULL, false);
+	ret = _set_opp(dev, opp_table, opp, NULL, forced);
 	dev_pm_opp_put_opp_table(opp_table);
 
 	return ret;
 }
+/**
+ * dev_pm_opp_set_opp() - Configure device for OPP
+ * @dev: device for which we do this operation
+ * @opp: OPP to set to
+ *
+ * This configures the device based on the properties of the OPP passed to this
+ * routine.
+ *
+ * Return: 0 on success, a negative error number otherwise.
+ */
+int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
+{
+	return dev_pm_opp_set_opp_forced(dev, opp, false);
+}
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_opp);
 
 /* OPP-dev Helpers */

-- 
viresh