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

[Sean Anderson <seanga2@gmail.com>]

On 2/20/23 12:27, Michal Suchánek wrote:
> Hello,
> 
> On Mon, Feb 20, 2023 at 11:08:41AM -0500, Sean Anderson wrote:
>> 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.
> 
> This is clk_get_rate.

Ah, whoops. In that case,

Reviewed-by: Sean Anderson <seanga2@gmail.com>

> It returns a clock rate. A clock rate of 2.5GHz is
> not out of question, and cannot be expressed with a signed integer.

And of course neither is a rate of 5 GHz :)

> Hence clock rate should not be treated as signed, and -ERROR should not
> be returned as clock rate.

Yes...

I would accept a conversion to "0 means unknown rate (for whatever reason)" API.

--Sean