[PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()

[PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()

[Viresh Kumar]

Since commit c0ab9e0812da ("opp: Allocate genpd_virt_devs from
dev_pm_opp_attach_genpd()"), the allocation of the virtual devices is
moved to dev_pm_opp_attach_genpd() and this check isn't required anymore
as it will always fail. Drop it.

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

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 8b3c3986f589..000d0fcb4680 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2031,12 +2031,6 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 			goto err;
 		}
 
-		if (opp_table->genpd_virt_devs[index]) {
-			dev_err(dev, "Genpd virtual device already set %s\n",
-				*name);
-			goto err;
-		}
-
 		virt_dev = dev_pm_domain_attach_by_name(dev, *name);
 		if (IS_ERR(virt_dev)) {
 			ret = PTR_ERR(virt_dev);
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()

[Stephan Gerhold]

On Thu, Aug 27, 2020 at 03:35:15PM +0530, Viresh Kumar wrote:
> Since commit c0ab9e0812da ("opp: Allocate genpd_virt_devs from
> dev_pm_opp_attach_genpd()"), the allocation of the virtual devices is
> moved to dev_pm_opp_attach_genpd() and this check isn't required anymore
> as it will always fail. Drop it.
> 

Only partially related to this patch, but actually I noticed that
dev_pm_opp_attach_genpd() does not work correctly if it is called
multiple times.

For example, qcom-cpufreq-nvmem calls this for every CPU because it is
not aware that the OPP table is shared between the CPUs.
dev_pm_opp_attach_genpd() does not check if opp_table->genpd_virt_devs
is already set, so when it is called again for other CPUs we will:

  - Cause a memory leak (opp_table->genpd_virt_devs is just replaced
    with new memory)
  - Attach the power domains multiple times
  - Never detach the power domains from earlier calls
  - Crash when dev_pm_opp_detach_genpd() is called the second time

Oh well. :)

I think the function should just return and do nothing if the power
domains were already attached, just like dev_pm_opp_set_supported_hw()
etc. But this is a bit complicated to implement with the "virt_devs"
parameter, since callers will probably assume that to be valid if we
return success.

Another advantage of my proposal to remove the virt_devs parameter [1] :)

Stephan

Re: [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()

[Viresh Kumar]

On 27-08-20, 14:14, Stephan Gerhold wrote:
> Only partially related to this patch, but actually I noticed that
> dev_pm_opp_attach_genpd() does not work correctly if it is called
> multiple times.
> 
> For example, qcom-cpufreq-nvmem calls this for every CPU because it is
> not aware that the OPP table is shared between the CPUs.

It could have called it from cpufreq_driver->init, but yeah I see the
problem here.

> dev_pm_opp_attach_genpd() does not check if opp_table->genpd_virt_devs
> is already set, so when it is called again for other CPUs we will:
> 
>   - Cause a memory leak (opp_table->genpd_virt_devs is just replaced
>     with new memory)
>   - Attach the power domains multiple times
>   - Never detach the power domains from earlier calls
>   - Crash when dev_pm_opp_detach_genpd() is called the second time
> 
> Oh well. :)
> 
> I think the function should just return and do nothing if the power
> domains were already attached, just like dev_pm_opp_set_supported_hw()
> etc. But this is a bit complicated to implement with the "virt_devs"
> parameter, since callers will probably assume that to be valid if we
> return success.

Or maybe at least make it work by returning the OPP table and not
setting the virt_devs.

> Another advantage of my proposal to remove the virt_devs parameter [1] :)

Yes, I do see the advantage there, lets see where that discussion
goes.

-- 
viresh