Re: [PATCH 2/7] remoteproc: use a local copy for the name field

From: Suman Anna <s-anna@ti.com>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Lokesh Vutla <lokeshvutla@ti.com>,
	linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/7] remoteproc: use a local copy for the name field
Date: Thu, 26 Mar 2020 15:35:38 -0500	[thread overview]
Message-ID: <3c38485d-5bae-a759-833c-45ddb0e6cfcd@ti.com> (raw)
In-Reply-To: <20200326194304.GB59436@builder>

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