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(+)
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;
--
2.2.0.rc0.207.ga3a616c
--
Dmitry
On 16 December 2014 at 05:40, Dmitry Torokhov <[email protected]> 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 <[email protected]>
> ---
> 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
On Mon, Dec 15, 2014 at 9:10 PM, Viresh Kumar <[email protected]> wrote:
> On 16 December 2014 at 05:40, Dmitry Torokhov <[email protected]> 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 <[email protected]>
>> ---
>> 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
On 16 December 2014 at 11:02, Dmitry Torokhov <[email protected]> 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 <[email protected]>
--
viresh
On Mon, Dec 15, 2014 at 9:38 PM, Viresh Kumar <[email protected]> wrote:
> On 16 December 2014 at 11:02, Dmitry Torokhov <[email protected]> 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 <[email protected]>
Thanks!
--
Dmitry