| Message ID | 20200415204858.2448-5-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: Refactor function rproc_alloc() | expand |
On 4/15/20 3:48 PM, Mathieu Poirier wrote: > For cases where @firmware is declared "const char *", use function > kstrdup_const() to avoid needlessly creating another copy on the > heap. > > Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Looks good. Reviewed-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 4 ++-- > include/linux/remoteproc.h | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 9899467fa1cf..ebaff496ef81 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = { > static int rproc_alloc_firmware(struct rproc *rproc, > const char *name, const char *firmware) > { > - char *p; > + const char *p; > > if (!firmware) > /* > @@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc, > */ > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); > else > - p = kstrdup(firmware, GFP_KERNEL); > + p = kstrdup_const(firmware, GFP_KERNEL); > > if (!p) > return -ENOMEM; > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 9c07d7958c53..38607107b7cb 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -489,7 +489,7 @@ struct rproc { > struct list_head node; > struct iommu_domain *domain; > const char *name; > - char *firmware; > + const char *firmware; > void *priv; > struct rproc_ops *ops; > struct device dev; >
On 4/15/20 4:25 PM, Alex Elder wrote: > On 4/15/20 3:48 PM, Mathieu Poirier wrote: >> For cases where @firmware is declared "const char *", use function >> kstrdup_const() to avoid needlessly creating another copy on the >> heap. >> >> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Looks good. > > Reviewed-by: Alex Elder <elder@linaro.org> > >> --- >> drivers/remoteproc/remoteproc_core.c | 4 ++-- >> include/linux/remoteproc.h | 2 +- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 9899467fa1cf..ebaff496ef81 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = { >> static int rproc_alloc_firmware(struct rproc *rproc, >> const char *name, const char *firmware) >> { >> - char *p; >> + const char *p; >> >> if (!firmware) >> /* >> @@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc, >> */ >> p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); So, to be consistent for both paths, should we be using kvasprintf_const() here and kfree_const() in release. The kfree_const() is needed to account for the kstrdup_const below for sure. regards Suman >> else >> - p = kstrdup(firmware, GFP_KERNEL); >> + p = kstrdup_const(firmware, GFP_KERNEL); >> >> if (!p) >> return -ENOMEM; >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index 9c07d7958c53..38607107b7cb 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -489,7 +489,7 @@ struct rproc { >> struct list_head node; >> struct iommu_domain *domain; >> const char *name; >> - char *firmware; >> + const char *firmware; >> void *priv; >> struct rproc_ops *ops; >> struct device dev; >> >
… > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = { … > - p = kstrdup(firmware, GFP_KERNEL); > + p = kstrdup_const(firmware, GFP_KERNEL); How do you think about to avoid a typo for a function name in the final commit subject? Regards, Markus
On Fri 17 Apr 06:44 PDT 2020, Suman Anna wrote: > On 4/15/20 4:25 PM, Alex Elder wrote: > > On 4/15/20 3:48 PM, Mathieu Poirier wrote: > > > For cases where @firmware is declared "const char *", use function > > > kstrdup_const() to avoid needlessly creating another copy on the > > > heap. > > > > > > Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > Looks good. > > > > Reviewed-by: Alex Elder <elder@linaro.org> > > > > > --- > > > drivers/remoteproc/remoteproc_core.c | 4 ++-- > > > include/linux/remoteproc.h | 2 +- > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > index 9899467fa1cf..ebaff496ef81 100644 > > > --- a/drivers/remoteproc/remoteproc_core.c > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = { > > > static int rproc_alloc_firmware(struct rproc *rproc, > > > const char *name, const char *firmware) > > > { > > > - char *p; > > > + const char *p; > > > if (!firmware) > > > /* > > > @@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc, > > > */ > > > p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); > > So, to be consistent for both paths, should we be using kvasprintf_const() > here and kfree_const() in release. Given that the second argument is a "proper" format string kvasprintf_const() is really just kasprintf() - but with the requirement that we set up a va_list. So I prefer that we stick with this. > The kfree_const() is needed to account > for the kstrdup_const below for sure. > You are correct Suman, this patch needs to also change the kfree() to a kfree_const() or bad things will happen after a visit to rproc_type_release(). Regards, Bjorn > regards > Suman > > > > else > > > - p = kstrdup(firmware, GFP_KERNEL); > > > + p = kstrdup_const(firmware, GFP_KERNEL); > > > if (!p) > > > return -ENOMEM; > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > > > index 9c07d7958c53..38607107b7cb 100644 > > > --- a/include/linux/remoteproc.h > > > +++ b/include/linux/remoteproc.h > > > @@ -489,7 +489,7 @@ struct rproc { > > > struct list_head node; > > > struct iommu_domain *domain; > > > const char *name; > > > - char *firmware; > > > + const char *firmware; > > > void *priv; > > > struct rproc_ops *ops; > > > struct device dev; > > > > > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 9899467fa1cf..ebaff496ef81 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1982,7 +1982,7 @@ static const struct device_type rproc_type = { static int rproc_alloc_firmware(struct rproc *rproc, const char *name, const char *firmware) { - char *p; + const char *p; if (!firmware) /* @@ -1991,7 +1991,7 @@ static int rproc_alloc_firmware(struct rproc *rproc, */ p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name); else - p = kstrdup(firmware, GFP_KERNEL); + p = kstrdup_const(firmware, GFP_KERNEL); if (!p) return -ENOMEM; diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index 9c07d7958c53..38607107b7cb 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -489,7 +489,7 @@ struct rproc { struct list_head node; struct iommu_domain *domain; const char *name; - char *firmware; + const char *firmware; void *priv; struct rproc_ops *ops; struct device dev;
For cases where @firmware is declared "const char *", use function kstrdup_const() to avoid needlessly creating another copy on the heap. Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 4 ++-- include/linux/remoteproc.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)