Messages in this thread | | | Subject | Re: [PATCH] cpufreq: dt: check the error returned by dev_pm_opp_of_cpumask_add_table | From | "quanyang.wang" <> | Date | Thu, 25 Mar 2021 13:15:09 +0800 |
| |
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
| |