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

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

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. It returns a clock rate. A clock rate of 2.5GHz is
not out of question, and cannot be expressed with a signed integer.

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

Thanks

Michal