diff mbox series

remoteproc: Don't bother checking the return value of debugfs_create*

Message ID 20220324181224.21542-1-manivannan.sadhasivam@linaro.org (mailing list archive)
State Superseded
Headers show
Series remoteproc: Don't bother checking the return value of debugfs_create* | expand

Commit Message

Manivannan Sadhasivam March 24, 2022, 6:12 p.m. UTC
DebugFS APIs are designed to return only the error pointers and not NULL
in the case of failure. So these return pointers are safe to be passed on
to the successive debugfs_create* APIs.

Therefore, let's just get rid of the checks.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/remoteproc/remoteproc_debugfs.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

Comments

Mathieu Poirier March 28, 2022, 3:51 p.m. UTC | #1
Hi Mani,

On Thu, Mar 24, 2022 at 11:42:24PM +0530, Manivannan Sadhasivam wrote:
> DebugFS APIs are designed to return only the error pointers and not NULL
> in the case of failure. So these return pointers are safe to be passed on
> to the successive debugfs_create* APIs.
> 
> Therefore, let's just get rid of the checks.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_debugfs.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index b5a1e3b697d9..2e2c4a31c154 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -386,16 +386,8 @@ void rproc_remove_trace_file(struct dentry *tfile)
>  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
>  				       struct rproc_debug_trace *trace)
>  {
> -	struct dentry *tfile;
> -
> -	tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> +	return debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
>  				    &trace_rproc_ops);
> -	if (!tfile) {
> -		dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
> -		return NULL;
> -	}
> -
> -	return tfile;

Please see this thread [1] for an earlier conversation on this topic.

[1]. https://lore.kernel.org/lkml/20220105131022.25247-1-linmq006@gmail.com/T/

>  }
>  
>  void rproc_delete_debug_dir(struct rproc *rproc)
> @@ -411,8 +403,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
>  		return;
>  
>  	rproc->dbg_dir = debugfs_create_dir(dev_name(dev), rproc_dbg);
> -	if (!rproc->dbg_dir)
> -		return;
> 
>  	debugfs_create_file("name", 0400, rproc->dbg_dir,
>  			    rproc, &rproc_name_ops);
> @@ -430,11 +420,8 @@ void rproc_create_debug_dir(struct rproc *rproc)
>  
>  void __init rproc_init_debugfs(void)
>  {
> -	if (debugfs_initialized()) {
> +	if (debugfs_initialized())
>  		rproc_dbg = debugfs_create_dir(KBUILD_MODNAME, NULL);
> -		if (!rproc_dbg)
> -			pr_err("can't create debugfs dir\n");
> -	}

The above two are fine since debugfs_create_file() and debugfs_create_dir() can
deal with @parent being an error code.

Thanks,
Mathieu

>  }
>  
>  void __exit rproc_exit_debugfs(void)
> -- 
> 2.25.1
>
Manivannan Sadhasivam March 29, 2022, 2:31 p.m. UTC | #2
Hi Mathieu,

On Mon, Mar 28, 2022 at 09:51:23AM -0600, Mathieu Poirier wrote:
> Hi Mani,
> 
> On Thu, Mar 24, 2022 at 11:42:24PM +0530, Manivannan Sadhasivam wrote:
> > DebugFS APIs are designed to return only the error pointers and not NULL
> > in the case of failure. So these return pointers are safe to be passed on
> > to the successive debugfs_create* APIs.
> > 
> > Therefore, let's just get rid of the checks.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_debugfs.c | 17 ++---------------
> >  1 file changed, 2 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > index b5a1e3b697d9..2e2c4a31c154 100644
> > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > @@ -386,16 +386,8 @@ void rproc_remove_trace_file(struct dentry *tfile)
> >  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> >  				       struct rproc_debug_trace *trace)
> >  {
> > -	struct dentry *tfile;
> > -
> > -	tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> > +	return debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> >  				    &trace_rproc_ops);
> > -	if (!tfile) {
> > -		dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
> > -		return NULL;
> > -	}
> > -
> > -	return tfile;
> 
> Please see this thread [1] for an earlier conversation on this topic.
> 
> [1]. https://lore.kernel.org/lkml/20220105131022.25247-1-linmq006@gmail.com/T/
> 

Thanks for the pointer! I believe the conclusion was to return 0 here
and ignore the return from debugfs_create_file(). If that's the case, it looks
fine to me and I'll send a follow-up patch.

> >  }
> >  
> >  void rproc_delete_debug_dir(struct rproc *rproc)
> > @@ -411,8 +403,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
> >  		return;
> >  
> >  	rproc->dbg_dir = debugfs_create_dir(dev_name(dev), rproc_dbg);
> > -	if (!rproc->dbg_dir)
> > -		return;
> > 
> >  	debugfs_create_file("name", 0400, rproc->dbg_dir,
> >  			    rproc, &rproc_name_ops);
> > @@ -430,11 +420,8 @@ void rproc_create_debug_dir(struct rproc *rproc)
> >  
> >  void __init rproc_init_debugfs(void)
> >  {
> > -	if (debugfs_initialized()) {
> > +	if (debugfs_initialized())
> >  		rproc_dbg = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > -		if (!rproc_dbg)
> > -			pr_err("can't create debugfs dir\n");
> > -	}
> 
> The above two are fine since debugfs_create_file() and debugfs_create_dir() can
> deal with @parent being an error code.
> 

debugfs_create_* APIs would never return NULL, so these checks are wrong.
Moreover, Greg recommends not to check the return value for any of these
functions.

I've found the mail thread where Greg explained the reasoning behind it:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1907800.html

Thanks,
Mani

> Thanks,
> Mathieu
> 
> >  }
> >  
> >  void __exit rproc_exit_debugfs(void)
> > -- 
> > 2.25.1
> >
Mathieu Poirier March 29, 2022, 2:59 p.m. UTC | #3
On Tue, 29 Mar 2022 at 08:31, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Hi Mathieu,
>
> On Mon, Mar 28, 2022 at 09:51:23AM -0600, Mathieu Poirier wrote:
> > Hi Mani,
> >
> > On Thu, Mar 24, 2022 at 11:42:24PM +0530, Manivannan Sadhasivam wrote:
> > > DebugFS APIs are designed to return only the error pointers and not NULL
> > > in the case of failure. So these return pointers are safe to be passed on
> > > to the successive debugfs_create* APIs.
> > >
> > > Therefore, let's just get rid of the checks.
> > >
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/remoteproc/remoteproc_debugfs.c | 17 ++---------------
> > >  1 file changed, 2 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > > index b5a1e3b697d9..2e2c4a31c154 100644
> > > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > > @@ -386,16 +386,8 @@ void rproc_remove_trace_file(struct dentry *tfile)
> > >  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> > >                                    struct rproc_debug_trace *trace)
> > >  {
> > > -   struct dentry *tfile;
> > > -
> > > -   tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> > > +   return debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> > >                                 &trace_rproc_ops);
> > > -   if (!tfile) {
> > > -           dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
> > > -           return NULL;
> > > -   }
> > > -
> > > -   return tfile;
> >
> > Please see this thread [1] for an earlier conversation on this topic.
> >
> > [1]. https://lore.kernel.org/lkml/20220105131022.25247-1-linmq006@gmail.com/T/
> >
>
> Thanks for the pointer! I believe the conclusion was to return 0 here
> and ignore the return from debugfs_create_file(). If that's the case, it looks
> fine to me and I'll send a follow-up patch.

Correct.

>
> > >  }
> > >
> > >  void rproc_delete_debug_dir(struct rproc *rproc)
> > > @@ -411,8 +403,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
> > >             return;
> > >
> > >     rproc->dbg_dir = debugfs_create_dir(dev_name(dev), rproc_dbg);
> > > -   if (!rproc->dbg_dir)
> > > -           return;
> > >
> > >     debugfs_create_file("name", 0400, rproc->dbg_dir,
> > >                         rproc, &rproc_name_ops);
> > > @@ -430,11 +420,8 @@ void rproc_create_debug_dir(struct rproc *rproc)
> > >
> > >  void __init rproc_init_debugfs(void)
> > >  {
> > > -   if (debugfs_initialized()) {
> > > +   if (debugfs_initialized())
> > >             rproc_dbg = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > > -           if (!rproc_dbg)
> > > -                   pr_err("can't create debugfs dir\n");
> > > -   }
> >
> > The above two are fine since debugfs_create_file() and debugfs_create_dir() can
> > deal with @parent being an error code.
> >
>
> debugfs_create_* APIs would never return NULL, so these checks are wrong.
> Moreover, Greg recommends not to check the return value for any of these
> functions.
>

When writing "the above two are fine", I meant that I am in agreement
with your changes.  Reading my comment again I can see how it could be
interpreted as "I don't think your changes are necessary", which isn't
the case.

> I've found the mail thread where Greg explained the reasoning behind it:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1907800.html
>

I'll bookmark this one as it is bound to come back again.

> Thanks,
> Mani
>
> > Thanks,
> > Mathieu
> >
> > >  }
> > >
> > >  void __exit rproc_exit_debugfs(void)
> > > --
> > > 2.25.1
> > >
Manivannan Sadhasivam March 29, 2022, 3:35 p.m. UTC | #4
On Tue, Mar 29, 2022 at 08:59:48AM -0600, Mathieu Poirier wrote:
> On Tue, 29 Mar 2022 at 08:31, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > Hi Mathieu,
> >
> > On Mon, Mar 28, 2022 at 09:51:23AM -0600, Mathieu Poirier wrote:
> > > Hi Mani,
> > >
> > > On Thu, Mar 24, 2022 at 11:42:24PM +0530, Manivannan Sadhasivam wrote:
> > > > DebugFS APIs are designed to return only the error pointers and not NULL
> > > > in the case of failure. So these return pointers are safe to be passed on
> > > > to the successive debugfs_create* APIs.
> > > >
> > > > Therefore, let's just get rid of the checks.
> > > >
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  drivers/remoteproc/remoteproc_debugfs.c | 17 ++---------------
> > > >  1 file changed, 2 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > > > index b5a1e3b697d9..2e2c4a31c154 100644
> > > > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > > > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > > > @@ -386,16 +386,8 @@ void rproc_remove_trace_file(struct dentry *tfile)
> > > >  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> > > >                                    struct rproc_debug_trace *trace)
> > > >  {
> > > > -   struct dentry *tfile;
> > > > -
> > > > -   tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> > > > +   return debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
> > > >                                 &trace_rproc_ops);
> > > > -   if (!tfile) {
> > > > -           dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
> > > > -           return NULL;
> > > > -   }
> > > > -
> > > > -   return tfile;
> > >
> > > Please see this thread [1] for an earlier conversation on this topic.
> > >
> > > [1]. https://lore.kernel.org/lkml/20220105131022.25247-1-linmq006@gmail.com/T/
> > >
> >
> > Thanks for the pointer! I believe the conclusion was to return 0 here
> > and ignore the return from debugfs_create_file(). If that's the case, it looks
> > fine to me and I'll send a follow-up patch.
> 
> Correct.
> 
> >
> > > >  }
> > > >
> > > >  void rproc_delete_debug_dir(struct rproc *rproc)
> > > > @@ -411,8 +403,6 @@ void rproc_create_debug_dir(struct rproc *rproc)
> > > >             return;
> > > >
> > > >     rproc->dbg_dir = debugfs_create_dir(dev_name(dev), rproc_dbg);
> > > > -   if (!rproc->dbg_dir)
> > > > -           return;
> > > >
> > > >     debugfs_create_file("name", 0400, rproc->dbg_dir,
> > > >                         rproc, &rproc_name_ops);
> > > > @@ -430,11 +420,8 @@ void rproc_create_debug_dir(struct rproc *rproc)
> > > >
> > > >  void __init rproc_init_debugfs(void)
> > > >  {
> > > > -   if (debugfs_initialized()) {
> > > > +   if (debugfs_initialized())
> > > >             rproc_dbg = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > > > -           if (!rproc_dbg)
> > > > -                   pr_err("can't create debugfs dir\n");
> > > > -   }
> > >
> > > The above two are fine since debugfs_create_file() and debugfs_create_dir() can
> > > deal with @parent being an error code.
> > >
> >
> > debugfs_create_* APIs would never return NULL, so these checks are wrong.
> > Moreover, Greg recommends not to check the return value for any of these
> > functions.
> >
> 
> When writing "the above two are fine", I meant that I am in agreement
> with your changes.  Reading my comment again I can see how it could be
> interpreted as "I don't think your changes are necessary", which isn't
> the case.
> 

Sorry for the misinterpretation. Will send v2.

Thanks,
Mani

> > I've found the mail thread where Greg explained the reasoning behind it:
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1907800.html
> >
> 
> I'll bookmark this one as it is bound to come back again.
> 
> > Thanks,
> > Mani
> >
> > > Thanks,
> > > Mathieu
> > >
> > > >  }
> > > >
> > > >  void __exit rproc_exit_debugfs(void)
> > > > --
> > > > 2.25.1
> > > >
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index b5a1e3b697d9..2e2c4a31c154 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -386,16 +386,8 @@  void rproc_remove_trace_file(struct dentry *tfile)
 struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
 				       struct rproc_debug_trace *trace)
 {
-	struct dentry *tfile;
-
-	tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
+	return debugfs_create_file(name, 0400, rproc->dbg_dir, trace,
 				    &trace_rproc_ops);
-	if (!tfile) {
-		dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
-		return NULL;
-	}
-
-	return tfile;
 }
 
 void rproc_delete_debug_dir(struct rproc *rproc)
@@ -411,8 +403,6 @@  void rproc_create_debug_dir(struct rproc *rproc)
 		return;
 
 	rproc->dbg_dir = debugfs_create_dir(dev_name(dev), rproc_dbg);
-	if (!rproc->dbg_dir)
-		return;
 
 	debugfs_create_file("name", 0400, rproc->dbg_dir,
 			    rproc, &rproc_name_ops);
@@ -430,11 +420,8 @@  void rproc_create_debug_dir(struct rproc *rproc)
 
 void __init rproc_init_debugfs(void)
 {
-	if (debugfs_initialized()) {
+	if (debugfs_initialized())
 		rproc_dbg = debugfs_create_dir(KBUILD_MODNAME, NULL);
-		if (!rproc_dbg)
-			pr_err("can't create debugfs dir\n");
-	}
 }
 
 void __exit rproc_exit_debugfs(void)