cpufreq-dt: defer probing if OPP table is not ready
PATCH 1/1 cpufreq-dt: defer probing if OPP table is not ready
cpufreq-dt driver supports mode when OPP table is provided by platform code and not device tree. However on certain platforms code that fills OPP table may run after cpufreq driver tries to initialize, so let's report -EPROBE_DEFER if we do not find any entires in OPP table for the CPU.
--- drivers/cpufreq/cpufreq-dt.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
drivers/cpufreq/cpufreq-dt.c +13 -0
@@ -211,6 +211,19 @@ static int cpufreq_init(struct cpufreq_policy *policy)/* OPPs might be populated at runtime, don't check for error here */ of_init_opp_table (cpu_dev); +/* +* But we need OPP table to function so if it is not there let's +* give platform code chance to provide it for us. +*/ +rcu_read_lock (); + ret =dev_pm_opp_get_opp_count (cpu_dev); +rcu_read_unlock (); +if (ret <=0 ) { +pr_debug ("OPP table is not ready, deferring probe\n" ); + ret = -EPROBE_DEFER ; + goto out_free_opp; + } + priv =kzalloc (sizeof (*priv),GFP_KERNEL );if (!priv) { ret = -ENOMEM ;
Signature · 5 lines
2.2.0.rc0.207.ga3a616c -- Dmitry
Viresh Kumar
maintainerQuoted 20 lines · Dmitry Torokhov
cpufreq-dt driver supports mode when OPP table is provided by platform code and not device tree. However on certain platforms code that fills OPP table may run after cpufreq driver tries to initialize, so let's report -EPROBE_DEFER if we do not find any entires in OPP table for the CPU.
Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/cpufreq/cpufreq-dt.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/cpufreq/cpufreq-dt.c +1
-0@@ -211,6 +211,19 @@ static int cpufreq_init(struct cpufreq_policy *policy)/* OPPs might be populated at runtime, don't check for error here */ of_init_opp_table (cpu_dev); +/*
drivers/cpufreq/cpufreq-dt.c static int cpufreq_init(struct cpufreq_policy *policy) · line 224
Quoted 10 lines
drivers/cpufreq/cpufreq-dt.c +10
-0+* But we need OPP table to function so if it is not there let's +* give platform code chance to provide it for us. +*/ +rcu_read_lock (); + ret =dev_pm_opp_get_opp_count (cpu_dev); +rcu_read_unlock (); +if (ret <=0 ) { +pr_debug ("OPP table is not ready, deferring probe\n" ); + ret = -EPROBE_DEFER ; + goto out_free_opp;
Hmm, so we are trying to free opps while we failed to find one.
Are you sure you have tested this on one such platform where we fail here?
Because this is what I see:
void of_free_opp_table(struct device *dev)
{
struct device_opp *dev_opp;
struct dev_pm_opp *opp, *tmp;
/* Check for existing list for 'dev' */
dev_opp = find_device_opp(dev);
if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev),
PTR_ERR(dev_opp)))
return;
...
}And we probably will hit this WARN(), wouldn't we ?
Quoted 11 lines
drivers/cpufreq/cpufreq-dt.c +2
-1+ } + priv =kzalloc (sizeof (*priv),GFP_KERNEL );if (!priv) { ret = -ENOMEM ; --2.2.0.rc0.207.ga3a616c
--
Dmitry
Quoted 34 lines · Viresh Kumar
Quoted 20 lines · Dmitry Torokhov
cpufreq-dt driver supports mode when OPP table is provided by platform code and not device tree. However on certain platforms code that fills OPP table may run after cpufreq driver tries to initialize, so let's report -EPROBE_DEFER if we do not find any entires in OPP table for the CPU.
Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/cpufreq/cpufreq-dt.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)drivers/cpufreq/cpufreq-dt.c +1
-0@@ -211,6 +211,19 @@ static int cpufreq_init(struct cpufreq_policy *policy)/* OPPs might be populated at runtime, don't check for error here */ of_init_opp_table (cpu_dev); +/* drivers/cpufreq/cpufreq-dt.c static int cpufreq_init(struct cpufreq_policy *policy) · line 224
Quoted 10 lines
drivers/cpufreq/cpufreq-dt.c +10
-0+* But we need OPP table to function so if it is not there let's +* give platform code chance to provide it for us. +*/ +rcu_read_lock (); + ret =dev_pm_opp_get_opp_count (cpu_dev); +rcu_read_unlock (); +if (ret <=0 ) { +pr_debug ("OPP table is not ready, deferring probe\n" ); + ret = -EPROBE_DEFER ; + goto out_free_opp;Hmm, so we are trying to free opps while we failed to find one.
Quoted 2 lines
Are you sure you have tested this on one such platform where we fail here?
Not with the linux-next, but generally yes.
Quoted 18 lines
Because this is what I see:
void
of_free_opp_table(struct device *dev){ struct device_opp *dev_opp; struct dev_pm_opp *opp, *tmp; /* Check for existing list for 'dev' */ dev_opp = find_device_opp(dev); if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev), PTR_ERR(dev_opp))) return; ... }
Quoted 1 line
And we probably will hit this
WARN(), wouldn't we ?
Yes we will. Which simply means that this WARN is stupid. We also will hit it if there is no opp table and the allocation below fails; or if it succeeds then dev_pm_opp_init_cpufreq_table() will fail and we'll hit this code path again.
We should either drop that WARN() or handle -ENODEV there properly.
Thanks,
Dmitry
Quoted 6 lines · Dmitry Torokhov
Yes we will. Which simply means that this WARN is stupid. We also will hit it if there is no opp table and the allocation below fails; or if it succeeds then
dev_pm_opp_init_cpufreq_table()will fail and we'll hit this code path again.We should either drop that
WARN()or handle-ENODEVthere properly.
Yeah, we should drop it. Its just too hard for such cases. You will take care of that in your series?
For your patch:
--
viresh
Quoted 11 lines · Viresh Kumar
Quoted 6 lines · Dmitry Torokhov
Yes we will. Which simply means that this WARN is stupid. We also will hit it if there is no opp table and the allocation below fails; or if it succeeds then
dev_pm_opp_init_cpufreq_table()will fail and we'll hit this code path again.We should either drop that
WARN()or handle-ENODEVthere properly.Yeah, we should drop it. Its just too hard for such cases. You will take care of that in your series?
Yes, I'll address other comments and resubmit.
Quoted 3 lines
For your patch:
Reviewed-by Viresh Kumar <[email protected]>
Thanks!
Signature · 1 line
Dmitry