| Message ID | 20220105131022.25247-1-linmq006@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] remoteproc: Fix NULL vs IS_ERR() checking in rproc_create_trace_file | expand |
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 >
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 > >
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 > > >
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 > > > >
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 > > > > >
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; }
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(-)