Re: Seeking advice on API return type inconsistency

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

Hello,

On Fri, Sep 19, 2025 at 09:51:15AM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Wed, 13 Aug 2025 at 05:41, Michal Suchánek <msuchanek@suse.de> 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.
> 
> I don't think that is a good idea. Debugging drivers then becomes
> quite complicated since there is no feedback and you must head into
> the clock subsystem to see what is going on. Bring this into U-Boot
> more generally and it will be very painful. At the moment at least you
> can enable CONFIG_LOG_ERROR_RETURN and see what happened.

Does that really work with unsigned return values containing negative
error values as large positive integers?

> Better I think to create some new functions with the API you want
> (e.g. panic on failure, ignore failure). Or rename clk_get_rate_int()
> - for internal - and create a new clk_get_rate() that does what you
> want / what Linux does.

The problem is that once an error is returned as 'clock rate' there is
really no way to determine if what you got is really a clock rate or an
error. You can assume that the values IS_ERR decodes as errors are not
clock rates but there is no guarantee that is the case. While IS_ERR
only uses small part of the avaiale range the valid values are not
spread evenly over the range, and hardware-specific.

Thanks

Michal