| Message ID | 20160526064541.GA6680@mwanda (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html