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

PATCH In review Dan Carpenter 5 participants 22 messages May 18, 2016 – Jun 1, 2016

Subsystems (4)

Kernel janitors Arm Samsung Exynos Device frequency (devfreq) Device frequency event (devfreq-event)

lore

Share & tools

lore

Dan Carpenter

author

2016-05-18 10:37 UTC #0 lore raw

PATCH 1/1 PM / devfreq: exynos-nocp: Checking for IS_ERR instead of NULL

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

drivers/devfreq/event/exynos-nocp.c +2 -2

index 6b6a5f3..03b35d5 100644
@@ -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))

Chanwoo Choi

maintainer Reviewed-by on v1 1/1

2016-05-18 10:57 UTC #1 lore raw

Hi Dan,

Quoted 11 lines · 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 <[email protected]>
drivers/devfreq/event/exynos-nocp.c +0 -0
index 6b6a5f3..03b35d5 100644

drivers/devfreq/event/exynos-nocp.c static int exynos_nocp_parse_dt(struct platform_device *pdev, · line 227

Quoted 10 lines

drivers/devfreq/event/exynos-nocp.c +2 -2

@@ -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 <[email protected]>

Thanks,
Chanwoo Choi

Chanwoo Choi

maintainer Reviewed-by on v1 1/1

2016-05-25 12:34 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 11 lines · 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 <[email protected]>
drivers/devfreq/event/exynos-nocp.c +0 -0
index 6b6a5f3..03b35d5 100644

drivers/devfreq/event/exynos-nocp.c static int exynos_nocp_parse_dt(struct platform_device *pdev, · line 227

Quoted 10 lines

drivers/devfreq/event/exynos-nocp.c +2 -2

@@ -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 <[email protected]>

Thanks,
Chanwoo Choi

Julia Lawall

2016-05-25 15:11 UTC #3 lore raw

Quoted 20 lines · Chanwoo Choi

Quoted 19 lines · 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 <[email protected]>
drivers/devfreq/event/exynos-nocp.c +2 -2
index 6b6a5f3..03b35d5 100644
@@ -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;
 

Quoted 4 lines

drivers/devfreq/event/exynos-nocp.c static int exynos_nocp_parse_dt(struct platform_device *pdev, · line 227
Quoted 2 lines
drivers/devfreq/event/exynos-nocp.c +0 -0
 	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

Quoted 11 lines

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

Dan Carpenter

author

2016-05-26 06:39 UTC #4 lore raw

Quoted 24 lines · Julia Lawall

Quoted 20 lines · Chanwoo Choi
Quoted 19 lines · 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 <[email protected]>
drivers/devfreq/event/exynos-nocp.c +2 -2
index 6b6a5f3..03b35d5 100644
@@ -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;
 

Quoted 7 lines

Quoted 4 lines
drivers/devfreq/event/exynos-nocp.c static int exynos_nocp_parse_dt(struct platform_device *pdev, · line 227
Quoted 2 lines
drivers/devfreq/event/exynos-nocp.c +0 -0
 	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

Dan Carpenter

author

2016-05-26 06:45 UTC #5 lore raw

PATCH v2 1/1 PM / devfreq: exynos-nocp: Remove incorrect IS_ERR() check

In reply to Julia Lawall

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

---
v2: just remove the check

drivers/devfreq/event/exynos-nocp.c +0 -3

index 6b6a5f3..a584140 100644
@@ -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);

Chanwoo Choi

maintainer Reviewed-by on v2 1/1

2016-05-26 08:12 UTC #6 lore raw

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

Hi Dan,

Quoted 14 lines · 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 <[email protected]>
---
v2: just remove the check
drivers/devfreq/event/exynos-nocp.c +0 -0
index 6b6a5f3..a584140 100644

drivers/devfreq/event/exynos-nocp.c static int exynos_nocp_parse_dt(struct platform_device *pdev, · line 225

Quoted 9 lines

drivers/devfreq/event/exynos-nocp.c +0 -3

@@ -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 <[email protected]>

Thanks,
Chanwoo Choi

Dan Carpenter

author

2016-05-26 10:02 UTC #7 lore raw

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

Quoted 2 lines · Chanwoo Choi

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

Chanwoo Choi

maintainer

2016-05-26 10:16 UTC #8 lore raw

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

Quoted 8 lines · Dan Carpenter

Quoted 2 lines · Chanwoo Choi
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

Dan Carpenter

author

2016-05-26 10:51 UTC #9 lore raw

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

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

Chanwoo Choi

maintainer

2016-05-26 10:57 UTC #10 lore raw

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

Quoted 3 lines · 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.

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

Chanwoo Choi

maintainer

2016-05-26 11:01 UTC #11 lore raw

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

Quoted 10 lines · Chanwoo Choi

Quoted 3 lines · 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.
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

Dan Carpenter

author

2016-05-26 11:56 UTC #12 lore raw

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

In reply to Chanwoo Choi

Quoted 2 lines · Chanwoo Choi

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

Chanwoo Choi

2016-05-26 13:46 UTC #13 lore raw

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

Quoted 6 lines · Dan Carpenter

Quoted 2 lines · Chanwoo Choi
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]

Thanks,
Chanwoo Choi

Julia Lawall

2016-05-26 13:54 UTC #14 lore raw

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

Quoted 8 lines · Chanwoo Choi

Quoted 6 lines · Dan Carpenter
Quoted 2 lines · Chanwoo Choi
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).

Quoted 6 lines

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

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

Dan Carpenter

author

2016-05-26 13:56 UTC #15 lore raw

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

In reply to Chanwoo Choi

Quoted 9 lines · Chanwoo Choi

Quoted 6 lines · Dan Carpenter
Quoted 2 lines · Chanwoo Choi
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

Chanwoo Choi

2016-05-26 14:10 UTC #16 lore raw

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

Quoted 6 lines · Dan Carpenter

Quoted 5 lines · Chanwoo Choi
Quoted 4 lines · Dan Carpenter
Quoted 2 lines · Chanwoo Choi
I don't recommend that you mention the name of engineer on patch description directly.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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

Julia Lawall

2016-05-26 14:15 UTC #17 lore raw

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

Quoted 11 lines · Chanwoo Choi

Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Chanwoo Choi
Quoted 6 lines · Dan Carpenter
Quoted 2 lines · Chanwoo Choi
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).

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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

Quoted 15 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2016-05-26 16:06 UTC #18 lore raw

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

In reply to Chanwoo Choi

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

Julia Lawall

Reviewed-by on v2 1/1

2016-05-26 11:13 UTC #19 lore raw

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

In reply to Dan Carpenter

Reviewed-by Julia Lawall <[email protected]>

Quoted 28 lines · 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 <[email protected]>
---
v2: just remove the check
drivers/devfreq/event/exynos-nocp.c +0 -4
index 6b6a5f3..a584140 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

MyungJoo Ham

maintainer Acked-by on v2 1/1 Applied

2016-06-01 10:25 UTC #20 lore raw

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

In reply to Dan Carpenter

Quoted 6 lines · 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 <[email protected]>

Acked-by MyungJoo Ham <[email protected]>

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

Quoted 21 lines

---
v2: just remove the check

drivers/devfreq/event/exynos-nocp.c +0 -4

index 6b6a5f3..a584140 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

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

Chanwoo Choi

2016-06-01 11:33 UTC #21 lore raw

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

Hi Myungjoo,

Quoted 11 lines · MyungJoo Ham

Quoted 6 lines · 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 <[email protected]>
Acked-by MyungJoo Ham <[email protected]>
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&idDa3c02256bb69b2a20f55d2bd77c5a2fd20bc52

Regards,
Chanwoo Choi