| Message ID | ff1de6a7af60b4e3804162e553d7174e2ad1ea16.1477339880.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Stephen Boyd |
| Headers | show |
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. > 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@wanadoo.fr> > --- > Un-compiled and un-tested. Please at least compile test patches.
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@wanadoo.fr> >> --- >> 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. CJ -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/keystone/pll.c b/drivers/clk/keystone/pll.c index a26ba2184454..70d6fb2d23bc 100644 --- a/drivers/clk/keystone/pll.c +++ b/drivers/clk/keystone/pll.c @@ -140,7 +140,7 @@ static struct clk *clk_register_pll(struct device *dev, init.parent_names = (parent_name ? &parent_name : NULL); init.num_parents = (parent_name ? 1 : 0); - pll->pll_data = pll_data; + pll->pll_data = pll_data; pll->hw.init = &init; clk = clk_register(NULL, &pll->hw); @@ -150,7 +150,7 @@ static struct clk *clk_register_pll(struct device *dev, return clk; out: kfree(pll); - return NULL; + return clk; } /** @@ -213,7 +213,7 @@ static void __init _of_pll_clk_init(struct device_node *node, bool pllctrl) } clk = clk_register_pll(NULL, node->name, parent_name, pll_data); - if (clk) { + if (!IS_ERR(clk)) { of_clk_add_provider(node, of_clk_src_simple_get, clk); return; }
clk_register_pll() can return ERR_PTR(-ENOMEM) so checking the return value against NULL only is not correct. 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@wanadoo.fr> --- Un-compiled and un-tested. --- drivers/clk/keystone/pll.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)