Re: [PATCH] opp: Handle multiple calls for same OPP table in _of_add_opp_table_v1()

[Viresh Kumar]

On 01-09-20, 15:57, Viresh Kumar wrote:
> Until now for V1 OPP bindings we used to call
> dev_pm_opp_of_cpumask_add_table() first and then
> dev_pm_opp_set_sharing_cpus() in the cpufreq-dt driver.
> 
> A later patch will though update the cpufreq-dt driver to optimize the
> code a bit and we will call dev_pm_opp_set_sharing_cpus() first followed
> by dev_pm_opp_of_cpumask_add_table(), which doesn't work well today as
> it tries to re parse the OPP entries. This should work nevertheless for
> V1 bindings as the same works for V2 bindings.
> 
> Adapt the same approach from V2 bindings and fix this.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  drivers/opp/of.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index d8b623cc015a..6fc56660fa52 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -886,6 +886,16 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
>  	const __be32 *val;
>  	int nr, ret = 0;
>  
> +	mutex_lock(&opp_table->lock);
> +	if (opp_table->parsed_static_opps) {
> +		opp_table->parsed_static_opps++;
> +		mutex_unlock(&opp_table->lock);
> +		return 0;
> +	}
> +
> +	opp_table->parsed_static_opps = 1;
> +	mutex_unlock(&opp_table->lock);
> +
>  	prop = of_find_property(dev->of_node, "operating-points", NULL);
>  	if (!prop)
>  		return -ENODEV;
> @@ -902,10 +912,6 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
>  		return -EINVAL;
>  	}
>  
> -	mutex_lock(&opp_table->lock);
> -	opp_table->parsed_static_opps = 1;
> -	mutex_unlock(&opp_table->lock);
> -
>  	val = prop->value;
>  	while (nr) {
>  		unsigned long freq = be32_to_cpup(val++) * 1000;

This wasn't enough. Merged this diff as well to this patch:

 drivers/opp/of.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index e72753be7dc7..aa829a569825 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -916,10 +916,14 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
        mutex_unlock(&opp_table->lock);
 
        prop = of_find_property(dev->of_node, "operating-points", NULL);
-       if (!prop)
-               return -ENODEV;
-       if (!prop->value)
-               return -ENODATA;
+       if (!prop) {
+               ret = -ENODEV;
+               goto remove_static_opp;
+       }
+       if (!prop->value) {
+               ret = -ENODATA;
+               goto remove_static_opp;
+       }
 
        /*
         * Each OPP is a set of tuples consisting of frequency and
@@ -928,7 +932,8 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
        nr = prop->length / sizeof(u32);
        if (nr % 2) {
                dev_err(dev, "%s: Invalid OPP table\n", __func__);
-               return -EINVAL;
+               ret = -EINVAL;
+               goto remove_static_opp;
        }
 
        val = prop->value;
@@ -940,12 +945,14 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
                if (ret) {
                        dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
                                __func__, freq, ret);
-                       _opp_remove_all_static(opp_table);
-                       return ret;
+                       goto remove_static_opp;
                }
                nr -= 2;
        }
 
+remove_static_opp:
+       _opp_remove_all_static(opp_table);
+
        return ret;
 }

-- 
viresh