* [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() @ 2016-06-23 12:36 Luis de Bethencourt 2016-06-23 12:36 ` [PATCH v2 2/2] staging: wilc1000: fix error values " Luis de Bethencourt 2016-06-25 21:36 ` [PATCH v2 1/2] staging: wilc1000: fix error handling " Greg KH 0 siblings, 2 replies; 7+ messages in thread From: Luis de Bethencourt @ 2016-06-23 12:36 UTC (permalink / raw) To: linux-kernel Cc: johnny.kim, austin.shin, chris.park, tony.cho, glen.lee, leo.kim, gregkh, linux-wireless, devel, julian.calaby, Luis de Bethencourt The common format to check if a function returned an error pointer is to use PTR_ERR(). Instead of ERR_PTR() which is used to return said errors. Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> Reviewed-by: Julian Calaby <julian.calaby@gmail.com> --- drivers/staging/wilc1000/wilc_debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c index fcbc95d..48797dc 100644 --- a/drivers/staging/wilc1000/wilc_debugfs.c +++ b/drivers/staging/wilc1000/wilc_debugfs.c @@ -107,7 +107,7 @@ static int __init wilc_debugfs_init(void) struct wilc_debugfs_info_t *info; wilc_dir = debugfs_create_dir("wilc_wifi", NULL); - if (wilc_dir == ERR_PTR(-ENODEV)) { + if (PTR_ERR(wilc_dir) == -ENODEV) { /* it's not error. the debugfs is just not being enabled. */ printk("ERR, kernel has built without debugfs support\n"); return 0; -- 2.6.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] staging: wilc1000: fix error values in wilc_debugfs_init() 2016-06-23 12:36 [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() Luis de Bethencourt @ 2016-06-23 12:36 ` Luis de Bethencourt 2016-06-25 21:36 ` Greg KH 2016-06-25 21:36 ` [PATCH v2 1/2] staging: wilc1000: fix error handling " Greg KH 1 sibling, 1 reply; 7+ messages in thread From: Luis de Bethencourt @ 2016-06-23 12:36 UTC (permalink / raw) To: linux-kernel Cc: johnny.kim, austin.shin, chris.park, tony.cho, glen.lee, leo.kim, gregkh, linux-wireless, devel, julian.calaby, Luis de Bethencourt If there was an error, returning -EINVAL is more appropriate than -1. Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> Reviewed-by: Julian Calaby <julian.calaby@gmail.com> --- drivers/staging/wilc1000/wilc_debugfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c index 48797dc..6252931 100644 --- a/drivers/staging/wilc1000/wilc_debugfs.c +++ b/drivers/staging/wilc1000/wilc_debugfs.c @@ -115,7 +115,7 @@ static int __init wilc_debugfs_init(void) if (!wilc_dir) { printk("ERR, debugfs create dir\n"); - return -1; + return -EINVAL; } for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { @@ -129,7 +129,7 @@ static int __init wilc_debugfs_init(void) if (!debugfs_files) { printk("ERR fail to create the debugfs file, %s\n", info->name); debugfs_remove_recursive(wilc_dir); - return -1; + return -EINVAL; } } return 0; -- 2.6.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] staging: wilc1000: fix error values in wilc_debugfs_init() 2016-06-23 12:36 ` [PATCH v2 2/2] staging: wilc1000: fix error values " Luis de Bethencourt @ 2016-06-25 21:36 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2016-06-25 21:36 UTC (permalink / raw) To: Luis de Bethencourt Cc: linux-kernel, johnny.kim, austin.shin, chris.park, tony.cho, glen.lee, leo.kim, linux-wireless, devel, julian.calaby On Thu, Jun 23, 2016 at 01:36:18PM +0100, Luis de Bethencourt wrote: > If there was an error, returning -EINVAL is more appropriate than -1. > > Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> > Reviewed-by: Julian Calaby <julian.calaby@gmail.com> > --- > drivers/staging/wilc1000/wilc_debugfs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c > index 48797dc..6252931 100644 > --- a/drivers/staging/wilc1000/wilc_debugfs.c > +++ b/drivers/staging/wilc1000/wilc_debugfs.c > @@ -115,7 +115,7 @@ static int __init wilc_debugfs_init(void) > > if (!wilc_dir) { > printk("ERR, debugfs create dir\n"); > - return -1; > + return -EINVAL; > } Same here, don't check. > > for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > @@ -129,7 +129,7 @@ static int __init wilc_debugfs_init(void) > if (!debugfs_files) { > printk("ERR fail to create the debugfs file, %s\n", info->name); > debugfs_remove_recursive(wilc_dir); > - return -1; > + return -EINVAL; And here. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() 2016-06-23 12:36 [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() Luis de Bethencourt 2016-06-23 12:36 ` [PATCH v2 2/2] staging: wilc1000: fix error values " Luis de Bethencourt @ 2016-06-25 21:36 ` Greg KH 2016-06-25 21:43 ` Luis de Bethencourt 1 sibling, 1 reply; 7+ messages in thread From: Greg KH @ 2016-06-25 21:36 UTC (permalink / raw) To: Luis de Bethencourt Cc: linux-kernel, devel, linux-wireless, chris.park, austin.shin, johnny.kim, julian.calaby, tony.cho, leo.kim On Thu, Jun 23, 2016 at 01:36:17PM +0100, Luis de Bethencourt wrote: > The common format to check if a function returned an error pointer is to > use PTR_ERR(). Instead of ERR_PTR() which is used to return said errors. > > Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> > Reviewed-by: Julian Calaby <julian.calaby@gmail.com> > --- > drivers/staging/wilc1000/wilc_debugfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c > index fcbc95d..48797dc 100644 > --- a/drivers/staging/wilc1000/wilc_debugfs.c > +++ b/drivers/staging/wilc1000/wilc_debugfs.c > @@ -107,7 +107,7 @@ static int __init wilc_debugfs_init(void) > struct wilc_debugfs_info_t *info; > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > - if (wilc_dir == ERR_PTR(-ENODEV)) { > + if (PTR_ERR(wilc_dir) == -ENODEV) { > /* it's not error. the debugfs is just not being enabled. */ > printk("ERR, kernel has built without debugfs support\n"); > return 0; No, the best way to do this is to just ignore the return value, you don't care about it. It can be passed back into any debugfs calls just fine. So don't check the value and all is good, debugfs was written in a way to make it _easy_ to use, no need for fancy error checking at all with it. thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() 2016-06-25 21:36 ` [PATCH v2 1/2] staging: wilc1000: fix error handling " Greg KH @ 2016-06-25 21:43 ` Luis de Bethencourt 2016-06-25 22:16 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Luis de Bethencourt @ 2016-06-25 21:43 UTC (permalink / raw) To: Greg KH Cc: linux-kernel, devel, linux-wireless, chris.park, austin.shin, johnny.kim, julian.calaby, tony.cho, leo.kim On 25/06/16 22:36, Greg KH wrote: > On Thu, Jun 23, 2016 at 01:36:17PM +0100, Luis de Bethencourt wrote: >> The common format to check if a function returned an error pointer is to >> use PTR_ERR(). Instead of ERR_PTR() which is used to return said errors. >> >> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> >> Reviewed-by: Julian Calaby <julian.calaby@gmail.com> >> --- >> drivers/staging/wilc1000/wilc_debugfs.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c >> index fcbc95d..48797dc 100644 >> --- a/drivers/staging/wilc1000/wilc_debugfs.c >> +++ b/drivers/staging/wilc1000/wilc_debugfs.c >> @@ -107,7 +107,7 @@ static int __init wilc_debugfs_init(void) >> struct wilc_debugfs_info_t *info; >> >> wilc_dir = debugfs_create_dir("wilc_wifi", NULL); >> - if (wilc_dir == ERR_PTR(-ENODEV)) { >> + if (PTR_ERR(wilc_dir) == -ENODEV) { >> /* it's not error. the debugfs is just not being enabled. */ >> printk("ERR, kernel has built without debugfs support\n"); >> return 0; > > No, the best way to do this is to just ignore the return value, you > don't care about it. It can be passed back into any debugfs calls just > fine. > > So don't check the value and all is good, debugfs was written in a way > to make it _easy_ to use, no need for fancy error checking at all with > it. > > thanks, > > greg k-h > Thanks for the review Greg. Just to make sure. You are proposing I just drop the 3 if checks? [0] If that's what you mean I will send a patch as soon as you confirm :) Happy hacking, Luis [0] Making the function look like this: static int __init wilc_debugfs_init(void) { int i; struct dentry *debugfs_files; struct wilc_debugfs_info_t *info; wilc_dir = debugfs_create_dir("wilc_wifi", NULL); for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { info = &debugfs_info[i]; debugfs_files = debugfs_create_file(info->name, info->perm, wilc_dir, &info->data, &info->fops); } return 0; } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() 2016-06-25 21:43 ` Luis de Bethencourt @ 2016-06-25 22:16 ` Greg KH 2016-06-27 13:02 ` Luis de Bethencourt 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2016-06-25 22:16 UTC (permalink / raw) To: Luis de Bethencourt Cc: linux-kernel, devel, linux-wireless, chris.park, austin.shin, johnny.kim, julian.calaby, tony.cho, leo.kim On Sat, Jun 25, 2016 at 10:43:33PM +0100, Luis de Bethencourt wrote: > On 25/06/16 22:36, Greg KH wrote: > > On Thu, Jun 23, 2016 at 01:36:17PM +0100, Luis de Bethencourt wrote: > >> The common format to check if a function returned an error pointer is to > >> use PTR_ERR(). Instead of ERR_PTR() which is used to return said errors. > >> > >> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> > >> Reviewed-by: Julian Calaby <julian.calaby@gmail.com> > >> --- > >> drivers/staging/wilc1000/wilc_debugfs.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c > >> index fcbc95d..48797dc 100644 > >> --- a/drivers/staging/wilc1000/wilc_debugfs.c > >> +++ b/drivers/staging/wilc1000/wilc_debugfs.c > >> @@ -107,7 +107,7 @@ static int __init wilc_debugfs_init(void) > >> struct wilc_debugfs_info_t *info; > >> > >> wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > >> - if (wilc_dir == ERR_PTR(-ENODEV)) { > >> + if (PTR_ERR(wilc_dir) == -ENODEV) { > >> /* it's not error. the debugfs is just not being enabled. */ > >> printk("ERR, kernel has built without debugfs support\n"); > >> return 0; > > > > No, the best way to do this is to just ignore the return value, you > > don't care about it. It can be passed back into any debugfs calls just > > fine. > > > > So don't check the value and all is good, debugfs was written in a way > > to make it _easy_ to use, no need for fancy error checking at all with > > it. > > > > thanks, > > > > greg k-h > > > > Thanks for the review Greg. > > Just to make sure. You are proposing I just drop the 3 if checks? [0] > > If that's what you mean I will send a patch as soon as you confirm :) > > Happy hacking, > Luis > > > > [0] Making the function look like this: > static int __init wilc_debugfs_init(void) > { > int i; > > struct dentry *debugfs_files; > struct wilc_debugfs_info_t *info; > > wilc_dir = debugfs_create_dir("wilc_wifi", NULL); > for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { > info = &debugfs_info[i]; > debugfs_files = debugfs_create_file(info->name, > info->perm, > wilc_dir, > &info->data, > &info->fops); Why even assign anything to debugfs_files? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() 2016-06-25 22:16 ` Greg KH @ 2016-06-27 13:02 ` Luis de Bethencourt 0 siblings, 0 replies; 7+ messages in thread From: Luis de Bethencourt @ 2016-06-27 13:02 UTC (permalink / raw) To: Greg KH; +Cc: linux-kernel, devel, linux-wireless, julian.calaby On 25/06/16 23:16, Greg KH wrote: > On Sat, Jun 25, 2016 at 10:43:33PM +0100, Luis de Bethencourt wrote: >> On 25/06/16 22:36, Greg KH wrote: >>> On Thu, Jun 23, 2016 at 01:36:17PM +0100, Luis de Bethencourt wrote: >>>> The common format to check if a function returned an error pointer is to >>>> use PTR_ERR(). Instead of ERR_PTR() which is used to return said errors. >>>> >>>> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> >>>> Reviewed-by: Julian Calaby <julian.calaby@gmail.com> >>>> --- >>>> drivers/staging/wilc1000/wilc_debugfs.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/staging/wilc1000/wilc_debugfs.c b/drivers/staging/wilc1000/wilc_debugfs.c >>>> index fcbc95d..48797dc 100644 >>>> --- a/drivers/staging/wilc1000/wilc_debugfs.c >>>> +++ b/drivers/staging/wilc1000/wilc_debugfs.c >>>> @@ -107,7 +107,7 @@ static int __init wilc_debugfs_init(void) >>>> struct wilc_debugfs_info_t *info; >>>> >>>> wilc_dir = debugfs_create_dir("wilc_wifi", NULL); >>>> - if (wilc_dir == ERR_PTR(-ENODEV)) { >>>> + if (PTR_ERR(wilc_dir) == -ENODEV) { >>>> /* it's not error. the debugfs is just not being enabled. */ >>>> printk("ERR, kernel has built without debugfs support\n"); >>>> return 0; >>> >>> No, the best way to do this is to just ignore the return value, you >>> don't care about it. It can be passed back into any debugfs calls just >>> fine. >>> >>> So don't check the value and all is good, debugfs was written in a way >>> to make it _easy_ to use, no need for fancy error checking at all with >>> it. >>> >>> thanks, >>> >>> greg k-h >>> >> >> Thanks for the review Greg. >> >> Just to make sure. You are proposing I just drop the 3 if checks? [0] >> >> If that's what you mean I will send a patch as soon as you confirm :) >> >> Happy hacking, >> Luis >> >> >> >> [0] Making the function look like this: >> static int __init wilc_debugfs_init(void) >> { >> int i; >> >> struct dentry *debugfs_files; >> struct wilc_debugfs_info_t *info; >> >> wilc_dir = debugfs_create_dir("wilc_wifi", NULL); >> for (i = 0; i < ARRAY_SIZE(debugfs_info); i++) { >> info = &debugfs_info[i]; >> debugfs_files = debugfs_create_file(info->name, >> info->perm, >> wilc_dir, >> &info->data, >> &info->fops); > > Why even assign anything to debugfs_files? > Sorry Greg. I was just confirming I understood your suggestion, and pasted that without cleaning the code. Thanks for review and idea! Luis ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-06-27 13:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-23 12:36 [PATCH v2 1/2] staging: wilc1000: fix error handling in wilc_debugfs_init() Luis de Bethencourt 2016-06-23 12:36 ` [PATCH v2 2/2] staging: wilc1000: fix error values " Luis de Bethencourt 2016-06-25 21:36 ` Greg KH 2016-06-25 21:36 ` [PATCH v2 1/2] staging: wilc1000: fix error handling " Greg KH 2016-06-25 21:43 ` Luis de Bethencourt 2016-06-25 22:16 ` Greg KH 2016-06-27 13:02 ` Luis de Bethencourt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).