diff mbox series

opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()

Message ID 88c8522b556d15bd44b8388d47cf25ac6f06b057.1598522635.git.viresh.kumar@linaro.org
State New
Headers show
Series opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd() | expand

Commit Message

Viresh Kumar Aug. 27, 2020, 10:05 a.m. UTC
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(-)

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

Stephan Gerhold Aug. 27, 2020, 12:14 p.m. UTC | #1
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
Viresh Kumar Aug. 31, 2020, 5:51 a.m. UTC | #2
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
diff mbox series

Patch

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);