diff mbox

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

Message ID 20160526064541.GA6680@mwanda (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Dan Carpenter May 26, 2016, 6:45 a.m. UTC
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

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

Comments

Chanwoo Choi May 26, 2016, 8:12 a.m. UTC | #1
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

--
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
Dan Carpenter May 26, 2016, 10:02 a.m. UTC | #2
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

--
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
Chanwoo Choi May 26, 2016, 10:16 a.m. UTC | #3
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

--
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
Dan Carpenter May 26, 2016, 10:51 a.m. UTC | #4
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

--
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
Chanwoo Choi May 26, 2016, 10:57 a.m. UTC | #5
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
--
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
Chanwoo Choi May 26, 2016, 11:01 a.m. UTC | #6
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

--
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
Julia Lawall May 26, 2016, 11:13 a.m. UTC | #7
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
>
--
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
Dan Carpenter May 26, 2016, 11:56 a.m. UTC | #8
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

--
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
Chanwoo Choi May 26, 2016, 1:46 p.m. UTC | #9
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
--
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
Julia Lawall May 26, 2016, 1:54 p.m. UTC | #10
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
--
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
Dan Carpenter May 26, 2016, 1:56 p.m. UTC | #11
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

--
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
Chanwoo Choi May 26, 2016, 2:10 p.m. UTC | #12
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
--
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
Julia Lawall May 26, 2016, 2:15 p.m. UTC | #13
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
>
--
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
Dan Carpenter May 26, 2016, 4:06 p.m. UTC | #14
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

--
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 June 1, 2016, 10:25 a.m. UTC | #15
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
Chanwoo Choi June 1, 2016, 11:33 a.m. UTC | #16
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
--
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
diff mbox

Patch

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