On Thu 26 Mar 07:01 PDT 2020, Suman Anna wrote:

Hi Bjorn,

On 3/26/20 12:42 AM, Bjorn Andersson wrote:

quoted

On Tue 24 Mar 13:18 PDT 2020, Suman Anna wrote:

quoted

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 <redacted>
---
 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.

quoted

quoted

+	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

quoted

Regards,
Bjorn

quoted

+	}
 	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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel