| Message ID | 20210325043129.2255918-1-quanyang.wang@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: dt: check the error returned by dev_pm_opp_of_cpumask_add_table | expand |
On 25-03-21, 12:31, quanyang.wang@windriver.com wrote: > From: Quanyang Wang <quanyang.wang@windriver.com> > > The function dev_pm_opp_of_cpumask_add_table may return zero or an > error. When it returns an error, this means that no OPP table is > added for the cpumask because _dev_pm_opp_cpumask_remove_table is > called to free all OPPs associated with the cpu devices in the error > label "remove_table". So continuing to run the next function > dev_pm_opp_get_opp_count is meaningless since it always return the > count value as 0. > > There is another reason why we should check the error returned by > dev_pm_opp_of_cpumask_add_table is that it may return -EPROBE_DEFER > which comes from clk_get(dev, NULL) in _update_opp_table_clk. When > the clk for cpu device isn't ready, dt_cpufreq_probe should be deferred > and wait to be called again. But if we ignore the return error of > dev_pm_opp_of_cpumask_add_table, dt_cpufreq_probe will return -ENODEV > because dev_pm_opp_get_opp_count returns the count value as 0, > the cpufreq-dt driver will fail with the error log as below: > > [ 0.724069] cpu cpu0: OPP table can't be empty > > Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com> > --- > drivers/cpufreq/cpufreq-dt.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index b1e1bdc63b01..f24359f47b1a 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -255,10 +255,16 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu) > * before updating priv->cpus. Otherwise, we will end up creating > * duplicate OPPs for the CPUs. > * > - * OPPs might be populated at runtime, don't check for error here. As the comment (which you removed) clearly says, the OPPs maybe added at runtime, don't check for error here. When we say runtime, we mean someone may have called dev_pm_opp_add() for the devices. > + * We need check the return value here, if it is non-zero, there is > + * need to go on. > */ > - if (!dev_pm_opp_of_cpumask_add_table(priv->cpus)) > - priv->have_static_opps = true; > + ret = dev_pm_opp_of_cpumask_add_table(priv->cpus); > + if (ret) { > + dev_err(cpu_dev, "Failed to add OPP table for CPUs\n"); > + goto out; > + } > + > + priv->have_static_opps = true; > > /* > * The OPP table must be initialized, statically or dynamically, by this
Hi Viresh, On 3/25/21 12:45 PM, Viresh Kumar wrote: > On 25-03-21, 12:31, quanyang.wang@windriver.com wrote: >> From: Quanyang Wang <quanyang.wang@windriver.com> >> >> The function dev_pm_opp_of_cpumask_add_table may return zero or an >> error. When it returns an error, this means that no OPP table is >> added for the cpumask because _dev_pm_opp_cpumask_remove_table is >> called to free all OPPs associated with the cpu devices in the error >> label "remove_table". So continuing to run the next function >> dev_pm_opp_get_opp_count is meaningless since it always return the >> count value as 0. >> >> There is another reason why we should check the error returned by >> dev_pm_opp_of_cpumask_add_table is that it may return -EPROBE_DEFER >> which comes from clk_get(dev, NULL) in _update_opp_table_clk. When >> the clk for cpu device isn't ready, dt_cpufreq_probe should be deferred >> and wait to be called again. But if we ignore the return error of >> dev_pm_opp_of_cpumask_add_table, dt_cpufreq_probe will return -ENODEV >> because dev_pm_opp_get_opp_count returns the count value as 0, >> the cpufreq-dt driver will fail with the error log as below: >> >> [ 0.724069] cpu cpu0: OPP table can't be empty >> >> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com> >> --- >> drivers/cpufreq/cpufreq-dt.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c >> index b1e1bdc63b01..f24359f47b1a 100644 >> --- a/drivers/cpufreq/cpufreq-dt.c >> +++ b/drivers/cpufreq/cpufreq-dt.c >> @@ -255,10 +255,16 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu) >> * before updating priv->cpus. Otherwise, we will end up creating >> * duplicate OPPs for the CPUs. >> * >> - * OPPs might be populated at runtime, don't check for error here. > As the comment (which you removed) clearly says, the OPPs maybe added > at runtime, don't check for error here. > > When we say runtime, we mean someone may have called dev_pm_opp_add() > for the devices. Thank you for pointing it out. Do you mean that even if dev_pm_opp_of_cpumask_add_table returns an error, dev_pm_opp_get_opp_count may still return count > 0 because someone may call dev_pm_opp_add to add OPP to cpu succcessfully at somewhere else? Thanks, Quanyang > >> + * We need check the return value here, if it is non-zero, there is >> + * need to go on. >> */ >> - if (!dev_pm_opp_of_cpumask_add_table(priv->cpus)) >> - priv->have_static_opps = true; >> + ret = dev_pm_opp_of_cpumask_add_table(priv->cpus); >> + if (ret) { >> + dev_err(cpu_dev, "Failed to add OPP table for CPUs\n"); >> + goto out; >> + } >> + >> + priv->have_static_opps = true; >> >> /* >> * The OPP table must be initialized, statically or dynamically, by this
On 25-03-21, 13:15, quanyang.wang wrote: > Thank you for pointing it out. Do you mean that even if > dev_pm_opp_of_cpumask_add_table returns > > an error, dev_pm_opp_get_opp_count may still return count > 0 because > someone may call dev_pm_opp_add > > to add OPP to cpu succcessfully at somewhere else? Yes. There are two ways we can add OPPs today: - Statically via device tree. This is what dev_pm_opp_of_cpumask_add_table() tries to do. - Dynamically via call to dev_pm_opp_add(), which I described earlier. What failed here is the static way of adding OPPs, we still need to check if OPPs were added dynamically.
Hi Viresh, On 3/25/21 1:24 PM, Viresh Kumar wrote: > On 25-03-21, 13:15, quanyang.wang wrote: >> Thank you for pointing it out. Do you mean that even if >> dev_pm_opp_of_cpumask_add_table returns >> >> an error, dev_pm_opp_get_opp_count may still return count > 0 because >> someone may call dev_pm_opp_add >> >> to add OPP to cpu succcessfully at somewhere else? > Yes. > > There are two ways we can add OPPs today: > > - Statically via device tree. This is what > dev_pm_opp_of_cpumask_add_table() tries to do. > > - Dynamically via call to dev_pm_opp_add(), which I described earlier. > > What failed here is the static way of adding OPPs, we still need to > check if OPPs were added dynamically. Thank you for shedding light on this. I will send a V2 patch which only check the return error -EPROBE_DEFER. Thanks, Quanyang >
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index b1e1bdc63b01..f24359f47b1a 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -255,10 +255,16 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu) * before updating priv->cpus. Otherwise, we will end up creating * duplicate OPPs for the CPUs. * - * OPPs might be populated at runtime, don't check for error here. + * We need check the return value here, if it is non-zero, there is + * need to go on. */ - if (!dev_pm_opp_of_cpumask_add_table(priv->cpus)) - priv->have_static_opps = true; + ret = dev_pm_opp_of_cpumask_add_table(priv->cpus); + if (ret) { + dev_err(cpu_dev, "Failed to add OPP table for CPUs\n"); + goto out; + } + + priv->have_static_opps = true; /* * The OPP table must be initialized, statically or dynamically, by this