| Message ID | 20190618155830.GB21825@kroah.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | watchdog: mei_wdt: no need to check return value of debugfs_create functions | expand |
On Tue, Jun 18, 2019 at 05:58:30PM +0200, Greg Kroah-Hartman 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: Tomas Winkler <tomas.winkler@intel.com> > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: linux-watchdog@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/mei_wdt.c | 30 +++++++----------------------- > 1 file changed, 7 insertions(+), 23 deletions(-) > > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c > index 8023cf28657a..96a770938ff0 100644 > --- a/drivers/watchdog/mei_wdt.c > +++ b/drivers/watchdog/mei_wdt.c > @@ -539,38 +539,23 @@ static void dbgfs_unregister(struct mei_wdt *wdt) > wdt->dbgfs_dir = NULL; > } > > -static int dbgfs_register(struct mei_wdt *wdt) > +static void dbgfs_register(struct mei_wdt *wdt) > { > - struct dentry *dir, *f; > + struct dentry *dir; > > dir = debugfs_create_dir(KBUILD_MODNAME, NULL); > - if (!dir) > - return -ENOMEM; > - > wdt->dbgfs_dir = dir; > - f = debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state); > - if (!f) > - goto err; > > - f = debugfs_create_file("activation", S_IRUSR, > - dir, wdt, &dbgfs_fops_activation); > - if (!f) > - goto err; > + debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state); > > - return 0; > -err: > - dbgfs_unregister(wdt); > - return -ENODEV; > + debugfs_create_file("activation", S_IRUSR, dir, wdt, > + &dbgfs_fops_activation); > } > > #else > > static inline void dbgfs_unregister(struct mei_wdt *wdt) {} > - > -static inline int dbgfs_register(struct mei_wdt *wdt) > -{ > - return 0; > -} > +static inline void dbgfs_register(struct mei_wdt *wdt) {} > #endif /* CONFIG_DEBUG_FS */ > > static int mei_wdt_probe(struct mei_cl_device *cldev, > @@ -623,8 +608,7 @@ static int mei_wdt_probe(struct mei_cl_device *cldev, > if (ret) > goto err_disable; > > - if (dbgfs_register(wdt)) > - dev_warn(&cldev->dev, "cannot register debugfs\n"); > + dbgfs_register(wdt); > > return 0; > > -- > 2.22.0 >
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > --- > > drivers/watchdog/mei_wdt.c | 30 +++++++----------------------- > > 1 file changed, 7 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c > > index 8023cf28657a..96a770938ff0 100644 > > --- a/drivers/watchdog/mei_wdt.c > > +++ b/drivers/watchdog/mei_wdt.c > > @@ -539,38 +539,23 @@ static void dbgfs_unregister(struct mei_wdt *wdt) > > wdt->dbgfs_dir = NULL; > > } > > > > -static int dbgfs_register(struct mei_wdt *wdt) > > +static void dbgfs_register(struct mei_wdt *wdt) > > { > > - struct dentry *dir, *f; > > + struct dentry *dir; > > > > dir = debugfs_create_dir(KBUILD_MODNAME, NULL); > > - if (!dir) > > - return -ENOMEM; If create dir fails but the consequent create file succeeds, I would guess this will be a mess in the debugs root directory. Also the tear down won't work as debugfs_remove_recursive will just bail out on NULL check. Isn't it better just return here, when create_dir fails? > > - > > wdt->dbgfs_dir = dir; > > - f = debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state); > > - if (!f) > > - goto err; > > > > - f = debugfs_create_file("activation", S_IRUSR, > > - dir, wdt, &dbgfs_fops_activation); > > - if (!f) > > - goto err; > > + debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state); > > > > - return 0; > > -err: > > - dbgfs_unregister(wdt); > > - return -ENODEV; > > + debugfs_create_file("activation", S_IRUSR, dir, wdt, > > + &dbgfs_fops_activation); > > } > > > > #else > > > > static inline void dbgfs_unregister(struct mei_wdt *wdt) {} > > - > > -static inline int dbgfs_register(struct mei_wdt *wdt) -{ > > - return 0; > > -} > > +static inline void dbgfs_register(struct mei_wdt *wdt) {} > > #endif /* CONFIG_DEBUG_FS */ > > > > static int mei_wdt_probe(struct mei_cl_device *cldev, @@ -623,8 > > +608,7 @@ static int mei_wdt_probe(struct mei_cl_device *cldev, > > if (ret) > > goto err_disable; > > > > - if (dbgfs_register(wdt)) > > - dev_warn(&cldev->dev, "cannot register debugfs\n"); > > + dbgfs_register(wdt); > > > > return 0; > > > > -- > > 2.22.0 > >
On Tue, Jun 18, 2019 at 05:58:41PM +0000, Winkler, Tomas wrote: > > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > > > --- > > > drivers/watchdog/mei_wdt.c | 30 +++++++----------------------- > > > 1 file changed, 7 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c > > > index 8023cf28657a..96a770938ff0 100644 > > > --- a/drivers/watchdog/mei_wdt.c > > > +++ b/drivers/watchdog/mei_wdt.c > > > @@ -539,38 +539,23 @@ static void dbgfs_unregister(struct mei_wdt *wdt) > > > wdt->dbgfs_dir = NULL; > > > } > > > > > > -static int dbgfs_register(struct mei_wdt *wdt) > > > +static void dbgfs_register(struct mei_wdt *wdt) > > > { > > > - struct dentry *dir, *f; > > > + struct dentry *dir; > > > > > > dir = debugfs_create_dir(KBUILD_MODNAME, NULL); > > > - if (!dir) > > > - return -ENOMEM; > If create dir fails but the consequent create file succeeds, I would > guess this will be a mess in the debugs root directory. Nope, debugfs_create_dir() can not return NULL, so no files will be created based on a -ERROR as a parent. > Also the tear down won't work as debugfs_remove_recursive will just > bail out on NULL check. Nope, NULL can not be returned from debugfs_create*() functions anymore. > Isn't it better just return here, when create_dir fails? Nope, no need to :) thanks, greg k-h
> > On Tue, Jun 18, 2019 at 05:58:41PM +0000, Winkler, Tomas wrote: > > > > > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > > > > > --- > > > > drivers/watchdog/mei_wdt.c | 30 +++++++----------------------- > > > > 1 file changed, 7 insertions(+), 23 deletions(-) > > > > > > > > diff --git a/drivers/watchdog/mei_wdt.c > > > > b/drivers/watchdog/mei_wdt.c index 8023cf28657a..96a770938ff0 > > > > 100644 > > > > --- a/drivers/watchdog/mei_wdt.c > > > > +++ b/drivers/watchdog/mei_wdt.c > > > > @@ -539,38 +539,23 @@ static void dbgfs_unregister(struct mei_wdt > *wdt) > > > > wdt->dbgfs_dir = NULL; > > > > } > > > > > > > > -static int dbgfs_register(struct mei_wdt *wdt) > > > > +static void dbgfs_register(struct mei_wdt *wdt) > > > > { > > > > - struct dentry *dir, *f; > > > > + struct dentry *dir; > > > > > > > > dir = debugfs_create_dir(KBUILD_MODNAME, NULL); > > > > - if (!dir) > > > > - return -ENOMEM; > > If create dir fails but the consequent create file succeeds, I would > > guess this will be a mess in the debugs root directory. > > Nope, debugfs_create_dir() can not return NULL, so no files will be created > based on a -ERROR as a parent. > > > Also the tear down won't work as debugfs_remove_recursive will just > > bail out on NULL check. > > Nope, NULL can not be returned from debugfs_create*() functions anymore. > > > Isn't it better just return here, when create_dir fails? > > Nope, no need to :) I see, not a fun of those dirty tricks but I admit it streamlines the code. Ack Thanks Tomas
diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c index 8023cf28657a..96a770938ff0 100644 --- a/drivers/watchdog/mei_wdt.c +++ b/drivers/watchdog/mei_wdt.c @@ -539,38 +539,23 @@ static void dbgfs_unregister(struct mei_wdt *wdt) wdt->dbgfs_dir = NULL; } -static int dbgfs_register(struct mei_wdt *wdt) +static void dbgfs_register(struct mei_wdt *wdt) { - struct dentry *dir, *f; + struct dentry *dir; dir = debugfs_create_dir(KBUILD_MODNAME, NULL); - if (!dir) - return -ENOMEM; - wdt->dbgfs_dir = dir; - f = debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state); - if (!f) - goto err; - f = debugfs_create_file("activation", S_IRUSR, - dir, wdt, &dbgfs_fops_activation); - if (!f) - goto err; + debugfs_create_file("state", S_IRUSR, dir, wdt, &dbgfs_fops_state); - return 0; -err: - dbgfs_unregister(wdt); - return -ENODEV; + debugfs_create_file("activation", S_IRUSR, dir, wdt, + &dbgfs_fops_activation); } #else static inline void dbgfs_unregister(struct mei_wdt *wdt) {} - -static inline int dbgfs_register(struct mei_wdt *wdt) -{ - return 0; -} +static inline void dbgfs_register(struct mei_wdt *wdt) {} #endif /* CONFIG_DEBUG_FS */ static int mei_wdt_probe(struct mei_cl_device *cldev, @@ -623,8 +608,7 @@ static int mei_wdt_probe(struct mei_cl_device *cldev, if (ret) goto err_disable; - if (dbgfs_register(wdt)) - dev_warn(&cldev->dev, "cannot register debugfs\n"); + dbgfs_register(wdt); return 0;
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: Tomas Winkler <tomas.winkler@intel.com> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> Cc: Guenter Roeck <linux@roeck-us.net> Cc: linux-watchdog@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/watchdog/mei_wdt.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-)