Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 25 Mar 2021 10:15:41 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     quanyang.wang@windriver.com
Cc:     "Rafael J . Wysocki" <rjw@rjwysocki.net>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cpufreq: dt: check the error returned by
 dev_pm_opp_of_cpumask_add_table
Message-ID: <20210325044541.rsncitrmkpaes4dm@vireshk-i7>
References: <20210325043129.2255918-1-quanyang.wang@windriver.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210325043129.2255918-1-quanyang.wang@windriver.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

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