Re: [PATCH] cpufreq: dt: check the error returned by dev_pm_opp_of_cpumask_add_table

[quanyang.wang]

Hi Viresh,

On 3/25/21 12:45 PM, Viresh Kumar wrote:
> On 25-03-21, 12:31, quanyang.wang@xxxxxxxxxxxxx wrote:
> > From: Quanyang Wang <quanyang.wang@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > ---
> >   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