[patch] PM / devfreq: exynos-nocp: Checking for IS_ERR instead of NULL

[patch] PM / devfreq: exynos-nocp: Checking for IS_ERR instead of NULL

[Dan Carpenter]

The platform_get_resource() function returns NULL on error, it never
returns error pointers.

Fixes: 0179a913875a ('PM / devfreq: event: Add new Exynos NoC probe driver')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
index 6b6a5f3..03b35d5 100644
--- a/drivers/devfreq/event/exynos-nocp.c
+++ b/drivers/devfreq/event/exynos-nocp.c
@@ -220,8 +220,8 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
 
 	/* Maps the memory mapped IO to control nocp register */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (IS_ERR(res))
-		return PTR_ERR(res);
+	if (!res)
+		return -ENXIO;
 
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))

Re: [patch] PM / devfreq: exynos-nocp: Checking for IS_ERR instead of NULL

[Chanwoo Choi]

Hi Dan,

On 2016년 05월 18일 19:37, Dan Carpenter wrote:
> The platform_get_resource() function returns NULL on error, it never
> returns error pointers.
> 
> Fixes: 0179a913875a ('PM / devfreq: event: Add new Exynos NoC probe driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
> index 6b6a5f3..03b35d5 100644
> --- a/drivers/devfreq/event/exynos-nocp.c
> +++ b/drivers/devfreq/event/exynos-nocp.c
> @@ -220,8 +220,8 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
>  
>  	/* Maps the memory mapped IO to control nocp register */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (IS_ERR(res))
> -		return PTR_ERR(res);
> +	if (!res)
> +		return -ENXIO;
>  
>  	base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(base))

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks,
Chanwoo Choi


--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] PM / devfreq: exynos-nocp: Checking for IS_ERR instead of NULL

[Chanwoo Choi]

On 2016년 05월 18일 19:37, Dan Carpenter wrote:
> The platform_get_resource() function returns NULL on error, it never
> returns error pointers.
> 
> Fixes: 0179a913875a ('PM / devfreq: event: Add new Exynos NoC probe driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
> index 6b6a5f3..03b35d5 100644
> --- a/drivers/devfreq/event/exynos-nocp.c
> +++ b/drivers/devfreq/event/exynos-nocp.c
> @@ -220,8 +220,8 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
>  
>  	/* Maps the memory mapped IO to control nocp register */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (IS_ERR(res))
> -		return PTR_ERR(res);
> +	if (!res)
> +		return -ENXIO;
>  
>  	base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(base))

Looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks,
Chanwoo Choi

Re: [patch] PM / devfreq: exynos-nocp: Checking for IS_ERR instead of NULL

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1359 bytes --]



On Wed, 25 May 2016, Chanwoo Choi wrote:

> On 2016년 05월 18일 19:37, Dan Carpenter wrote:
> > The platform_get_resource() function returns NULL on error, it never
> > returns error pointers.
> >
> > Fixes: 0179a913875a ('PM / devfreq: event: Add new Exynos NoC probe driver')
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
> > index 6b6a5f3..03b35d5 100644
> > --- a/drivers/devfreq/event/exynos-nocp.c
> > +++ b/drivers/devfreq/event/exynos-nocp.c
> > @@ -220,8 +220,8 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
> >
> >  	/* Maps the memory mapped IO to control nocp register */
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	if (IS_ERR(res))
> > -		return PTR_ERR(res);
> > +	if (!res)
> > +		return -ENXIO;
> >
> >  	base = devm_ioremap_resource(dev, res);
> >  	if (IS_ERR(base))
>
> Looks good to me.

I believe that there is no need for the test at all.
devm_ioremap_resource should make the test.

julia

>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> Thanks,
> Chanwoo Choi
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [patch] PM / devfreq: exynos-nocp: Checking for IS_ERR instead of NULL

[Dan Carpenter]

On Wed, May 25, 2016 at 05:11:33PM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 25 May 2016, Chanwoo Choi wrote:
> 
> > On 2016년 05월 18일 19:37, Dan Carpenter wrote:
> > > The platform_get_resource() function returns NULL on error, it never
> > > returns error pointers.
> > >
> > > Fixes: 0179a913875a ('PM / devfreq: event: Add new Exynos NoC probe driver')
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
> > > index 6b6a5f3..03b35d5 100644
> > > --- a/drivers/devfreq/event/exynos-nocp.c
> > > +++ b/drivers/devfreq/event/exynos-nocp.c
> > > @@ -220,8 +220,8 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
> > >
> > >  	/* Maps the memory mapped IO to control nocp register */
> > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > -	if (IS_ERR(res))
> > > -		return PTR_ERR(res);
> > > +	if (!res)
> > > +		return -ENXIO;
> > >
> > >  	base = devm_ioremap_resource(dev, res);
> > >  	if (IS_ERR(base))
> >
> > Looks good to me.
> 
> I believe that there is no need for the test at all.
> devm_ioremap_resource should make the test.

True.  The static checker warning here is just that "res" isn't an error
pointer, not that we dereference it.

I'll resend.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Dan Carpenter]

Smatch complains because platform_get_resource() returns NULL on error
and not an error pointer so the check is wrong.  Julia Lawall pointed
out that normally we don't check these, because devm_ioremap_resource()
has a check for NULL.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: just remove the check

diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
index 6b6a5f3..a584140 100644
--- a/drivers/devfreq/event/exynos-nocp.c
+++ b/drivers/devfreq/event/exynos-nocp.c
@@ -220,9 +220,6 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
 
 	/* Maps the memory mapped IO to control nocp register */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (IS_ERR(res))
-		return PTR_ERR(res);
-
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Chanwoo Choi]

Hi Dan,

On 2016년 05월 26일 15:45, Dan Carpenter wrote:
> Smatch complains because platform_get_resource() returns NULL on error
> and not an error pointer so the check is wrong.  Julia Lawall pointed
> out that normally we don't check these, because devm_ioremap_resource()
> has a check for NULL.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: just remove the check
> 
> diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
> index 6b6a5f3..a584140 100644
> --- a/drivers/devfreq/event/exynos-nocp.c
> +++ b/drivers/devfreq/event/exynos-nocp.c
> @@ -220,9 +220,6 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
>  
>  	/* Maps the memory mapped IO to control nocp register */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (IS_ERR(res))
> -		return PTR_ERR(res);
> -
>  	base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);


I don't recommend that you mention the name of engineer on patch description
directly. Except for this comment, looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks,
Chanwoo Choi

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Dan Carpenter]

On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
> I don't recommend that you mention the name of engineer on patch description
> directly. Except for this comment, looks good to me.

I always feel it's nicer to give credit.  Plus Julia is already a well
known kernel dev and this was a pubic list.  (I wouldn't give credit if
she had sent hurtful comments about my spelling to me privately, for
example).

regards,
dan carpenter

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Chanwoo Choi]

On 2016년 05월 26일 19:02, Dan Carpenter wrote:
> On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
>> I don't recommend that you mention the name of engineer on patch description
>> directly. Except for this comment, looks good to me.
> 
> I always feel it's nicer to give credit.  Plus Julia is already a well
> known kernel dev and this was a pubic list.  (I wouldn't give credit if
> she had sent hurtful comments about my spelling to me privately, for
> example).

If you want to give credit as you mentioned, you can add the acked-by
or signed-off for Julia. But, usually, the patch description don't
include the engineer's name directly.

Thanks,
Chanwoo Choi

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Dan Carpenter]

Signed-off-by is like signing a legal document to say that you haven't
stolen any of the code from SCO Unix.  Please don't put sign for other
people.

regards,
dan carpenter

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Chanwoo Choi]

On 2016년 05월 26일 19:51, Dan Carpenter wrote:
> Signed-off-by is like signing a legal document to say that you haven't
> stolen any of the code from SCO Unix.  Please don't put sign for other
> people.

You're right. I'm forcing to put the Sign-off. It is just example.

But, I still think that patch description including the engineer's name is not appropriate
to give the credit someone.

Thanks,
Chanwoo Choi

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Chanwoo Choi]

On 2016년 05월 26일 19:57, Chanwoo Choi wrote:
> On 2016년 05월 26일 19:51, Dan Carpenter wrote:
>> Signed-off-by is like signing a legal document to say that you haven't
>> stolen any of the code from SCO Unix.  Please don't put sign for other
>> people.
> 
> You're right. I'm forcing to put the Sign-off. It is just example.
> 
> But, I still think that patch description including the engineer's name is not appropriate
> to give the credit someone.
> 

Also, If Julia review this patch, you just can add the reviewed-by tag.
I think that it is enough.

Thanks,
Chanwoo Choi

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Dan Carpenter]

On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
> I don't recommend that you mention the name of engineer on patch description
> directly.

This really is normal.  I've been mentioned over 100 times in commit
messages like 7051924f771 (xillybus: Move out of staging).

regards,
dan carpenter

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Chanwoo Choi]

On Thu, May 26, 2016 at 8:56 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
>> I don't recommend that you mention the name of engineer on patch description
>> directly.
>
> This really is normal.  I've been mentioned over 100 times in commit
> messages like 7051924f771 (xillybus: Move out of staging).

I'm still reluctant to use the name on description.
How about you use the Suggested-by tag as following?

[julia.lawall : Suggest that it is not necessary to check return value
of platform_get_resource]
Suggested-by: Julia Lawall <julia.lawall@lip6.fr>

Thanks,
Chanwoo Choi

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Julia Lawall]

On Thu, 26 May 2016, Chanwoo Choi wrote:

> On Thu, May 26, 2016 at 8:56 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
> >> I don't recommend that you mention the name of engineer on patch description
> >> directly.
> >
> > This really is normal.  I've been mentioned over 100 times in commit
> > messages like 7051924f771 (xillybus: Move out of staging).
>
> I'm still reluctant to use the name on description.
> How about you use the Suggested-by tag as following?
>
> [julia.lawall : Suggest that it is not necessary to check return value
> of platform_get_resource]
> Suggested-by: Julia Lawall <julia.lawall@lip6.fr>

Like Dan, I really don't see the problem.  The text in [ ] looks ugly.  A
suggested by by itself would not be appropriate, since I didn't identify
the original issue.  There are ther patches that refer to peoples'
comments in a similar way.  Example:
d8aacd87180141ff6b812b53de77a4336e87c91a

julia

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Dan Carpenter]

On Thu, May 26, 2016 at 10:46:26PM +0900, Chanwoo Choi wrote:
> On Thu, May 26, 2016 at 8:56 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
> >> I don't recommend that you mention the name of engineer on patch description
> >> directly.
> >
> > This really is normal.  I've been mentioned over 100 times in commit
> > messages like 7051924f771 (xillybus: Move out of staging).
> 
> I'm still reluctant to use the name on description.

I understand that but I don't understand why, though.  Anyway, aren't
forwarding this to someone?  You can change it to say whatever you want.

regards,
dan carpenter

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Chanwoo Choi]

On Thu, May 26, 2016 at 10:56 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Thu, May 26, 2016 at 10:46:26PM +0900, Chanwoo Choi wrote:
>> On Thu, May 26, 2016 at 8:56 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
>> >> I don't recommend that you mention the name of engineer on patch description
>> >> directly.
>> >
>> > This really is normal.  I've been mentioned over 100 times in commit
>> > messages like 7051924f771 (xillybus: Move out of staging).
>>
>> I'm still reluctant to use the name on description.
>
> I understand that but I don't understand why, though.  Anyway, aren't
> forwarding this to someone?  You can change it to say whatever you want.

Because if the name without any unique email address is included in
the description,
I think that it is not appropriate. Always, the information should
include the email address.
So, I just prefer to use the Suggested-by or the different kind of tag
with [id: explanation].

But, I'll stop about it because it is not critical issue.
It depend on what is the preferred method to add the information.

Thanks,
Chanwoo Choi

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Julia Lawall]

On Thu, 26 May 2016, Chanwoo Choi wrote:

> On Thu, May 26, 2016 at 10:56 PM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
> > On Thu, May 26, 2016 at 10:46:26PM +0900, Chanwoo Choi wrote:
> >> On Thu, May 26, 2016 at 8:56 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > On Thu, May 26, 2016 at 05:12:19PM +0900, Chanwoo Choi wrote:
> >> >> I don't recommend that you mention the name of engineer on patch description
> >> >> directly.
> >> >
> >> > This really is normal.  I've been mentioned over 100 times in commit
> >> > messages like 7051924f771 (xillybus: Move out of staging).
> >>
> >> I'm still reluctant to use the name on description.
> >
> > I understand that but I don't understand why, though.  Anyway, aren't
> > forwarding this to someone?  You can change it to say whatever you want.
>
> Because if the name without any unique email address is included in
> the description,

I sent a Reviewed-by, so this issue is solved.

julia

> I think that it is not appropriate. Always, the information should
> include the email address.
> So, I just prefer to use the Suggested-by or the different kind of tag
> with [id: explanation].
>
> But, I'll stop about it because it is not critical issue.
> It depend on what is the preferred method to add the information.
>
> Thanks,
> Chanwoo Choi
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Dan Carpenter]

The only duplicates that I know about currently are Dan Williams and Xi
Wang.  I do sometimes get those people mixed up.  I hope someday there
will be another Julia Lawall duplicate as well.  :)

regards,
dan carpenter

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Julia Lawall]

Reviewed-by: Julia Lawall <julia.lawall@lip6.fr>

On Thu, 26 May 2016, Dan Carpenter wrote:

> Smatch complains because platform_get_resource() returns NULL on error
> and not an error pointer so the check is wrong.  Julia Lawall pointed
> out that normally we don't check these, because devm_ioremap_resource()
> has a check for NULL.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: just remove the check
>
> diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
> index 6b6a5f3..a584140 100644
> --- a/drivers/devfreq/event/exynos-nocp.c
> +++ b/drivers/devfreq/event/exynos-nocp.c
> @@ -220,9 +220,6 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
>
>  	/* Maps the memory mapped IO to control nocp register */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (IS_ERR(res))
> -		return PTR_ERR(res);
> -
>  	base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[MyungJoo Ham]

On Thu, May 26, 2016 at 3:45 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Smatch complains because platform_get_resource() returns NULL on error
> and not an error pointer so the check is wrong.  Julia Lawall pointed
> out that normally we don't check these, because devm_ioremap_resource()
> has a check for NULL.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>

Queued in fixes branch to be sent with other fixes for RCx.

> ---
> v2: just remove the check
>
> diff --git a/drivers/devfreq/event/exynos-nocp.c b/drivers/devfreq/event/exynos-nocp.c
> index 6b6a5f3..a584140 100644
> --- a/drivers/devfreq/event/exynos-nocp.c
> +++ b/drivers/devfreq/event/exynos-nocp.c
> @@ -220,9 +220,6 @@ static int exynos_nocp_parse_dt(struct platform_device *pdev,
>
>         /* Maps the memory mapped IO to control nocp register */
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -       if (IS_ERR(res))
> -               return PTR_ERR(res);
> -
>         base = devm_ioremap_resource(dev, res);
>         if (IS_ERR(base))
>                 return PTR_ERR(base);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
MyungJoo Ham, Ph.D.
Frontier CS Lab, S/W Center, Samsung Electronics

Re: [patch v2] PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

[Chanwoo Choi]

Hi Myungjoo,

On Wed, Jun 1, 2016 at 7:25 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Thu, May 26, 2016 at 3:45 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> Smatch complains because platform_get_resource() returns NULL on error
>> and not an error pointer so the check is wrong.  Julia Lawall pointed
>> out that normally we don't check these, because devm_ioremap_resource()
>> has a check for NULL.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>
> Queued in fixes branch to be sent with other fixes for RCx.


I already reviewed this patch. Also, the merged patch[1] don't include
the my reviewed-by tag.
I'd like you to add my reviewed-by tag again.

[1] https://git.kernel.org/cgit/linux/kernel/git/mzx/devfreq.git/commit/?h=fixes&id=44a3c02256bb69b2a20f55d2bd77c5a2fd20bc52

Regards,
Chanwoo Choi