Re: [PATCH 2/6] clk: Fix error handling in clk_get_rate()

["Michal Suchánek" <msuchanek@suse.de>]

Hello,

On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
> log_ret() cannot work with unsigned values, and the assignment to 'ret'
> incorrectly truncates the rate from long to int.
> 
> Fixes: 5c5992cb90cf ("clk: Add debugging for return values")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/clk/clk-uclass.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index dc3e9d6a261..78299dbceb2 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -471,7 +471,6 @@ void clk_free(struct clk *clk)
>  ulong clk_get_rate(struct clk *clk)
>  {
>  	const struct clk_ops *ops;
> -	int ret;
>  
>  	debug("%s(clk=%p)\n", __func__, clk);
>  	if (!clk_valid(clk))
> @@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk)
>  	if (!ops->get_rate)
>  		return -ENOSYS;
>  
> -	ret = ops->get_rate(clk);
> -	if (ret)
> -		return log_ret(ret);
> -
> -	return 0;
> +	return ops->get_rate(clk);
>  }

This is generally poor design of the clock stuff in u-boot.

It returns -ERROR for many error conditions, but also 0 for other error
conditions.

Some error handling checks for 0, some for errval, some casts to int and
checks for <= 0.

I think that using -ERROR for clocks does not make much sense in u-boot.

Even in the kernel the errval checks are pretty much limited to places
where integers are used to store page frame numbers or pointers, that is
errptr stored in an integer. For adresses it is pretty easy to make sure
that the last page is not mapped making the error pointers invalid
(although bugs in that part happened too).

For clocks no such guarantee exists. The only apparently invalid clock
is 0, and the correct fix is to fix up the clock code to return 0 on
error, always. It's a lot of code to fix, though.

If you do not want to fix everything then the correct thing to do is
make ret ulong, and check for errval *and* 0.

There is not much point in returning detailed error codes in u-boot,
anyway. It's not like there is some userspace that could interpret them.
Most errors are logged when they happen if ever, and callers only check
if error happened or not.

Thanks

Michal