| Message ID | 20190122152151.16139-6-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | power: domain: no need to check return value of debugfs_create functions | expand |
On Tue, 22 Jan 2019 at 16:23, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. Doesn't this boils done to whether we want to care to check if memory allocation failed? Somewhere down the call chain from debugfs_create_dir(), we end up in alloc_inode() and it looks like that can fail, no? Kind regards Uffe > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Len Brown <len.brown@intel.com> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/base/power/domain.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 500de1dee967..45eafe8cf7dd 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2948,18 +2948,11 @@ static int __init genpd_debug_init(void) > > genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL); > > - if (!genpd_debugfs_dir) > - return -ENOMEM; > - > - d = debugfs_create_file("pm_genpd_summary", S_IRUGO, > - genpd_debugfs_dir, NULL, &summary_fops); > - if (!d) > - return -ENOMEM; > + debugfs_create_file("pm_genpd_summary", S_IRUGO, genpd_debugfs_dir, > + NULL, &summary_fops); > > list_for_each_entry(genpd, &gpd_list, gpd_list_node) { > d = debugfs_create_dir(genpd->name, genpd_debugfs_dir); > - if (!d) > - return -ENOMEM; > > debugfs_create_file("current_state", 0444, > d, genpd, &status_fops); > -- > 2.20.1 >
On Wed, Jan 23, 2019 at 08:44:36AM +0100, Ulf Hansson wrote: > On Tue, 22 Jan 2019 at 16:23, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > When calling debugfs functions, there is no need to ever check the > > return value. The function can work or not, but the code logic should > > never do something different based on this. > > Doesn't this boils done to whether we want to care to check if memory > allocation failed? You should not care. > Somewhere down the call chain from debugfs_create_dir(), we end up in > alloc_inode() and it looks like that can fail, no? Yes it can, right now it will return NULL, I'll go change that to return ENOMEM, but even then, your really do not care what happens as none of your other code flow should ever care about what debugfs does, or does not, do. thanks, greg k-h
On Wed, 23 Jan 2019 at 08:59, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 23, 2019 at 08:44:36AM +0100, Ulf Hansson wrote: > > On Tue, 22 Jan 2019 at 16:23, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > When calling debugfs functions, there is no need to ever check the > > > return value. The function can work or not, but the code logic should > > > never do something different based on this. > > > > Doesn't this boils done to whether we want to care to check if memory > > allocation failed? > > You should not care. Okay. > > > Somewhere down the call chain from debugfs_create_dir(), we end up in > > alloc_inode() and it looks like that can fail, no? > > Yes it can, right now it will return NULL, I'll go change that to return > ENOMEM, but even then, your really do not care what happens as none of > your other code flow should ever care about what debugfs does, or does > not, do. In that case, why don't we convert the debugfs_create_dir() and friends, to becomes "void" functions? Or maybe that's your plan going forward? Kind regards Uffe
On Wed, Jan 23, 2019 at 09:20:05AM +0100, Ulf Hansson wrote: > On Wed, 23 Jan 2019 at 08:59, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Jan 23, 2019 at 08:44:36AM +0100, Ulf Hansson wrote: > > > On Tue, 22 Jan 2019 at 16:23, Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > When calling debugfs functions, there is no need to ever check the > > > > return value. The function can work or not, but the code logic should > > > > never do something different based on this. > > > > > > Doesn't this boils done to whether we want to care to check if memory > > > allocation failed? > > > > You should not care. > > Okay. > > > > > > Somewhere down the call chain from debugfs_create_dir(), we end up in > > > alloc_inode() and it looks like that can fail, no? > > > > Yes it can, right now it will return NULL, I'll go change that to return > > ENOMEM, but even then, your really do not care what happens as none of > > your other code flow should ever care about what debugfs does, or does > > not, do. > > In that case, why don't we convert the debugfs_create_dir() and > friends, to becomes "void" functions? Or maybe that's your plan going > forward? I can't, as sometimes you actually care about using the return value in another debugfs call. thanks, greg k-h
On Tue, 22 Jan 2019 at 16:23, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Len Brown <len.brown@intel.com> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/domain.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 500de1dee967..45eafe8cf7dd 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -2948,18 +2948,11 @@ static int __init genpd_debug_init(void) > > genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL); > > - if (!genpd_debugfs_dir) > - return -ENOMEM; > - > - d = debugfs_create_file("pm_genpd_summary", S_IRUGO, > - genpd_debugfs_dir, NULL, &summary_fops); > - if (!d) > - return -ENOMEM; > + debugfs_create_file("pm_genpd_summary", S_IRUGO, genpd_debugfs_dir, > + NULL, &summary_fops); > > list_for_each_entry(genpd, &gpd_list, gpd_list_node) { > d = debugfs_create_dir(genpd->name, genpd_debugfs_dir); > - if (!d) > - return -ENOMEM; > > debugfs_create_file("current_state", 0444, > d, genpd, &status_fops); > -- > 2.20.1 >
On Wednesday, January 23, 2019 1:49:00 PM CET Ulf Hansson wrote: > On Tue, 22 Jan 2019 at 16:23, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > When calling debugfs functions, there is no need to ever check the > > return value. The function can work or not, but the code logic should > > never do something different based on this. > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > Cc: Kevin Hilman <khilman@kernel.org> > > Cc: Ulf Hansson <ulf.hansson@linaro.org> > > Cc: Len Brown <len.brown@intel.com> > > Cc: linux-pm@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Patch applied, thanks!
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 500de1dee967..45eafe8cf7dd 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -2948,18 +2948,11 @@ static int __init genpd_debug_init(void) genpd_debugfs_dir = debugfs_create_dir("pm_genpd", NULL); - if (!genpd_debugfs_dir) - return -ENOMEM; - - d = debugfs_create_file("pm_genpd_summary", S_IRUGO, - genpd_debugfs_dir, NULL, &summary_fops); - if (!d) - return -ENOMEM; + debugfs_create_file("pm_genpd_summary", S_IRUGO, genpd_debugfs_dir, + NULL, &summary_fops); list_for_each_entry(genpd, &gpd_list, gpd_list_node) { d = debugfs_create_dir(genpd->name, genpd_debugfs_dir); - if (!d) - return -ENOMEM; debugfs_create_file("current_state", 0444, d, genpd, &status_fops);
When calling debugfs functions, there is no need to ever check the return value. The function can work or not, but the code logic should never do something different based on this. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Kevin Hilman <khilman@kernel.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Len Brown <len.brown@intel.com> Cc: linux-pm@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/base/power/domain.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)