Re: [PATCH -next] drm/tegra: fix return value check

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

On Mon, Oct 28, 2013 at 04:48:40PM -0600, Stephen Warren wrote:
> On 10/28/2013 04:38 PM, Thierry Reding wrote:
> > On Mon, Oct 28, 2013 at 03:51:32PM -0600, Stephen Warren wrote:
> >> On 10/28/2013 02:53 AM, Thierry Reding wrote:
> >>> On Mon, Oct 21, 2013 at 11:34:07AM +0800, Wei Yongjun wrote:
> >>>> From: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
> >>>> 
> >>>> In case of error, the function clk_get_parent() and 
> >>>> devm_ioremap_resource() returns ERR_PTR() and never returns
> >>>> NULL. The NULL test in the return value check should be
> >>>> replaced with IS_ERR().
> >>>> 
> >>>> Signed-off-by: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
> >>>> --- drivers/gpu/drm/tegra/dsi.c | 6 +++--- 1 file changed, 3 
> >>>> insertions(+), 3 deletions(-)
> >>> 
> >>> I've applied this, but with the first hunk removed, since
> >>> looking at the implementation of clk_get_parent() it can
> >>> actually return NULL. In fact it seems like it will never
> >>> return ERR_PTR().
> >>> 
> >>> I've also updated the commit message to reflect that.
> >> 
> >> Hmm. The documentation for clk_get() says:
> > 
> > The patch didn't check the return value clk_get() but
> > clk_get_parent(). Here's the implementation:
> > 
> > struct clk *__clk_get_parent(struct clk *clk) { return !clk ? NULL
> > : clk->parent; }
> > 
> > Note that clk_get_parent() in simply a locked version of the above.
> > That will obviously only return ERR_PTR() if clk->parent happens to
> > be set to one such value, which I don't think will ever happen.
> 
> Ah. That looks like a bug in __clk_get_parent() then, since !clk
> doesn't sound like  the correct error case for it to be checking.
> Shouldn't it return IS_ERR(clk) ? clk : clk->parent? Either that, or
> clk_get() shouldn't return an error value if the rest of the clock
> code doesn NULL checks.

Yes, that would seem to be more consistent. Then again, one could argue
that if somebody got an invalid (ERR_PTR()) clock from clk_get(), they
shouldn't be calling clk_get_parent() on it in the first place. But the
same would be true for NULL, so...

Looping in Mike.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]