Re: [PATCH 2/2] remoteproc: Fix and restore the parenting hierarchy for vdev

[Arnaud POULIQUEN]

hi Suman,

On 3/18/20 8:23 PM, Suman Anna wrote:
> Hi Arnaud,
> 
> On 3/18/20 11:38 AM, Arnaud POULIQUEN wrote:
>> Hi Suman, Mathieu,
>>
>> On 3/17/20 7:05 PM, Mathieu Poirier wrote:
>>> Hi Suman,
>>>
>>> On Thu, Mar 05, 2020 at 04:41:08PM -0600, Suman Anna wrote:
>>>> The commit 086d08725d34 ("remoteproc: create vdev subdevice with specific
>>>> dma memory pool") has introduced a new vdev subdevice for each vdev
>>>> declared in the firmware resource table and made it as the parent for the
>>>> created virtio rpmsg devices instead of the previous remoteproc device.
>>>> This changed the overall parenting hierarchy for the rpmsg devices, which
>>>> were children of virtio devices, and does not allow the corresponding
>>>> rpmsg drivers to retrieve the parent rproc device through the
>>>> rproc_get_by_child() API.
>>>>
>>>> Fix this by restoring the remoteproc device as the parent. The new vdev
>>>> subdevice can continue to inherit the DMA attributes from the remoteproc's
>>>> parent device (actual platform device).
>>>>
>>>> Fixes: 086d08725d34 ("remoteproc: create vdev subdevice with specific dma memory pool")
>>>> Signed-off-by: Suman Anna <s-anna@xxxxxx>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>> index 097f33e4f1f3..ba18f32bd0c4 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -510,7 +510,7 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>>>>  
>>>>  	/* Initialise vdev subdevice */
>>>>  	snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
>>>> -	rvdev->dev.parent = rproc->dev.parent;
>>>> +	rvdev->dev.parent = &rproc->dev;
>>>
>>> I can see how it would not be possible to retrieve the parent rproc device since
>>> rvdev->dev.parent was set to be platform device...
>>
>> In rpmsg_virtio_bus.c [1] the vdev buffers are allocated in a memory region using a dma_alloc_coherent
>> So the buffers are allocated in the platform driver memory region if declared, else in the default memory region. 
>>
>> According to  086d08725d34 ("remoteproc: create vdev subdevice with specific dma memory pool"),
>> A patch has been integrated in rpmsg framework:  d999b622fcfb3 ("rpmsg: virtio: allocate buffer from parent")
>>
>> -	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
>> +	bufs_va = dma_alloc_coherent(vdev->dev.parent,
>>
>> So in term of parent-child relationchip the Loic's patches seem coherent, and don't affect parenting hierarchy
>> for the rpmsg bus.
> 
> So, there are two things w.r.t rpmsg device hierarchy - buffer
> allocations and the overall hierarchy to allow a child rpmsg device to
> be able to retrieve the corresponding rproc. This is done using
> rproc_get_by_child() which walks up the dev parent hierarchy and
> matching the parent device type to rproc_type.
> 
> Commit 086d08725d34 adds a new vdevbuffer device with parent as the
> rproc platform device and makes this the parent of the virtio device, so
> the buffer allocations were unchanged just with that commit, but the
> rproc lookup will always fail. The later commit d999b622fcfb3 switches
> the buffer allocation over to the vdevbuffer device, and with rproc
> drivers that added dedicated vdevbuf pools allocates from those pools
> (these were mostly coming from a specific rproc platform device memory
> region index anyway). For those that did not define, this actually
> became the global pool even if the rproc device was using a single
> DMA/CMA pool (patch 1).
> 
> Please see my cover-letter for an example of the dev hierarchy.
You are right allocation is not affected by your patch.
It is still done in the memory pool attached to rvdev.
This is the main point i missed. I reviewed the whole parent hierarchy to better understand why your patch
does not affect the memory allocation.
Now it clear to me, sorry for my misunderstanding of your point.

Acked-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>

> 
>>
>> So It seems to me that this patch breaks the relationship between the rpmsg bus
>> and the rproc platform driver, at least concerning the buffer allocation.
> 
> I am not sure if you were interpreting this patch with or without
> d999b622fcfb3 ("rpmsg: virtio: allocate buffer from parent"). Both of
> the above commits are in 5.1, so I consider this patch to be fixing only
> on 5.1+ kernels and it does use d999b622fcfb3. Buffer allocations after
> this patch without d999b622fcfb3 would try to allocate from rproc device
> which is a pseudo-device and doesn't have any pools defined with it, so
> will allocate from the global pool.
> 
>> But on the other side this patch doesn't introduce regression for rpmsg bus on my platform... 
>>
>> I probably missed something important because i can not figure out how this patch don't introduce regression.
>> Can the rproc->dev inherits from the rproc platform device in term of memory regions?
>>
>> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/virtio_rpmsg_bus.c#L915
>> [2]: https://lkml.org/lkml/2018/11/30/180
>>
>>>
>>> I wonder how the original change didn't blow up sysmon_probe() and potentially
>>> other out-of-tree users of rproc_get_by_child().  It would be nice to have
>>> someone from the QCOM team test your patch.
>>
>> You are right the rproc platform device is now the grand parent of a rpmsg device, while before it was the parent.
>> Anyway, does it makes sense to have this kind of dependency between rpmsg device and rproc device?
>> The fix could be done in the rpmsg device that would be rproc dependent (if out-of-tree).
> 
> Not sure what you are proposing here, since you cannot retrieve a rproc
> handle. We use this to perform address translations in rpmsg drivers
> since all the addresses are with the rproc device.
>
>> The alternative could be to declare the rpmsg device in device tree as child of the rproc platform device...
> 
> And that is completely orthogonal and doesn't solve the current scenario
> where rpmsg devices are created through the virtio-rpmsg bus nameservice
> announcement.
Yes it another discussion. The only thing i would like to highlight here (for future discussion) is the relation chip
you create between rpmsg and remoteproc using rproc_get_by_child. 
Would be nice to have an abstraction layer for memory allocation and translation for generic RPMsg devices decorrelated
from remoteproc.
No more the topic here...

Thanks,
Arnaud

> 
> regards
> Suman
> 
> 
>>
>> Regards,
>> Arnaud
>>
>>>
>>>>  	rvdev->dev.dma_pfn_offset = rproc->dev.parent->dma_pfn_offset;
>>>>  	rvdev->dev.release = rproc_rvdev_release;
>>>>  	dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
>>>
>>> Be mindful there might be fallouts from applying this patch since it does change
>>> the location of the vdev under /sys/device/platform/ .
>>>
>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>>>
>>>> -- 
>>>> 2.23.0
>>>>
>