* [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()
@ 2020-08-27 10:05 Viresh Kumar
2020-08-27 12:14 ` Stephan Gerhold
0 siblings, 1 reply; 3+ messages in thread
From: Viresh Kumar @ 2020-08-27 10:05 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()
2020-08-27 10:05 [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd() Viresh Kumar
@ 2020-08-27 12:14 ` Stephan Gerhold
2020-08-31 5:51 ` Viresh Kumar
0 siblings, 1 reply; 3+ messages in thread
From: Stephan Gerhold @ 2020-08-27 12:14 UTC (permalink / raw)
To: Viresh Kumar
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael Wysocki, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd()
2020-08-27 12:14 ` Stephan Gerhold
@ 2020-08-31 5:51 ` Viresh Kumar
0 siblings, 0 replies; 3+ messages in thread
From: Viresh Kumar @ 2020-08-31 5:51 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, linux-pm,
Vincent Guittot, Rafael Wysocki, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-31 5:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27 10:05 [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach_genpd() Viresh Kumar
2020-08-27 12:14 ` Stephan Gerhold
2020-08-31 5:51 ` Viresh Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).