linux-kernel - Re: [PATCH] cpufreq: dt: check the error returned by dev_pm_opp_of_cpumask_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Mar 2021 13:15:09 +0800
From:   "quanyang.wang" <quanyang.wang@...driver.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: dt: check the error returned by
 dev_pm_opp_of_cpumask_add_table

Hi Viresh,

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