Seeking advice on API return type inconsistency

[Andrew Goodbody]

On 13/08/2025 16:40, Tom Rini wrote:
> On Wed, Aug 13, 2025 at 01:41:21PM +0200, Michal Suchánek wrote:
>> Hello,
>>
>> On Wed, Aug 13, 2025 at 11:58:19AM +0100, Andrew Goodbody wrote:
>>> On 12/08/2025 16:03, Tom Rini wrote:
>>>> On Tue, Aug 12, 2025 at 03:46:56PM +0100, Andrew Goodbody wrote:
>>>>> On 12/08/2025 15:33, Tom Rini wrote:
>>>>>> On Tue, Aug 12, 2025 at 10:17:47AM +0100, Andrew Goodbody wrote:
>>>>>>> On 11/08/2025 17:36, Quentin Schulz wrote:
>>>>>>>> Hi Andrew,
>>>>>>>>
>>>>>>>> On 8/11/25 5:24 PM, Andrew Goodbody wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I was wondering what people's thoughts were on API return types. In
>>>>>>>>> particular there is this and other examples in include/clk-uclass.h
>>>>>>>>>
>>>>>>>>> /**
>>>>>>>>>      * get_rate() - Get current clock rate.
>>>>>>>>>      * @clk:    The clock to query.
>>>>>>>>>      *
>>>>>>>>>      * This returns the current rate of a clock. If the clock is
>>>>>>>>> disabled, it
>>>>>>>>>      * returns the rate at which the clock would run if it was enabled. The
>>>>>>>>>      * following pseudo-code should hold::
>>>>>>>>>      *
>>>>>>>>>      *   disable(clk)
>>>>>>>>>      *   rate = get_rate(clk)
>>>>>>>>>      *   enable(clk)
>>>>>>>>>      *   assert(get_rate(clk) == rate)
>>>>>>>>>      *
>>>>>>>>>      * Return:
>>>>>>>>>      * * The rate of @clk
>>>>>>>>>      * * -%ENOSYS if this function is not implemented for @clk
>>>>>>>>>      * * -%ENOENT if @clk->id is invalid. Prefer using an assert
>>>>>>>>> instead, and doing
>>>>>>>>>      *   this check in request().
>>>>>>>>>      * * Another negative error value (such as %EIO or %ECOMM) if the
>>>>>>>>> rate could
>>>>>>>>>      *   not be determined due to a bus error.
>>>>>>>>>      */
>>>>>>>>> ulong get_rate(struct clk *clk);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> get_rate is declared as returning a ulong but the description says
>>>>>>>>> that it can return negative errors. A simple test of the return
>>>>>>>>> value for being less than 0 will always fail so errors can go
>>>>>>>>> undetected. Casting to a signed type seems less than ideal.
>>>>>>>>>
>>>>>>>>> What is the best way to deal with this? Cast to a signed or update
>>>>>>>>> the API to be signed or...?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Note that clk_get_rate() in the kernel has the same function signature
>>>>>>>> so I would refrain from changing the type otherwise we'll have some
>>>>>>>> "funny" bugs to handle considering it isn't that uncommon to import
>>>>>>>> drivers almost as-is from the Linux kernel.
>>>>>>>
>>>>>>> Ah yes. The difference being that the kernel does not seem to attempt to
>>>>>>> push an error code through this API, you get a rate or you get 0.
>>>>>>
>>>>>> How is the error code pushed? Or is it up to the caller to decide that 0
>>>>>> means on a case by case basis?
>>
>> Presumably it returns -EBUSY which is then implicitly converted to a large
>> positive value. For pointers there is IS_ERR macro to decode that, and
>> for integers there is another corresponding macro (I don't recall the
>> name).
>>
>> For memory this assumes that the last page that holds the 'error'
>> pointers cannot be mapped, and there was already a bug in the kernel
>> that this was not true.
>>
>> For clock rates this assumes that rates close to the maximum value are
>> invalid which is not guaranteed in any way whatsoever.
>>
>> Consequently, this should not be used.
>>
>> 0 is the only clock rate guaranteed to be invalid.
>>
>> As far as I have seen in the u-boot code almost no caller checks the
>> result, and those that do cannot do anything meaningful with it anyway,
>> at most a message is printed. That can be done by the driver without
>> passing around the error.
>>
>>>>> In the Linux kernel almost no code checks the return of clk_get_rate at all.
>>>>> Some code will check the value is sensible and 0 is obviously not sensible.
>>>>> But pretty much the call to clk_get_rate is not expected to fail.
>>>>
>>>> Perhaps getting lost in the specifics then, but perhaps for this case we
>>>> should just do the same? But your question was more general, so another
>>>> example might help.
>>>
>>> I suspect that the answer is always going to that it depends on the
>>> specifics of each case. The U-Boot implementation of clk_get_rate seems to
>>> have become more complicated leading to the addition of returning error
>>> codes. This leads to the question about what level of compatibility should
>>> there be maintained with the kernel? That addition of returning error codes
>>> in U-Boot already means that the API is no longer compatible with that of
>>> the kernel.
>>
>> I would suggest to abolish returning 'errors' as clock rates.
> 
> Yes, I think this is a case where we should change our internal API,
> both for easier driver migrations from the linux kernel and also because
> it seems our design here can't quite work as intended.

OK, I will see if I can put a patch series together.

Andrew