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