* [PATCH] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file @ 2021-12-27 9:06 Miaoqian Lin 2022-01-04 17:46 ` Mathieu Poirier 0 siblings, 1 reply; 9+ messages in thread From: Miaoqian Lin @ 2021-12-27 9:06 UTC (permalink / raw) Cc: linmq006, Ohad Ben-Cohen, Bjorn Andersson, Mathieu Poirier, linux-remoteproc, linux-kernel The debugfs_create_file() function doesn't return NULL. It returns error pointers. Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- drivers/remoteproc/remoteproc_debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c index b5a1e3b697d9..a2409fe2f57b 100644 --- a/drivers/remoteproc/remoteproc_debugfs.c +++ b/drivers/remoteproc/remoteproc_debugfs.c @@ -390,7 +390,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, &trace_rproc_ops); - if (!tfile) { + if (IS_ERR(tfile)) { dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); return NULL; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2021-12-27 9:06 [PATCH] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file Miaoqian Lin @ 2022-01-04 17:46 ` Mathieu Poirier 2022-01-05 6:42 ` [PATCH v2] " Miaoqian Lin 0 siblings, 1 reply; 9+ messages in thread From: Mathieu Poirier @ 2022-01-04 17:46 UTC (permalink / raw) To: Miaoqian Lin Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, linux-kernel Good morning, On Mon, Dec 27, 2021 at 09:06:45AM +0000, Miaoqian Lin wrote: > The debugfs_create_file() function doesn't return NULL. > It returns error pointers. You are correct. > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > --- > drivers/remoteproc/remoteproc_debugfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > index b5a1e3b697d9..a2409fe2f57b 100644 > --- a/drivers/remoteproc/remoteproc_debugfs.c > +++ b/drivers/remoteproc/remoteproc_debugfs.c > @@ -390,7 +390,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, > > tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, > &trace_rproc_ops); > - if (!tfile) { > + if (IS_ERR(tfile)) { > dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); > return NULL; Please return PTR_ERR(tfile) and fix rproc_handle_trace() to do the right error check and propagate the error code if needed. Thanks, Mathieu > } > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2022-01-04 17:46 ` Mathieu Poirier @ 2022-01-05 6:42 ` Miaoqian Lin 2022-01-05 13:10 ` [PATCH v3] " Miaoqian Lin 0 siblings, 1 reply; 9+ messages in thread From: Miaoqian Lin @ 2022-01-05 6:42 UTC (permalink / raw) To: mathieu.poirier Cc: bjorn.andersson, linmq006, linux-kernel, linux-remoteproc, ohad The debugfs_create_file() function doesn't return NULL. It returns error pointers. Fix check in rproc_create_trace_file and make it returns return error pointers. Fix check in rproc_handle_trace propagate the error code. Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- Changes in v2: - return PTR_ERR(tfile) in rproc_create_trace_file - fix check in rproc_handle_trace() --- drivers/remoteproc/remoteproc_core.c | 6 ++++-- drivers/remoteproc/remoteproc_debugfs.c | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 775df165eb45..5608408f8eac 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -656,6 +656,7 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, struct rproc_debug_trace *trace; struct device *dev = &rproc->dev; char name[15]; + int ret; if (sizeof(*rsc) > avail) { dev_err(dev, "trace rsc is truncated\n"); @@ -684,9 +685,10 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, /* create the debugfs entry */ trace->tfile = rproc_create_trace_file(name, rproc, trace); - if (!trace->tfile) { + if (IS_ERR(trace->tfile)) { + ret = PTR_ERR(trace->tfile); kfree(trace); - return -EINVAL; + return ret; } list_add_tail(&trace->node, &rproc->traces); diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c index b5a1e3b697d9..c1aa7272da1e 100644 --- a/drivers/remoteproc/remoteproc_debugfs.c +++ b/drivers/remoteproc/remoteproc_debugfs.c @@ -390,9 +390,9 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, &trace_rproc_ops); - if (!tfile) { + if (IS_ERR(tfile)) { dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); - return NULL; + return PTR_ERR(tfile); } return tfile; -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2022-01-05 6:42 ` [PATCH v2] " Miaoqian Lin @ 2022-01-05 13:10 ` Miaoqian Lin 2022-01-17 17:06 ` Mathieu Poirier 0 siblings, 1 reply; 9+ messages in thread From: Miaoqian Lin @ 2022-01-05 13:10 UTC (permalink / raw) To: linmq006 Cc: bjorn.andersson, linux-kernel, linux-remoteproc, mathieu.poirier, ohad The debugfs_create_file() function doesn't return NULL. It returns error pointers. Fix check in rproc_create_trace_file and make it returns return error pointers. Fix check in rproc_handle_trace to propagate the error code. Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- Changes in v2: - return PTR_ERR(tfile) in rproc_create_trace_file - fix check in rproc_handle_trace() Changes in v3: - return tfile to fix incorrect return type in v2 --- drivers/remoteproc/remoteproc_core.c | 6 ++++-- drivers/remoteproc/remoteproc_debugfs.c | 4 +--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 775df165eb45..5608408f8eac 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -656,6 +656,7 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, struct rproc_debug_trace *trace; struct device *dev = &rproc->dev; char name[15]; + int ret; if (sizeof(*rsc) > avail) { dev_err(dev, "trace rsc is truncated\n"); @@ -684,9 +685,10 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, /* create the debugfs entry */ trace->tfile = rproc_create_trace_file(name, rproc, trace); - if (!trace->tfile) { + if (IS_ERR(trace->tfile)) { + ret = PTR_ERR(trace->tfile); kfree(trace); - return -EINVAL; + return ret; } list_add_tail(&trace->node, &rproc->traces); diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c index b5a1e3b697d9..2ae59a365b7e 100644 --- a/drivers/remoteproc/remoteproc_debugfs.c +++ b/drivers/remoteproc/remoteproc_debugfs.c @@ -390,10 +390,8 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, &trace_rproc_ops); - if (!tfile) { + if (IS_ERR(tfile)) dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); - return NULL; - } return tfile; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2022-01-05 13:10 ` [PATCH v3] " Miaoqian Lin @ 2022-01-17 17:06 ` Mathieu Poirier 2022-01-17 22:31 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Mathieu Poirier @ 2022-01-17 17:06 UTC (permalink / raw) To: Miaoqian Lin; +Cc: bjorn.andersson, linux-kernel, linux-remoteproc, ohad On Wed, Jan 05, 2022 at 01:10:22PM +0000, Miaoqian Lin wrote: > The debugfs_create_file() function doesn't return NULL. > It returns error pointers. Fix check in rproc_create_trace_file > and make it returns return error pointers. s/"returns return"/return > Fix check in rproc_handle_trace to propagate the error code. > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > --- > Changes in v2: > - return PTR_ERR(tfile) in rproc_create_trace_file > - fix check in rproc_handle_trace() > Changes in v3: > - return tfile to fix incorrect return type in v2 > --- > drivers/remoteproc/remoteproc_core.c | 6 ++++-- > drivers/remoteproc/remoteproc_debugfs.c | 4 +--- > 2 files changed, 5 insertions(+), 5 deletions(-) > I will fix the above, add a proper "Fixes" tag and apply this patch to rproc-next when v5.17-rc1 comes out next week. Thanks, Mathieu > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 775df165eb45..5608408f8eac 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -656,6 +656,7 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > struct rproc_debug_trace *trace; > struct device *dev = &rproc->dev; > char name[15]; > + int ret; > > if (sizeof(*rsc) > avail) { > dev_err(dev, "trace rsc is truncated\n"); > @@ -684,9 +685,10 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > /* create the debugfs entry */ > trace->tfile = rproc_create_trace_file(name, rproc, trace); > - if (!trace->tfile) { > + if (IS_ERR(trace->tfile)) { > + ret = PTR_ERR(trace->tfile); > kfree(trace); > - return -EINVAL; > + return ret; > } > > list_add_tail(&trace->node, &rproc->traces); > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > index b5a1e3b697d9..2ae59a365b7e 100644 > --- a/drivers/remoteproc/remoteproc_debugfs.c > +++ b/drivers/remoteproc/remoteproc_debugfs.c > @@ -390,10 +390,8 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, > > tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, > &trace_rproc_ops); > - if (!tfile) { > + if (IS_ERR(tfile)) > dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); > - return NULL; > - } > > return tfile; > } > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2022-01-17 17:06 ` Mathieu Poirier @ 2022-01-17 22:31 ` Bjorn Andersson 2022-01-18 16:56 ` Mathieu Poirier 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2022-01-17 22:31 UTC (permalink / raw) To: Mathieu Poirier; +Cc: Miaoqian Lin, linux-kernel, linux-remoteproc, ohad On Mon 17 Jan 11:06 CST 2022, Mathieu Poirier wrote: > On Wed, Jan 05, 2022 at 01:10:22PM +0000, Miaoqian Lin wrote: > > The debugfs_create_file() function doesn't return NULL. > > It returns error pointers. Fix check in rproc_create_trace_file > > and make it returns return error pointers. > > s/"returns return"/return > > > Fix check in rproc_handle_trace to propagate the error code. > > > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > > --- > > Changes in v2: > > - return PTR_ERR(tfile) in rproc_create_trace_file > > - fix check in rproc_handle_trace() > > Changes in v3: > > - return tfile to fix incorrect return type in v2 > > --- > > drivers/remoteproc/remoteproc_core.c | 6 ++++-- > > drivers/remoteproc/remoteproc_debugfs.c | 4 +--- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > I will fix the above, add a proper "Fixes" tag and apply this patch to > rproc-next when v5.17-rc1 comes out next week. > We're actually not supposed to check debugfs_create_*() for errors. > Thanks, > Mathieu > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index 775df165eb45..5608408f8eac 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -656,6 +656,7 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > struct rproc_debug_trace *trace; > > struct device *dev = &rproc->dev; > > char name[15]; > > + int ret; > > > > if (sizeof(*rsc) > avail) { > > dev_err(dev, "trace rsc is truncated\n"); > > @@ -684,9 +685,10 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > > > /* create the debugfs entry */ > > trace->tfile = rproc_create_trace_file(name, rproc, trace); > > - if (!trace->tfile) { > > + if (IS_ERR(trace->tfile)) { > > + ret = PTR_ERR(trace->tfile); > > kfree(trace); > > - return -EINVAL; > > + return ret; And actually catching and propagating the error here means that we will start failing rproc_boot() for firmware including a RSC_TRACE when debugfs is disabled... So if we really want to save the heap space we should at least cleanly ignore the error, by cleaning up and returning 0 here. > > } > > > > list_add_tail(&trace->node, &rproc->traces); > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > > index b5a1e3b697d9..2ae59a365b7e 100644 > > --- a/drivers/remoteproc/remoteproc_debugfs.c > > +++ b/drivers/remoteproc/remoteproc_debugfs.c > > @@ -390,10 +390,8 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, > > > > tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, > > &trace_rproc_ops); > > - if (!tfile) { > > + if (IS_ERR(tfile)) > > dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); And I therefor think this function would be better reduced to: return debugfs_create_file(...); Regards, Bjorn > > - return NULL; > > - } > > > > return tfile; > > } > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2022-01-17 22:31 ` Bjorn Andersson @ 2022-01-18 16:56 ` Mathieu Poirier 2022-01-18 19:17 ` Bjorn Andersson 0 siblings, 1 reply; 9+ messages in thread From: Mathieu Poirier @ 2022-01-18 16:56 UTC (permalink / raw) To: Bjorn Andersson; +Cc: Miaoqian Lin, linux-kernel, linux-remoteproc, ohad On Mon, Jan 17, 2022 at 04:31:23PM -0600, Bjorn Andersson wrote: > On Mon 17 Jan 11:06 CST 2022, Mathieu Poirier wrote: > > > On Wed, Jan 05, 2022 at 01:10:22PM +0000, Miaoqian Lin wrote: > > > The debugfs_create_file() function doesn't return NULL. > > > It returns error pointers. Fix check in rproc_create_trace_file > > > and make it returns return error pointers. > > > > s/"returns return"/return > > > > > Fix check in rproc_handle_trace to propagate the error code. > > > > > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > > > --- > > > Changes in v2: > > > - return PTR_ERR(tfile) in rproc_create_trace_file > > > - fix check in rproc_handle_trace() > > > Changes in v3: > > > - return tfile to fix incorrect return type in v2 > > > --- > > > drivers/remoteproc/remoteproc_core.c | 6 ++++-- > > > drivers/remoteproc/remoteproc_debugfs.c | 4 +--- > > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > > > > I will fix the above, add a proper "Fixes" tag and apply this patch to > > rproc-next when v5.17-rc1 comes out next week. > > > > We're actually not supposed to check debugfs_create_*() for errors. I'm interested in knowing more about this - can you expand on the specifics or perharps provide a link? > > > Thanks, > > Mathieu > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > index 775df165eb45..5608408f8eac 100644 > > > --- a/drivers/remoteproc/remoteproc_core.c > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -656,6 +656,7 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > > struct rproc_debug_trace *trace; > > > struct device *dev = &rproc->dev; > > > char name[15]; > > > + int ret; > > > > > > if (sizeof(*rsc) > avail) { > > > dev_err(dev, "trace rsc is truncated\n"); > > > @@ -684,9 +685,10 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > > > > > /* create the debugfs entry */ > > > trace->tfile = rproc_create_trace_file(name, rproc, trace); > > > - if (!trace->tfile) { > > > + if (IS_ERR(trace->tfile)) { > > > + ret = PTR_ERR(trace->tfile); > > > kfree(trace); > > > - return -EINVAL; > > > + return ret; > > > And actually catching and propagating the error here means that we will > start failing rproc_boot() for firmware including a RSC_TRACE when > debugfs is disabled... > > So if we really want to save the heap space we should at least cleanly > ignore the error, by cleaning up and returning 0 here. Humm... To me the _intent_ of the upstream code has always been to propagate errors reported by rproc_create_trace_file(). The fact that is hasn't happen because of inappropriate error handling is something that should be corrected. That being said disabling debugfs is a common practice for production systems and I agree that handling such a condition by returning 0 when rproc_create_trace_file() returns -ENODEV is the right thing to do. Thanks, Mathieu > > > > } > > > > > > list_add_tail(&trace->node, &rproc->traces); > > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > > > index b5a1e3b697d9..2ae59a365b7e 100644 > > > --- a/drivers/remoteproc/remoteproc_debugfs.c > > > +++ b/drivers/remoteproc/remoteproc_debugfs.c > > > @@ -390,10 +390,8 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, > > > > > > tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, > > > &trace_rproc_ops); > > > - if (!tfile) { > > > + if (IS_ERR(tfile)) > > > dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); > > And I therefor think this function would be better reduced to: > > return debugfs_create_file(...); > > Regards, > Bjorn > > > > - return NULL; > > > - } > > > > > > return tfile; > > > } > > > -- > > > 2.17.1 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2022-01-18 16:56 ` Mathieu Poirier @ 2022-01-18 19:17 ` Bjorn Andersson 2022-01-20 20:05 ` Mathieu Poirier 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2022-01-18 19:17 UTC (permalink / raw) To: Mathieu Poirier; +Cc: Miaoqian Lin, linux-kernel, linux-remoteproc, ohad On Tue 18 Jan 10:56 CST 2022, Mathieu Poirier wrote: > On Mon, Jan 17, 2022 at 04:31:23PM -0600, Bjorn Andersson wrote: > > On Mon 17 Jan 11:06 CST 2022, Mathieu Poirier wrote: > > > > > On Wed, Jan 05, 2022 at 01:10:22PM +0000, Miaoqian Lin wrote: > > > > The debugfs_create_file() function doesn't return NULL. > > > > It returns error pointers. Fix check in rproc_create_trace_file > > > > and make it returns return error pointers. > > > > > > s/"returns return"/return > > > > > > > Fix check in rproc_handle_trace to propagate the error code. > > > > > > > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > > > > --- > > > > Changes in v2: > > > > - return PTR_ERR(tfile) in rproc_create_trace_file > > > > - fix check in rproc_handle_trace() > > > > Changes in v3: > > > > - return tfile to fix incorrect return type in v2 > > > > --- > > > > drivers/remoteproc/remoteproc_core.c | 6 ++++-- > > > > drivers/remoteproc/remoteproc_debugfs.c | 4 +--- > > > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > I will fix the above, add a proper "Fixes" tag and apply this patch to > > > rproc-next when v5.17-rc1 comes out next week. > > > > > > > We're actually not supposed to check debugfs_create_*() for errors. > > I'm interested in knowing more about this - can you expand on the specifics or > perharps provide a link? > I'm not able to find anything going into the reasoning behind it, but you can find lots of examples where Greg says that we shouldn't do this: $ git log --grep "no need to check return value of debugfs_create functions" E.g.: https://lore.kernel.org/r/20200818133701.462958-1-gregkh@linuxfoundation.org > > > > > Thanks, > > > Mathieu > > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > > index 775df165eb45..5608408f8eac 100644 > > > > --- a/drivers/remoteproc/remoteproc_core.c > > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > > @@ -656,6 +656,7 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > > > struct rproc_debug_trace *trace; > > > > struct device *dev = &rproc->dev; > > > > char name[15]; > > > > + int ret; > > > > > > > > if (sizeof(*rsc) > avail) { > > > > dev_err(dev, "trace rsc is truncated\n"); > > > > @@ -684,9 +685,10 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > > > > > > > /* create the debugfs entry */ > > > > trace->tfile = rproc_create_trace_file(name, rproc, trace); > > > > - if (!trace->tfile) { > > > > + if (IS_ERR(trace->tfile)) { > > > > + ret = PTR_ERR(trace->tfile); > > > > kfree(trace); > > > > - return -EINVAL; > > > > + return ret; > > > > > > And actually catching and propagating the error here means that we will > > start failing rproc_boot() for firmware including a RSC_TRACE when > > debugfs is disabled... > > > > So if we really want to save the heap space we should at least cleanly > > ignore the error, by cleaning up and returning 0 here. > > Humm... To me the _intent_ of the upstream code has always been to propagate > errors reported by rproc_create_trace_file(). The fact that is hasn't happen > because of inappropriate error handling is something that should be corrected. > I share that view, in general. I suspect that the idea with debugfs is that it's for debugging purposes and you don't want your remoteproc to stop working just because there might be an issue debugging it. > That being said disabling debugfs is a common practice for production systems > and I agree that handling such a condition by returning 0 when > rproc_create_trace_file() returns -ENODEV is the right thing to do. > Right, but even with debugfs enabled, do you want to prevent your remoteproc from booting just because the debugfs, for some reason, wasn't able to add the trace file? For me the question is if we should clean up the "trace" object or not, as this only relates to the debugfs file. Ignoring the error would imply that we just keep this memory allocated - which I'm fine with for the sake of avoiding the error handling. > Thanks, > Mathieu > > > > > > > } > > > > > > > > list_add_tail(&trace->node, &rproc->traces); > > > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > > > > index b5a1e3b697d9..2ae59a365b7e 100644 > > > > --- a/drivers/remoteproc/remoteproc_debugfs.c > > > > +++ b/drivers/remoteproc/remoteproc_debugfs.c > > > > @@ -390,10 +390,8 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, > > > > > > > > tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, > > > > &trace_rproc_ops); > > > > - if (!tfile) { > > > > + if (IS_ERR(tfile)) > > > > dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); > > > > And I therefor think this function would be better reduced to: > > > > return debugfs_create_file(...); > > Taking another look at the implementation of debugfs_create_file() this dev_err() should be removed, because there will already be a more useful error printed by debugfs_create_file(). Regards, Bjorn > > Regards, > > Bjorn > > > > > > - return NULL; > > > > - } > > > > > > > > return tfile; > > > > } > > > > -- > > > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file 2022-01-18 19:17 ` Bjorn Andersson @ 2022-01-20 20:05 ` Mathieu Poirier 0 siblings, 0 replies; 9+ messages in thread From: Mathieu Poirier @ 2022-01-20 20:05 UTC (permalink / raw) To: Bjorn Andersson; +Cc: Miaoqian Lin, linux-kernel, linux-remoteproc, ohad On Tue, Jan 18, 2022 at 01:17:50PM -0600, Bjorn Andersson wrote: > On Tue 18 Jan 10:56 CST 2022, Mathieu Poirier wrote: > > > On Mon, Jan 17, 2022 at 04:31:23PM -0600, Bjorn Andersson wrote: > > > On Mon 17 Jan 11:06 CST 2022, Mathieu Poirier wrote: > > > > > > > On Wed, Jan 05, 2022 at 01:10:22PM +0000, Miaoqian Lin wrote: > > > > > The debugfs_create_file() function doesn't return NULL. > > > > > It returns error pointers. Fix check in rproc_create_trace_file > > > > > and make it returns return error pointers. > > > > > > > > s/"returns return"/return > > > > > > > > > Fix check in rproc_handle_trace to propagate the error code. > > > > > > > > > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > > > > > --- > > > > > Changes in v2: > > > > > - return PTR_ERR(tfile) in rproc_create_trace_file > > > > > - fix check in rproc_handle_trace() > > > > > Changes in v3: > > > > > - return tfile to fix incorrect return type in v2 > > > > > --- > > > > > drivers/remoteproc/remoteproc_core.c | 6 ++++-- > > > > > drivers/remoteproc/remoteproc_debugfs.c | 4 +--- > > > > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > > > > I will fix the above, add a proper "Fixes" tag and apply this patch to > > > > rproc-next when v5.17-rc1 comes out next week. > > > > > > > > > > We're actually not supposed to check debugfs_create_*() for errors. > > > > I'm interested in knowing more about this - can you expand on the specifics or > > perharps provide a link? > > > > I'm not able to find anything going into the reasoning behind it, but > you can find lots of examples where Greg says that we shouldn't do this: > > $ git log --grep "no need to check return value of debugfs_create functions" > > E.g.: > https://lore.kernel.org/r/20200818133701.462958-1-gregkh@linuxfoundation.org Greg's changelog leaves little doubt, thanks for pointing that out. > > > > > > > > Thanks, > > > > Mathieu > > > > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > > > index 775df165eb45..5608408f8eac 100644 > > > > > --- a/drivers/remoteproc/remoteproc_core.c > > > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > > > @@ -656,6 +656,7 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > > > > struct rproc_debug_trace *trace; > > > > > struct device *dev = &rproc->dev; > > > > > char name[15]; > > > > > + int ret; > > > > > > > > > > if (sizeof(*rsc) > avail) { > > > > > dev_err(dev, "trace rsc is truncated\n"); > > > > > @@ -684,9 +685,10 @@ static int rproc_handle_trace(struct rproc *rproc, void *ptr, > > > > > > > > > > /* create the debugfs entry */ > > > > > trace->tfile = rproc_create_trace_file(name, rproc, trace); > > > > > - if (!trace->tfile) { > > > > > + if (IS_ERR(trace->tfile)) { > > > > > + ret = PTR_ERR(trace->tfile); > > > > > kfree(trace); > > > > > - return -EINVAL; > > > > > + return ret; > > > > > > > > > And actually catching and propagating the error here means that we will > > > start failing rproc_boot() for firmware including a RSC_TRACE when > > > debugfs is disabled... > > > > > > So if we really want to save the heap space we should at least cleanly > > > ignore the error, by cleaning up and returning 0 here. > > > > Humm... To me the _intent_ of the upstream code has always been to propagate > > errors reported by rproc_create_trace_file(). The fact that is hasn't happen > > because of inappropriate error handling is something that should be corrected. > > > > I share that view, in general. I suspect that the idea with debugfs is > that it's for debugging purposes and you don't want your remoteproc to > stop working just because there might be an issue debugging it. > > > That being said disabling debugfs is a common practice for production systems > > and I agree that handling such a condition by returning 0 when > > rproc_create_trace_file() returns -ENODEV is the right thing to do. > > > > Right, but even with debugfs enabled, do you want to prevent your > remoteproc from booting just because the debugfs, for some reason, > wasn't able to add the trace file? Greg's comments brings a different angle to the conversation - I agree there is no need to dwell on debugfs error conditions. > > For me the question is if we should clean up the "trace" object or not, > as this only relates to the debugfs file. Ignoring the error would imply > that we just keep this memory allocated - which I'm fine with for the > sake of avoiding the error handling. I would also be fine with not cleaning up the trace object, it will be free'd as part of the resource cleanup process anyway. > > > Thanks, > > Mathieu > > > > > > > > > > } > > > > > > > > > > list_add_tail(&trace->node, &rproc->traces); > > > > > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > > > > > index b5a1e3b697d9..2ae59a365b7e 100644 > > > > > --- a/drivers/remoteproc/remoteproc_debugfs.c > > > > > +++ b/drivers/remoteproc/remoteproc_debugfs.c > > > > > @@ -390,10 +390,8 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc, > > > > > > > > > > tfile = debugfs_create_file(name, 0400, rproc->dbg_dir, trace, > > > > > &trace_rproc_ops); > > > > > - if (!tfile) { > > > > > + if (IS_ERR(tfile)) > > > > > dev_err(&rproc->dev, "failed to create debugfs trace entry\n"); > > > > > > And I therefor think this function would be better reduced to: > > > > > > return debugfs_create_file(...); > > > > > Taking another look at the implementation of debugfs_create_file() this > dev_err() should be removed, because there will already be a more useful > error printed by debugfs_create_file(). > > Regards, > Bjorn > > > > Regards, > > > Bjorn > > > > > > > > - return NULL; > > > > > - } > > > > > > > > > > return tfile; > > > > > } > > > > > -- > > > > > 2.17.1 > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-20 20:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-27 9:06 [PATCH] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file Miaoqian Lin 2022-01-04 17:46 ` Mathieu Poirier 2022-01-05 6:42 ` [PATCH v2] " Miaoqian Lin 2022-01-05 13:10 ` [PATCH v3] " Miaoqian Lin 2022-01-17 17:06 ` Mathieu Poirier 2022-01-17 22:31 ` Bjorn Andersson 2022-01-18 16:56 ` Mathieu Poirier 2022-01-18 19:17 ` Bjorn Andersson 2022-01-20 20:05 ` Mathieu Poirier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.