[PATCH 2/6] clk: Fix error handling in clk_get_rate()
seanga2 at gmail.com
Mon Feb 20 18:58:51 CET 2023
On 2/20/23 12:27, Michal Suchánek wrote:
> On Mon, Feb 20, 2023 at 11:08:41AM -0500, Sean Anderson wrote:
>> On 2/20/23 05:37, Michal Suchánek wrote:
>>> 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 at 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
>>> 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.
>> 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 at 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.
I would accept a conversion to "0 means unknown rate (for whatever reason)" API.
More information about the U-Boot