[PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[Rajendra Nayak]

dev_pm_opp_set_rate() can now be called with freq = 0 inorder
to either drop performance or bandwidth votes or to disable
regulators on platforms which support them.
In such cases, a subsequent call to dev_pm_opp_set_rate() with
the same frequency ends up returning early because 'old_freq == freq'
Instead make it fall through and put back the dropped performance
and bandwidth votes and/or enable back the regulators.

Fixes: cd7ea582 ("opp: Make dev_pm_opp_set_rate() handle freq = 0 to drop performance votes")
Reported-by: Sajida Bhanu <sbhanu@codeaurora.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/opp/core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0c8c74a..a994f30 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -901,6 +901,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Return early if nothing to do */
 	if (old_freq == freq) {
+		if (opp_table->required_opp_tables || opp_table->regulators ||
+		    opp_table->paths)
+			goto skip_clk_only;
 		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
 			__func__, freq);
 		ret = 0;
@@ -919,6 +922,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		goto put_opp_table;
 	}
 
+skip_clk_only:
 	temp_freq = old_freq;
 	old_opp = _find_freq_ceil(opp_table, &temp_freq);
 	if (IS_ERR(old_opp)) {
@@ -954,8 +958,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
 						 opp->supplies);
 	} else {
+		ret = 0;
 		/* Only frequency scaling */
-		ret = _generic_set_opp_clk_only(dev, clk, freq);
+		if (freq != old_freq)
+			ret = _generic_set_opp_clk_only(dev, clk, freq);
 	}
 
 	/* Scaling down? Configure required OPPs after frequency */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[Sibi Sankar]

On 2020-08-10 12:36, Rajendra Nayak wrote:
> dev_pm_opp_set_rate() can now be called with freq = 0 inorder
> to either drop performance or bandwidth votes or to disable
> regulators on platforms which support them.
> In such cases, a subsequent call to dev_pm_opp_set_rate() with
> the same frequency ends up returning early because 'old_freq == freq'
> Instead make it fall through and put back the dropped performance
> and bandwidth votes and/or enable back the regulators.
> 
> Fixes: cd7ea582 ("opp: Make dev_pm_opp_set_rate() handle freq = 0 to
> drop performance votes")
> Reported-by: Sajida Bhanu <sbhanu@codeaurora.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>

Reviewed-by: Sibi Sankar <sibis@codeaurora.org>

> ---
>  drivers/opp/core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0c8c74a..a994f30 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -901,6 +901,9 @@ int dev_pm_opp_set_rate(struct device *dev,
> unsigned long target_freq)
> 
>  	/* Return early if nothing to do */
>  	if (old_freq == freq) {
> +		if (opp_table->required_opp_tables || opp_table->regulators ||
> +		    opp_table->paths)
> +			goto skip_clk_only;
>  		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to 
> do\n",
>  			__func__, freq);
>  		ret = 0;
> @@ -919,6 +922,7 @@ int dev_pm_opp_set_rate(struct device *dev,
> unsigned long target_freq)
>  		goto put_opp_table;
>  	}
> 
> +skip_clk_only:
>  	temp_freq = old_freq;
>  	old_opp = _find_freq_ceil(opp_table, &temp_freq);
>  	if (IS_ERR(old_opp)) {
> @@ -954,8 +958,10 @@ int dev_pm_opp_set_rate(struct device *dev,
> unsigned long target_freq)
>  						 IS_ERR(old_opp) ? NULL : old_opp->supplies,
>  						 opp->supplies);
>  	} else {
> +		ret = 0;
>  		/* Only frequency scaling */
> -		ret = _generic_set_opp_clk_only(dev, clk, freq);
> +		if (freq != old_freq)
> +			ret = _generic_set_opp_clk_only(dev, clk, freq);
>  	}
> 
>  	/* Scaling down? Configure required OPPs after frequency */

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[Matthias Kaehlcke]

On Mon, Aug 10, 2020 at 12:36:19PM +0530, Rajendra Nayak wrote:
> dev_pm_opp_set_rate() can now be called with freq = 0 inorder
> to either drop performance or bandwidth votes or to disable
> regulators on platforms which support them.
> In such cases, a subsequent call to dev_pm_opp_set_rate() with
> the same frequency ends up returning early because 'old_freq == freq'
> Instead make it fall through and put back the dropped performance
> and bandwidth votes and/or enable back the regulators.
> 
> Fixes: cd7ea582 ("opp: Make dev_pm_opp_set_rate() handle freq = 0 to drop performance votes")
> Reported-by: Sajida Bhanu <sbhanu@codeaurora.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>

Tested-by: Matthias Kaehlcke <mka@chromium.org>

Originally-reported-by: Matthias Kaehlcke <mka@chromium.org>
  https://patchwork.kernel.org/patch/11675369/#23514895 :P

Re: [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[Rajendra Nayak]

On 8/11/2020 10:42 PM, Matthias Kaehlcke wrote:
> On Mon, Aug 10, 2020 at 12:36:19PM +0530, Rajendra Nayak wrote:
>> dev_pm_opp_set_rate() can now be called with freq = 0 inorder
>> to either drop performance or bandwidth votes or to disable
>> regulators on platforms which support them.
>> In such cases, a subsequent call to dev_pm_opp_set_rate() with
>> the same frequency ends up returning early because 'old_freq == freq'
>> Instead make it fall through and put back the dropped performance
>> and bandwidth votes and/or enable back the regulators.
>>
>> Fixes: cd7ea582 ("opp: Make dev_pm_opp_set_rate() handle freq = 0 to drop performance votes")
>> Reported-by: Sajida Bhanu <sbhanu@codeaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> 
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> 
> Originally-reported-by: Matthias Kaehlcke <mka@chromium.org>
>    https://patchwork.kernel.org/patch/11675369/#23514895 :P

Sorry to have missed that :/
Thanks for testing.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[Stephen Boyd]

Quoting Rajendra Nayak (2020-08-10 00:06:19)
> dev_pm_opp_set_rate() can now be called with freq = 0 inorder
> to either drop performance or bandwidth votes or to disable
> regulators on platforms which support them.
> In such cases, a subsequent call to dev_pm_opp_set_rate() with
> the same frequency ends up returning early because 'old_freq == freq'
> Instead make it fall through and put back the dropped performance
> and bandwidth votes and/or enable back the regulators.
> 
> Fixes: cd7ea582 ("opp: Make dev_pm_opp_set_rate() handle freq = 0 to drop performance votes")
> Reported-by: Sajida Bhanu <sbhanu@codeaurora.org>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/opp/core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0c8c74a..a994f30 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -901,6 +901,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>  
>         /* Return early if nothing to do */
>         if (old_freq == freq) {
> +               if (opp_table->required_opp_tables || opp_table->regulators ||
> +                   opp_table->paths)
> +                       goto skip_clk_only;

This is a goto maze! Any chance we can clean this up?

        if (!opp_table->required_opp_tables && !opp_table->regulators &&
	    !opp_table->paths)
	    if (old_freq == freq) {
		    ret = 0
		    dev_dbg(..)
	    } else if (!_get_opp_count(opp_table)) {
		    ret = _generic_set_opp_clk_only(dev, clk, freq);
	    }
	} else {
		temp_freq = old_freq;
		old_opp = _find_freq_ceil(opp_table, &temp_freq);
		...
	        dev_pm_opp_put(opp);
	put_old_opp:
		if (!IS_ERR(old_opp))
			dev_pm_opp_put(old_opp);
	}
put_opp_table:
	dev_pm_opp_put_opp_table(opp_table);

And that stuff in the else should probably go to another function.

>                 dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
>                         __func__, freq);
>                 ret = 0;
> @@ -919,6 +922,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>                 goto put_opp_table;
>         }
>  
> +skip_clk_only:
>         temp_freq = old_freq;
>         old_opp = _find_freq_ceil(opp_table, &temp_freq);
>         if (IS_ERR(old_opp)) {
> @@ -954,8 +958,10 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>                                                  IS_ERR(old_opp) ? NULL : old_opp->supplies,
>                                                  opp->supplies);
>         } else {
> +               ret = 0;
>                 /* Only frequency scaling */
> -               ret = _generic_set_opp_clk_only(dev, clk, freq);
> +               if (freq != old_freq)
> +                       ret = _generic_set_opp_clk_only(dev, clk, freq);
>         }

And write this as 

	else if (freq != old_freq) {
		ret = _generic_set_opp_clk_only(..)
	} else {
		ret = 0;
	}

Re: [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[Viresh Kumar]

On 11-08-20, 14:09, Stephen Boyd wrote:
> This is a goto maze! Any chance we can clean this up?

I have sent a short series in reply to this series, please have a
look. It should look better now.

-- 
viresh

Re: [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early

[Rajendra Nayak]

On 8/13/2020 9:59 AM, Viresh Kumar wrote:
> On 11-08-20, 14:09, Stephen Boyd wrote:
>> This is a goto maze! Any chance we can clean this up?
> 
> I have sent a short series in reply to this series, please have a
> look. It should look better now.

Thanks, I was out a few days so could not get to the cleanups
that Stephen was suggesting.
I will give your series a try.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V2 1/4] opp: Enable resources again if they were disabled earlier

[Viresh Kumar]

From: Rajendra Nayak <rnayak@codeaurora.org>

dev_pm_opp_set_rate() can now be called with freq = 0 in order
to either drop performance or bandwidth votes or to disable
regulators on platforms which support them.

In such cases, a subsequent call to dev_pm_opp_set_rate() with
the same frequency ends up returning early because 'old_freq == freq'

Instead make it fall through and put back the dropped performance
and bandwidth votes and/or enable back the regulators.

Cc: v5.3+ <stable@vger.kernel.org> # v5.3+
Fixes: cd7ea582 ("opp: Make dev_pm_opp_set_rate() handle freq = 0 to drop performance votes")
Reported-by: Sajida Bhanu <sbhanu@codeaurora.org>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
Reported-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
[ Viresh: Don't skip clk_set_rate() and massaged changelog ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Hi Rajendra,

I wasn't able to test this stuff, please give it a try. I have
simplified your patch and cleaned up a bunch of stuff as well.

 drivers/opp/core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index bdb028c7793d..9668ea04cc80 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -934,10 +934,13 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
 	/* Return early if nothing to do */
 	if (old_freq == freq) {
-		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
-			__func__, freq);
-		ret = 0;
-		goto put_opp_table;
+		if (!opp_table->required_opp_tables && !opp_table->regulators &&
+		    !opp_table->paths) {
+			dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
+				__func__, freq);
+			ret = 0;
+			goto put_opp_table;
+		}
 	}
 
 	/*
-- 
2.14.1

[PATCH 2/4] opp: Track device's resources configuration status

[Viresh Kumar]

The OPP core needs to track if the resources of devices are enabled or
configured or not, as it disables the resources when target_freq is set
to 0.

Handle that with a separate variable to make it easy to maintain.

Also note that we will unconditionally call clk_set_rate() in the case
where the resources are getting enabled again. This shouldn't have any
side effects anyway.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 37 ++++++++++++++++++-------------------
 drivers/opp/opp.h  |  2 ++
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9668ea04cc80..e8882e7fd8a5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -888,22 +888,18 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	}
 
 	if (unlikely(!target_freq)) {
+		ret = 0;
+
+		if (!opp_table->enabled)
+			goto put_opp_table;
+
 		/*
 		 * Some drivers need to support cases where some platforms may
 		 * have OPP table for the device, while others don't and
 		 * opp_set_rate() just needs to behave like clk_set_rate().
 		 */
-		if (!_get_opp_count(opp_table)) {
-			ret = 0;
-			goto put_opp_table;
-		}
-
-		if (!opp_table->required_opp_tables && !opp_table->regulators &&
-		    !opp_table->paths) {
-			dev_err(dev, "target frequency can't be 0\n");
-			ret = -EINVAL;
+		if (!_get_opp_count(opp_table))
 			goto put_opp_table;
-		}
 
 		ret = _set_opp_bw(opp_table, NULL, dev, true);
 		if (ret)
@@ -915,6 +911,9 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		}
 
 		ret = _set_required_opps(dev, opp_table, NULL);
+		if (!ret)
+			opp_table->enabled = false;
+
 		goto put_opp_table;
 	}
 
@@ -933,14 +932,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	old_freq = clk_get_rate(clk);
 
 	/* Return early if nothing to do */
-	if (old_freq == freq) {
-		if (!opp_table->required_opp_tables && !opp_table->regulators &&
-		    !opp_table->paths) {
-			dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
-				__func__, freq);
-			ret = 0;
-			goto put_opp_table;
-		}
+	if (opp_table->enabled && old_freq == freq) {
+		dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
+			__func__, freq);
+		ret = 0;
+		goto put_opp_table;
 	}
 
 	/*
@@ -1001,8 +997,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 			dev_err(dev, "Failed to set required opps: %d\n", ret);
 	}
 
-	if (!ret)
+	if (!ret) {
 		ret = _set_opp_bw(opp_table, opp, dev, false);
+		if (!ret)
+			opp_table->enabled = true;
+	}
 
 put_opp:
 	dev_pm_opp_put(opp);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index e51646ff279e..bd35802acc6e 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -152,6 +152,7 @@ enum opp_table_access {
  * property).
  * @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_opp: Platform specific set_opp callback
@@ -198,6 +199,7 @@ struct opp_table {
 	bool regulator_enabled;
 	struct icc_path **paths;
 	unsigned int path_count;
+	bool enabled;
 	bool genpd_performance_state;
 	bool is_genpd;
 
-- 
2.14.1

Re: [PATCH 2/4] opp: Track device's resources configuration status

[Stephen Boyd]

Quoting Viresh Kumar (2020-08-12 21:28:59)
> The OPP core needs to track if the resources of devices are enabled or
> configured or not, as it disables the resources when target_freq is set
> to 0.
> 
> Handle that with a separate variable to make it easy to maintain.
> 
> Also note that we will unconditionally call clk_set_rate() in the case
> where the resources are getting enabled again. This shouldn't have any
> side effects anyway.

Any reason to want to do that? We'll have to grab the prepare lock in
the clk framework to figure out that there's nothing to do sometimes. If
anything, a comment may help to indicate that we call clk_set_rate()
again, but don't expect it to matter much.

Re: [PATCH 2/4] opp: Track device's resources configuration status

[Viresh Kumar]

On 15-08-20, 01:03, Stephen Boyd wrote:
> Quoting Viresh Kumar (2020-08-12 21:28:59)
> > The OPP core needs to track if the resources of devices are enabled or
> > configured or not, as it disables the resources when target_freq is set
> > to 0.
> > 
> > Handle that with a separate variable to make it easy to maintain.
> > 
> > Also note that we will unconditionally call clk_set_rate() in the case
> > where the resources are getting enabled again. This shouldn't have any
> > side effects anyway.
> 
> Any reason to want to do that?

To avoid more flags, code paths and simplicity of the code. And this
should normally happen in a corner case as well, like calling
set-rate(0) from suspend and then reinitializing things again in
resume.

> We'll have to grab the prepare lock in
> the clk framework to figure out that there's nothing to do sometimes. If
> anything, a comment may help to indicate that we call clk_set_rate()
> again, but don't expect it to matter much.

Ok.

-- 
viresh

[PATCH 3/4] opp: Reused enabled flag and remove regulator_enabled

[Viresh Kumar]

The common "enabled" flag can be used here instead of
"regulator_enabled" now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 13 +++----------
 drivers/opp/opp.h  |  2 --
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e8882e7fd8a5..5f5da257f58a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	 * Enable the regulator after setting its voltages, otherwise it breaks
 	 * some boot-enabled regulators.
 	 */
-	if (unlikely(!opp_table->regulator_enabled)) {
+	if (unlikely(!opp_table->enabled)) {
 		ret = regulator_enable(reg);
 		if (ret < 0)
 			dev_warn(dev, "Failed to enable regulator: %d", ret);
-		else
-			opp_table->regulator_enabled = true;
 	}
 
 	return 0;
@@ -905,10 +903,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		if (ret)
 			goto put_opp_table;
 
-		if (opp_table->regulator_enabled) {
-			regulator_disable(opp_table->regulators[0]);
-			opp_table->regulator_enabled = false;
-		}
+		regulator_disable(opp_table->regulators[0]);
 
 		ret = _set_required_opps(dev, opp_table, NULL);
 		if (!ret)
@@ -1795,11 +1790,9 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
 
-	if (opp_table->regulator_enabled) {
+	if (opp_table->enabled) {
 		for (i = opp_table->regulator_count - 1; i >= 0; i--)
 			regulator_disable(opp_table->regulators[i]);
-
-		opp_table->regulator_enabled = false;
 	}
 
 	for (i = opp_table->regulator_count - 1; i >= 0; i--)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index bd35802acc6e..0c3de3f6db5c 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -147,7 +147,6 @@ enum opp_table_access {
  * @clk: Device's clock handle
  * @regulators: Supply regulators
  * @regulator_count: Number of power supply regulators. Its value can be -1
- * @regulator_enabled: Set to true if regulators were previously enabled.
  * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt
  * property).
  * @paths: Interconnect path handles
@@ -196,7 +195,6 @@ struct opp_table {
 	struct clk *clk;
 	struct regulator **regulators;
 	int regulator_count;
-	bool regulator_enabled;
 	struct icc_path **paths;
 	unsigned int path_count;
 	bool enabled;
-- 
2.14.1

Re: [PATCH 3/4] opp: Reused enabled flag and remove regulator_enabled

[Stephen Boyd]

Quoting Viresh Kumar (2020-08-12 21:29:00)
> The common "enabled" flag can be used here instead of
> "regulator_enabled" now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Why not put this before the other patch? And mention that it will be
reused in another place soon? Then the previous patch won't look like
we're adding a variable to the struct when it is only used inside a
single function. Or at least squash it with the previous patch.

> ---
>  drivers/opp/core.c | 13 +++----------
>  drivers/opp/opp.h  |  2 --
>  2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e8882e7fd8a5..5f5da257f58a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>          * Enable the regulator after setting its voltages, otherwise it breaks
>          * some boot-enabled regulators.
>          */
> -       if (unlikely(!opp_table->regulator_enabled)) {
> +       if (unlikely(!opp_table->enabled)) {
>                 ret = regulator_enable(reg);
>                 if (ret < 0)
>                         dev_warn(dev, "Failed to enable regulator: %d", ret);
> -               else
> -                       opp_table->regulator_enabled = true;

A quick glance makes this look unsafe now because we're only checking
'enabled' and not actually setting it when this function is called. I
have to go back to the previous patch to understand where enabled is now
set to confirm that it is OK. If it was all one patch all the context
would be here.

Re: [PATCH 3/4] opp: Reused enabled flag and remove regulator_enabled

[Viresh Kumar]

On 15-08-20, 00:55, Stephen Boyd wrote:
> Quoting Viresh Kumar (2020-08-12 21:29:00)
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index e8882e7fd8a5..5f5da257f58a 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
> >          * Enable the regulator after setting its voltages, otherwise it breaks
> >          * some boot-enabled regulators.
> >          */
> > -       if (unlikely(!opp_table->regulator_enabled)) {
> > +       if (unlikely(!opp_table->enabled)) {
> >                 ret = regulator_enable(reg);
> >                 if (ret < 0)
> >                         dev_warn(dev, "Failed to enable regulator: %d", ret);
> > -               else
> > -                       opp_table->regulator_enabled = true;
> 
> A quick glance makes this look unsafe now because we're only checking
> 'enabled' and not actually setting it when this function is called. I
> have to go back to the previous patch to understand where enabled is now
> set to confirm that it is OK. If it was all one patch all the context
> would be here.

The only case where things can go crazy are the cases where (for
example) clk_set_rate() fails, or something like that which would be a
bug and it shouldn't bother in the normal working of this code.

-- 
viresh

Re: [PATCH 3/4] opp: Reused enabled flag and remove regulator_enabled

[Rajendra Nayak]

On 8/13/2020 9:59 AM, Viresh Kumar wrote:
> The common "enabled" flag can be used here instead of
> "regulator_enabled" now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/opp/core.c | 13 +++----------
>   drivers/opp/opp.h  |  2 --
>   2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e8882e7fd8a5..5f5da257f58a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>   	 * Enable the regulator after setting its voltages, otherwise it breaks
>   	 * some boot-enabled regulators.
>   	 */
> -	if (unlikely(!opp_table->regulator_enabled)) {
> +	if (unlikely(!opp_table->enabled)) {
>   		ret = regulator_enable(reg);
>   		if (ret < 0)
>   			dev_warn(dev, "Failed to enable regulator: %d", ret);
> -		else
> -			opp_table->regulator_enabled = true;
>   	}
>   
>   	return 0;
> @@ -905,10 +903,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>   		if (ret)
>   			goto put_opp_table;
>   
> -		if (opp_table->regulator_enabled) {
> -			regulator_disable(opp_table->regulators[0]);
> -			opp_table->regulator_enabled = false;
> -		}
> +		regulator_disable(opp_table->regulators[0]);

unconditionally calling regulator_disable() here based on the common
'enabled' flag results in a crash on platforms without regulators
associated with the opp table.

>   
>   		ret = _set_required_opps(dev, opp_table, NULL);
>   		if (!ret)
> @@ -1795,11 +1790,9 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>   	/* Make sure there are no concurrent readers while updating opp_table */
>   	WARN_ON(!list_empty(&opp_table->opp_list));
>   
> -	if (opp_table->regulator_enabled) {
> +	if (opp_table->enabled) {
>   		for (i = opp_table->regulator_count - 1; i >= 0; i--)
>   			regulator_disable(opp_table->regulators[i]);
> -
> -		opp_table->regulator_enabled = false;
>   	}
>   
>   	for (i = opp_table->regulator_count - 1; i >= 0; i--)
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index bd35802acc6e..0c3de3f6db5c 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -147,7 +147,6 @@ enum opp_table_access {
>    * @clk: Device's clock handle
>    * @regulators: Supply regulators
>    * @regulator_count: Number of power supply regulators. Its value can be -1
> - * @regulator_enabled: Set to true if regulators were previously enabled.
>    * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt
>    * property).
>    * @paths: Interconnect path handles
> @@ -196,7 +195,6 @@ struct opp_table {
>   	struct clk *clk;
>   	struct regulator **regulators;
>   	int regulator_count;
> -	bool regulator_enabled;
>   	struct icc_path **paths;
>   	unsigned int path_count;
>   	bool enabled;
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 4/4] opp: Split out _opp_set_rate_zero()

[Viresh Kumar]

Create separate routine _opp_set_rate_zero() to handle !target_freq
case.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 52 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5f5da257f58a..e198b57efcf8 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -860,6 +860,34 @@ int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
 
+static int _opp_set_rate_zero(struct device *dev, struct opp_table *opp_table)
+{
+	int ret;
+
+	if (!opp_table->enabled)
+		return 0;
+
+	/*
+	 * Some drivers need to support cases where some platforms may
+	 * have OPP table for the device, while others don't and
+	 * opp_set_rate() just needs to behave like clk_set_rate().
+	 */
+	if (!_get_opp_count(opp_table))
+		return 0;
+
+	ret = _set_opp_bw(opp_table, NULL, dev, true);
+	if (ret)
+		return ret;
+
+	regulator_disable(opp_table->regulators[0]);
+
+	ret = _set_required_opps(dev, opp_table, NULL);
+	if (!ret)
+		opp_table->enabled = false;
+
+	return ret;
+}
+
 /**
  * dev_pm_opp_set_rate() - Configure new OPP based on frequency
  * @dev:	 device for which we do this operation
@@ -886,29 +914,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	}
 
 	if (unlikely(!target_freq)) {
-		ret = 0;
-
-		if (!opp_table->enabled)
-			goto put_opp_table;
-
-		/*
-		 * Some drivers need to support cases where some platforms may
-		 * have OPP table for the device, while others don't and
-		 * opp_set_rate() just needs to behave like clk_set_rate().
-		 */
-		if (!_get_opp_count(opp_table))
-			goto put_opp_table;
-
-		ret = _set_opp_bw(opp_table, NULL, dev, true);
-		if (ret)
-			goto put_opp_table;
-
-		regulator_disable(opp_table->regulators[0]);
-
-		ret = _set_required_opps(dev, opp_table, NULL);
-		if (!ret)
-			opp_table->enabled = false;
-
+		ret = _opp_set_rate_zero(dev, opp_table);
 		goto put_opp_table;
 	}
 
-- 
2.14.1