| Message ID | 20141216001058.GA31286@dtor-ws (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 16 December 2014 at 05:40, Dmitry Torokhov <dtor@chromium.org> wrote: > 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 <dtor@chromium.org> > --- > drivers/cpufreq/cpufreq-dt.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index f56147a..4f874fa 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -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; 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 ? > + } > + > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > if (!priv) { > ret = -ENOMEM; > -- > 2.2.0.rc0.207.ga3a616c > > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 15, 2014 at 9:10 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 16 December 2014 at 05:40, Dmitry Torokhov <dtor@chromium.org> wrote: >> 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 <dtor@chromium.org> >> --- >> drivers/cpufreq/cpufreq-dt.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c >> index f56147a..4f874fa 100644 >> --- a/drivers/cpufreq/cpufreq-dt.c >> +++ b/drivers/cpufreq/cpufreq-dt.c >> @@ -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; > > 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? Not with the linux-next, but generally yes. > > 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 ? 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 December 2014 at 11:02, Dmitry Torokhov <dtor@chromium.org> wrote: > 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. Yeah, we should drop it. Its just too hard for such cases. You will take care of that in your series? For your patch: Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 15, 2014 at 9:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 16 December 2014 at 11:02, Dmitry Torokhov <dtor@chromium.org> wrote: >> 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. > > 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. > For your patch: > > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> Thanks!
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index f56147a..4f874fa 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -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;
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 <dtor@chromium.org> --- drivers/cpufreq/cpufreq-dt.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)