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