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