Re: [PATCH 1/3] clk: keystone: Fix an error checking

From: Christophe JAILLET
Date: Fri Nov 04 2016 - 01:44:24 EST

Le 02/11/2016 à 01:22, Stephen Boyd a écrit :
On 10/24, Christophe JAILLET wrote:
clk_register_pll() can return ERR_PTR(-ENOMEM) so checking the return value
against NULL only is not correct.
The code just doesn't propagate the error up to the caller.
Instead the caller treats NULL as an error and non-NULL as valid.
If the callee detects an error it hides it and returns NULL.

Could you please clarify?
I thought that your point was that 'clk_register_pll()' was returning NULL when 'clk_register()' was returning an error.
The proposed patch fixes it and now 'clk_register_pll()' returns:
- either ERR_PTR(-ENOMEM) if zkalloc fails
- or clk if 'clk_register()' fails. In this case it is an error pointer
- or a valid, non NULL, pointer in case of success

The caller, '_of_pll_clk_init()' has also been updated to test the return value using IS_ERR.

So, which caller/callee do you refer to?
Would you like '_of_pll_clk_init()' to also propagate the error?

Or is it the phrasing of the log entry which is not clear enough and should be updated?

In order to fix it, update clk_register_pll() to always return an error
pointer in case of error and check the return value with IS_ERR.

While at it, also fix a tab vs space in the surrounding code.

Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
Un-compiled and un-tested.
Please at least compile test patches.
Agreed, and I usually do.
But in this particular case, I don't have the build environment to do it.

I preferred to report what looks like a (small) bug to me and clearly state that I didn't even compile-tested it rather than just ignoring it.
Hoping that this approach, in such a case, is acceptable.