linux-kernel - Re: [PATCH] opp: Handle multiple calls for same OPP table in _of_add_opp_table

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] [day] [month] [year] [list]

Date:   Wed, 16 Sep 2020 14:38:44 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>
Cc:     linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Rafael Wysocki <rjw@...ysocki.net>, stephan@...hold.net,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        linux-kernel@...r.kernel.org, naresh.kamboju@...aro.org,
        ulf.hansson@...aro.org
Subject: Re: [PATCH] opp: Handle multiple calls for same OPP table in
 _of_add_opp_table_v1()

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@...sung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  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