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

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

[Wei Yongjun]

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(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index 1cfbace..7bc2eeb 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -914,7 +914,7 @@ static int tegra_dsi_setup_clocks(struct tegra_dsi *dsi)
 	int err;
 
 	parent = clk_get_parent(dsi->clk);
-	if (!parent)
+	if (IS_ERR(parent))
 		return -EINVAL;
 
 	err = clk_set_parent(parent, dsi->clk_parent);
@@ -969,8 +969,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	dsi->regs = devm_ioremap_resource(&pdev->dev, regs);
-	if (!dsi->regs)
-		return -EADDRNOTAVAIL;
+	if (IS_ERR(dsi->regs))
+		return PTR_ERR(dsi->regs);
 
 	INIT_LIST_HEAD(&dsi->client.list);
 	dsi->client.ops = &dsi_client_ops;

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

[Thierry Reding]

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

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.

Thierry

> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 1cfbace..7bc2eeb 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -914,7 +914,7 @@ static int tegra_dsi_setup_clocks(struct tegra_dsi *dsi)
>  	int err;
>  
>  	parent = clk_get_parent(dsi->clk);
> -	if (!parent)
> +	if (IS_ERR(parent))
>  		return -EINVAL;
>  
>  	err = clk_set_parent(parent, dsi->clk_parent);
> @@ -969,8 +969,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  
>  	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	dsi->regs = devm_ioremap_resource(&pdev->dev, regs);
> -	if (!dsi->regs)
> -		return -EADDRNOTAVAIL;
> +	if (IS_ERR(dsi->regs))
> +		return PTR_ERR(dsi->regs);
>  
>  	INIT_LIST_HEAD(&dsi->client.list);
>  	dsi->client.ops = &dsi_client_ops;
> 

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

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

[Stephen Warren]

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:

/**
 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.  The implementation
 * uses @dev and @id to determine the clock consumer, and thereby
 * the clock producer.  (IOW, @id may be identical strings, but
 * clk_get may return different clock producers depending on @dev.)

If the implementation doesn't match that, then it's a bug, and a whole
slew of drivers will need changing... On the surface, it looks like
the hunk you dropped was correct.

NULL may be a perfectly legal return value from a function that
returns either valid data or an IS_ERR() value.

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

[Thierry Reding]

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

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.

Thierry

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

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

[Stephen Warren]

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.

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

[Thierry Reding]

[-- 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 --]