Re: Seeking advice on API return type inconsistency

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

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.

Thanks

Michal