Re: [PATCH] clk: spacemit: fix error handling in ccu_pll_set_rate/_round_rate

[Haylen Chu <heylenay@4d2.org>]

On Tue, Jul 22, 2025 at 12:29:23AM +0530, Akhilesh Patil wrote:
> Initialize best_entry pointer with NULL in ccu_pll_lookup_best_rate()
> to avoid returning garbage value when function fails to assign it
> a valid rate entry.

This doesn't sound very reasonable to me. Looking through
ccu_pll_lookup_best_rate(),

	static const struct ccu_pll_rate_tbl *
	ccu_pll_lookup_best_rate(struct ccu_pll *pll, unsigned long rate)
	{
	        struct ccu_pll_config *config = &pll->config;
	        const struct ccu_pll_rate_tbl *best_entry;
	        unsigned long best_delta = ULONG_MAX;
	        int i;

	        for (i = 0; i < config->tbl_num; i++) {
	                const struct ccu_pll_rate_tbl *entry = &config->rate_tbl[i];
	                unsigned long delta = abs_diff(entry->rate, rate);

	                if (delta < best_delta) {
	                        best_delta = delta;
	                        best_entry = entry;
	                }
	        }

	        return best_entry;
	}

best_entry is assigned as long as there's one entry fits the delta
better. Since best_delta is set to ULONG_MAX, any entry with non-zero
rates fits the required rate "better" at start of the loop. As long as
we have at least one non-zero entry defined for the PLL, best_entry is
always initialized and ccu_pll_lookup_best_rate() cannot return an
invalid pointer. And all existing PLLs do define at least one entry.

> Avoid passing invalid rate entry reference to
> ccu_pll_update_param by adding appropriate error handling in
> ccu_pll_set_rate and ccu_pll_round_rate.
> Address the effects of uninitialized pointer as reported
> by smatch and coverity static code analysis tools.
> 
> Addresses-Coverity-ID: 1649164
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/r/202505111057.ejK2J56K-lkp@intel.com/

Thus this looks like a false-positive to me.

> Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>

Regards,
Haylen Chu

> ---
>  drivers/clk/spacemit/ccu_pll.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/ccu_pll.c b/drivers/clk/spacemit/ccu_pll.c
> index 4427dcfbbb97..3fc6a30f98b7 100644
> --- a/drivers/clk/spacemit/ccu_pll.c
> +++ b/drivers/clk/spacemit/ccu_pll.c
> @@ -21,7 +21,7 @@ static const struct ccu_pll_rate_tbl *ccu_pll_lookup_best_rate(struct ccu_pll *p
>  							       unsigned long rate)
>  {
>  	struct ccu_pll_config *config = &pll->config;
> -	const struct ccu_pll_rate_tbl *best_entry;
> +	const struct ccu_pll_rate_tbl *best_entry = NULL;
>  	unsigned long best_delta = ULONG_MAX;
>  	int i;
>  
> @@ -107,6 +107,10 @@ static int ccu_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  	const struct ccu_pll_rate_tbl *entry;
>  
>  	entry = ccu_pll_lookup_best_rate(pll, rate);
> +
> +	if (!entry)
> +		return -EINVAL;
> +
>  	ccu_pll_update_param(pll, entry);
>  
>  	return 0;
> @@ -129,8 +133,11 @@ static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>  			       unsigned long *prate)
>  {
>  	struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +	const struct ccu_pll_rate_tbl *entry;
> +
> +	entry = ccu_pll_lookup_best_rate(pll, rate);
>  
> -	return ccu_pll_lookup_best_rate(pll, rate)->rate;
> +	return entry ? entry->rate : 0;
>  }
>  
>  static int ccu_pll_init(struct clk_hw *hw)
> -- 
> 2.34.1
>