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

[Sean Anderson <seanga2@gmail.com>]

On 2/20/23 05:37, Michal Suchánek wrote:
> 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

clk_get_parent is the only place where we return a clock pointer directly.
Everywhere else, we have an integer return we can use. This function is
different of course because CCF is broken and assumes there is only one
canonical struct clk for a logical clock. So it can't initialize a caller-passed
struct clk, but instead has to return the one true struct clk.

I agree with Michal here. The signature should really be

int clk_get_parent(struct clk *child, struct clk *parent)

but for now there is no point confusing the rest of the clock subsystem to make
it work with error pointers.

--Sean