Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Mar 2022 20:01:46 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     bjorn.andersson@linaro.org, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] remoteproc: Don't bother checking the return value of
 debugfs_create*
Message-ID: <20220329143146.GA2137@thinkpad>
References: <20220324181224.21542-1-manivannan.sadhasivam@linaro.org>
 <20220328155123.GA3722211@p14s>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220328155123.GA3722211@p14s>
Precedence: bulk

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
> >