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

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

[quanyang.wang]

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.
+	 * 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
-- 
2.25.1

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

[Viresh Kumar]

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

-- 
viresh

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@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

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

[Viresh Kumar]

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.

-- 
viresh

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

[quanyang.wang]

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

>