| Message ID | 20200324201819.23095-3-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TI K3 R5F remoteproc support | expand |
On Tue 24 Mar 13:18 PDT 2020, Suman Anna wrote: > The current name field used in the remoteproc structure is simply > a pointer to a name field supplied during the rproc_alloc() call. > The pointer passed in by remoteproc drivers during registration is > typically a dev_name pointer, but it is possible that the pointer > will no longer remain valid if the devices themselves were created > at runtime like in the case of of_platform_populate(), and were > deleted upon any failures within the respective remoteproc driver > probe function. > > So, allocate and maintain a local copy for this name field to > keep it agnostic of the logic used in the remoteproc drivers. > > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > drivers/remoteproc/remoteproc_core.c | 9 ++++++++- > include/linux/remoteproc.h | 2 +- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index aca6d022901a..6e0b91fa6f11 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1989,6 +1989,7 @@ static void rproc_type_release(struct device *dev) > > kfree(rproc->firmware); > kfree(rproc->ops); > + kfree(rproc->name); > kfree(rproc); > } > > @@ -2061,7 +2062,13 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > } > > rproc->firmware = p; > - rproc->name = name; > + rproc->name = kstrdup(name, GFP_KERNEL); Let's use kstrdup_const() instead here (and kfree_const() instead of kfree()), so that the cases where we are passed a constant we won't create a duplicate on the heap. And the "name" in struct rproc can remain const. > + if (!rproc->name) { > + kfree(p); > + kfree(rproc->ops); > + kfree(rproc); > + return NULL; Perhaps we can rearrange the hunks here slightly and get to a point where we can rely on the release function earlier? Regards, Bjorn > + } > rproc->priv = &rproc[1]; > rproc->auto_boot = true; > rproc->elf_class = ELFCLASS32; > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index ddce7a7775d1..77788a4bb94e 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -490,7 +490,7 @@ struct rproc_dump_segment { > struct rproc { > struct list_head node; > struct iommu_domain *domain; > - const char *name; > + char *name; > char *firmware; > void *priv; > struct rproc_ops *ops; > -- > 2.23.0 >
Hi Bjorn, On 3/26/20 12:42 AM, Bjorn Andersson wrote: > On Tue 24 Mar 13:18 PDT 2020, Suman Anna wrote: > >> The current name field used in the remoteproc structure is simply >> a pointer to a name field supplied during the rproc_alloc() call. >> The pointer passed in by remoteproc drivers during registration is >> typically a dev_name pointer, but it is possible that the pointer >> will no longer remain valid if the devices themselves were created >> at runtime like in the case of of_platform_populate(), and were >> deleted upon any failures within the respective remoteproc driver >> probe function. >> >> So, allocate and maintain a local copy for this name field to >> keep it agnostic of the logic used in the remoteproc drivers. >> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> --- >> drivers/remoteproc/remoteproc_core.c | 9 ++++++++- >> include/linux/remoteproc.h | 2 +- >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index aca6d022901a..6e0b91fa6f11 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1989,6 +1989,7 @@ static void rproc_type_release(struct device *dev) >> >> kfree(rproc->firmware); >> kfree(rproc->ops); >> + kfree(rproc->name); >> kfree(rproc); >> } >> >> @@ -2061,7 +2062,13 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, >> } >> >> rproc->firmware = p; >> - rproc->name = name; >> + rproc->name = kstrdup(name, GFP_KERNEL); > > Let's use kstrdup_const() instead here (and kfree_const() instead of > kfree()), so that the cases where we are passed a constant we won't > create a duplicate on the heap. > > And the "name" in struct rproc can remain const. Agreed, that's better functions to use for this. > >> + if (!rproc->name) { >> + kfree(p); >> + kfree(rproc->ops); >> + kfree(rproc); >> + return NULL; > > Perhaps we can rearrange the hunks here slightly and get to a point > where we can rely on the release function earlier? Not sure I understand. I don't see any release function, all failure paths in rproc_alloc() directly unwind the previous operations. You mean move this to before the alloc for rproc structure, something similar to what we are doing with firmware? regards Suman > > Regards, > Bjorn > >> + } >> rproc->priv = &rproc[1]; >> rproc->auto_boot = true; >> rproc->elf_class = ELFCLASS32; >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index ddce7a7775d1..77788a4bb94e 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -490,7 +490,7 @@ struct rproc_dump_segment { >> struct rproc { >> struct list_head node; >> struct iommu_domain *domain; >> - const char *name; >> + char *name; >> char *firmware; >> void *priv; >> struct rproc_ops *ops; >> -- >> 2.23.0 >>
On Thu 26 Mar 07:01 PDT 2020, Suman Anna wrote: > Hi Bjorn, > > On 3/26/20 12:42 AM, Bjorn Andersson wrote: > > On Tue 24 Mar 13:18 PDT 2020, Suman Anna wrote: > > > >> The current name field used in the remoteproc structure is simply > >> a pointer to a name field supplied during the rproc_alloc() call. > >> The pointer passed in by remoteproc drivers during registration is > >> typically a dev_name pointer, but it is possible that the pointer > >> will no longer remain valid if the devices themselves were created > >> at runtime like in the case of of_platform_populate(), and were > >> deleted upon any failures within the respective remoteproc driver > >> probe function. > >> > >> So, allocate and maintain a local copy for this name field to > >> keep it agnostic of the logic used in the remoteproc drivers. > >> > >> Signed-off-by: Suman Anna <s-anna@ti.com> > >> --- > >> drivers/remoteproc/remoteproc_core.c | 9 ++++++++- > >> include/linux/remoteproc.h | 2 +- > >> 2 files changed, 9 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > >> index aca6d022901a..6e0b91fa6f11 100644 > >> --- a/drivers/remoteproc/remoteproc_core.c > >> +++ b/drivers/remoteproc/remoteproc_core.c > >> @@ -1989,6 +1989,7 @@ static void rproc_type_release(struct device *dev) > >> > >> kfree(rproc->firmware); > >> kfree(rproc->ops); > >> + kfree(rproc->name); > >> kfree(rproc); > >> } > >> > >> @@ -2061,7 +2062,13 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > >> } > >> > >> rproc->firmware = p; > >> - rproc->name = name; > >> + rproc->name = kstrdup(name, GFP_KERNEL); > > > > Let's use kstrdup_const() instead here (and kfree_const() instead of > > kfree()), so that the cases where we are passed a constant we won't > > create a duplicate on the heap. > > > > And the "name" in struct rproc can remain const. > > Agreed, that's better functions to use for this. > > > > >> + if (!rproc->name) { > >> + kfree(p); > >> + kfree(rproc->ops); > >> + kfree(rproc); > >> + return NULL; > > > > Perhaps we can rearrange the hunks here slightly and get to a point > > where we can rely on the release function earlier? > > Not sure I understand. I don't see any release function, all failure > paths in rproc_alloc() directly unwind the previous operations. You mean > move this to before the alloc for rproc structure, something similar to > what we are doing with firmware? > Look at the failure for ida_simple_get(), there we're past the setup of rproc->dev.type, so the rproc_type->release function will be invoked as we call put_device(). So if you move the initialization of rproc->dev up right after the allocation of rproc we should be able to rely on that to clean up all these for us. Regards, Bjorn > regards > Suman > > > > > > Regards, > > Bjorn > > > >> + } > >> rproc->priv = &rproc[1]; > >> rproc->auto_boot = true; > >> rproc->elf_class = ELFCLASS32; > >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > >> index ddce7a7775d1..77788a4bb94e 100644 > >> --- a/include/linux/remoteproc.h > >> +++ b/include/linux/remoteproc.h > >> @@ -490,7 +490,7 @@ struct rproc_dump_segment { > >> struct rproc { > >> struct list_head node; > >> struct iommu_domain *domain; > >> - const char *name; > >> + char *name; > >> char *firmware; > >> void *priv; > >> struct rproc_ops *ops; > >> -- > >> 2.23.0 > >> >
On 3/26/20 2:43 PM, Bjorn Andersson wrote: > On Thu 26 Mar 07:01 PDT 2020, Suman Anna wrote: > >> Hi Bjorn, >> >> On 3/26/20 12:42 AM, Bjorn Andersson wrote: >>> On Tue 24 Mar 13:18 PDT 2020, Suman Anna wrote: >>> >>>> The current name field used in the remoteproc structure is simply >>>> a pointer to a name field supplied during the rproc_alloc() call. >>>> The pointer passed in by remoteproc drivers during registration is >>>> typically a dev_name pointer, but it is possible that the pointer >>>> will no longer remain valid if the devices themselves were created >>>> at runtime like in the case of of_platform_populate(), and were >>>> deleted upon any failures within the respective remoteproc driver >>>> probe function. >>>> >>>> So, allocate and maintain a local copy for this name field to >>>> keep it agnostic of the logic used in the remoteproc drivers. >>>> >>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>> --- >>>> drivers/remoteproc/remoteproc_core.c | 9 ++++++++- >>>> include/linux/remoteproc.h | 2 +- >>>> 2 files changed, 9 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >>>> index aca6d022901a..6e0b91fa6f11 100644 >>>> --- a/drivers/remoteproc/remoteproc_core.c >>>> +++ b/drivers/remoteproc/remoteproc_core.c >>>> @@ -1989,6 +1989,7 @@ static void rproc_type_release(struct device *dev) >>>> >>>> kfree(rproc->firmware); >>>> kfree(rproc->ops); >>>> + kfree(rproc->name); >>>> kfree(rproc); >>>> } >>>> >>>> @@ -2061,7 +2062,13 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, >>>> } >>>> >>>> rproc->firmware = p; >>>> - rproc->name = name; >>>> + rproc->name = kstrdup(name, GFP_KERNEL); >>> >>> Let's use kstrdup_const() instead here (and kfree_const() instead of >>> kfree()), so that the cases where we are passed a constant we won't >>> create a duplicate on the heap. >>> >>> And the "name" in struct rproc can remain const. >> >> Agreed, that's better functions to use for this. >> >>> >>>> + if (!rproc->name) { >>>> + kfree(p); >>>> + kfree(rproc->ops); >>>> + kfree(rproc); >>>> + return NULL; >>> >>> Perhaps we can rearrange the hunks here slightly and get to a point >>> where we can rely on the release function earlier? >> >> Not sure I understand. I don't see any release function, all failure >> paths in rproc_alloc() directly unwind the previous operations. You mean >> move this to before the alloc for rproc structure, something similar to >> what we are doing with firmware? >> > > Look at the failure for ida_simple_get(), there we're past the setup of > rproc->dev.type, so the rproc_type->release function will be invoked as > we call put_device(). > > So if you move the initialization of rproc->dev up right after the > allocation of rproc we should be able to rely on that to clean up all > these for us. Yeah ok. That's cleanup though, and probably a patch of its own, and not directly related to the subject of this patch. Yeah, I can rework this patch to sit on top of that cleanup patch. regards Suman > > Regards, > Bjorn > >> regards >> Suman >> >> >>> >>> Regards, >>> Bjorn >>> >>>> + } >>>> rproc->priv = &rproc[1]; >>>> rproc->auto_boot = true; >>>> rproc->elf_class = ELFCLASS32; >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>>> index ddce7a7775d1..77788a4bb94e 100644 >>>> --- a/include/linux/remoteproc.h >>>> +++ b/include/linux/remoteproc.h >>>> @@ -490,7 +490,7 @@ struct rproc_dump_segment { >>>> struct rproc { >>>> struct list_head node; >>>> struct iommu_domain *domain; >>>> - const char *name; >>>> + char *name; >>>> char *firmware; >>>> void *priv; >>>> struct rproc_ops *ops; >>>> -- >>>> 2.23.0 >>>> >>
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index aca6d022901a..6e0b91fa6f11 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1989,6 +1989,7 @@ static void rproc_type_release(struct device *dev) kfree(rproc->firmware); kfree(rproc->ops); + kfree(rproc->name); kfree(rproc); } @@ -2061,7 +2062,13 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, } rproc->firmware = p; - rproc->name = name; + rproc->name = kstrdup(name, GFP_KERNEL); + if (!rproc->name) { + kfree(p); + kfree(rproc->ops); + kfree(rproc); + return NULL; + } rproc->priv = &rproc[1]; rproc->auto_boot = true; rproc->elf_class = ELFCLASS32; diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index ddce7a7775d1..77788a4bb94e 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -490,7 +490,7 @@ struct rproc_dump_segment { struct rproc { struct list_head node; struct iommu_domain *domain; - const char *name; + char *name; char *firmware; void *priv; struct rproc_ops *ops;
The current name field used in the remoteproc structure is simply a pointer to a name field supplied during the rproc_alloc() call. The pointer passed in by remoteproc drivers during registration is typically a dev_name pointer, but it is possible that the pointer will no longer remain valid if the devices themselves were created at runtime like in the case of of_platform_populate(), and were deleted upon any failures within the respective remoteproc driver probe function. So, allocate and maintain a local copy for this name field to keep it agnostic of the logic used in the remoteproc drivers. Signed-off-by: Suman Anna <s-anna@ti.com> --- drivers/remoteproc/remoteproc_core.c | 9 ++++++++- include/linux/remoteproc.h | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-)