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 <[email protected]>
---
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;
}
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)
--
2.25.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 <[email protected]>
> ---
> 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/[email protected]/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 <[email protected]>
> > ---
> > 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/[email protected]/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/[email protected]/msg1907800.html
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
> <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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/[email protected]/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/[email protected]/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, 29 Mar 2022 at 08:31, Manivannan Sadhasivam
<[email protected]> 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 <[email protected]>
> > > ---
> > > 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/[email protected]/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/[email protected]/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
> > >