* [PATCH -next] drm/tegra: fix return value check
@ 2013-10-21 3:34 Wei Yongjun
[not found] ` <CAPgLHd_JqYS5DC4tmzgGVEYmJpeM96FAEDvmM3cUUep+1jj7mA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Wei Yongjun @ 2013-10-21 3:34 UTC (permalink / raw)
To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
tbergstrom-DDmLM1+adcrQT0dZR+AlfA, airlied-cv59FeDIM0c,
swarren-3lzwWm7+Weoh9ZMKESR00Q,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ
Cc: yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next] drm/tegra: fix return value check
[not found] ` <CAPgLHd_JqYS5DC4tmzgGVEYmJpeM96FAEDvmM3cUUep+1jj7mA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-10-28 8:53 ` Thierry Reding
[not found] ` <20131028085358.GC10718-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2013-10-28 8:53 UTC (permalink / raw)
To: Wei Yongjun
Cc: tbergstrom-DDmLM1+adcrQT0dZR+AlfA, airlied-cv59FeDIM0c,
swarren-3lzwWm7+Weoh9ZMKESR00Q,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] drm/tegra: fix return value check
[not found] ` <20131028085358.GC10718-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
@ 2013-10-28 21:51 ` Stephen Warren
[not found] ` <526EDC64.5050202-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2013-10-28 21:51 UTC (permalink / raw)
To: Thierry Reding, Wei Yongjun
Cc: tbergstrom-DDmLM1+adcrQT0dZR+AlfA, airlied-cv59FeDIM0c,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] drm/tegra: fix return value check
[not found] ` <526EDC64.5050202-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
@ 2013-10-28 22:38 ` Thierry Reding
2013-10-28 22:48 ` Stephen Warren
0 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2013-10-28 22:38 UTC (permalink / raw)
To: Stephen Warren
Cc: Wei Yongjun, tbergstrom-DDmLM1+adcrQT0dZR+AlfA,
airlied-cv59FeDIM0c, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] drm/tegra: fix return value check
2013-10-28 22:38 ` Thierry Reding
@ 2013-10-28 22:48 ` Stephen Warren
[not found] ` <526EE9C8.4080308-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2013-10-28 22:48 UTC (permalink / raw)
To: Thierry Reding
Cc: Wei Yongjun, tbergstrom-DDmLM1+adcrQT0dZR+AlfA,
airlied-cv59FeDIM0c, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] drm/tegra: fix return value check
[not found] ` <526EE9C8.4080308-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
@ 2013-10-28 23:04 ` Thierry Reding
0 siblings, 0 replies; 6+ messages in thread
From: Thierry Reding @ 2013-10-28 23:04 UTC (permalink / raw)
To: Stephen Warren, Mike Turquette
Cc: Wei Yongjun, tbergstrom-DDmLM1+adcrQT0dZR+AlfA,
airlied-cv59FeDIM0c, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-28 23:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-21 3:34 [PATCH -next] drm/tegra: fix return value check Wei Yongjun
[not found] ` <CAPgLHd_JqYS5DC4tmzgGVEYmJpeM96FAEDvmM3cUUep+1jj7mA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-10-28 8:53 ` Thierry Reding
[not found] ` <20131028085358.GC10718-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
2013-10-28 21:51 ` Stephen Warren
[not found] ` <526EDC64.5050202-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-10-28 22:38 ` Thierry Reding
2013-10-28 22:48 ` Stephen Warren
[not found] ` <526EE9C8.4080308-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2013-10-28 23:04 ` Thierry Reding
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.