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
