diff mbox series

watchdog: mei_wdt: no need to check return value of debugfs_create functions

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

Commit Message

Greg KH June 18, 2019, 3:58 p.m. UTC
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(-)

Comments

Guenter Roeck June 18, 2019, 5:06 p.m. UTC | #1
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
>
Winkler, Tomas June 18, 2019, 5:58 p.m. UTC | #2
> 
> 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
> >
Greg KH June 18, 2019, 6:04 p.m. UTC | #3
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
Winkler, Tomas June 18, 2019, 6:45 p.m. UTC | #4
> 
> 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 mbox series

Patch

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;