Re: [PATCH virtio] virtio: virtio_console: fix DMA memory allocation for rproc serial

[Alexander Lobakin]

From: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
Date: Mon, 16 Nov 2020 07:25:31 -0500

> On Mon, Nov 16, 2020 at 09:19:50AM +0000, Christoph Hellwig wrote:
>> I just noticed this showing up in Linus' tree and I'm not happy.
>
> Are you sure? I think it's in next.

Nope, it goes to fixes since it just fixes the regression introduced
in 5.7.
That's why it's not about any refactoring or rethinking the whole
model.

>> This whole model of the DMA subdevices in remoteproc is simply broken.
>>
>> We really need to change the virtio code pass an expicit DMA device (
>> similar to what e.g. the USB and RDMA code does), instead of faking up
>> devices with broken adhoc inheritance of DMA properties and magic poking
>> into device parent relationships.

But lots of subsystems like netdev for example uses dev->parent for
DMA operations. I know that their pointers go directly to the
platform/PCI/etc. device, but still.

The only reason behind "fake" DMA devices for rproc is to be able to
reserve DMA memory through the Device Tree exclusively for only one
virtio dev like virtio_console or virtio_rpmsg_bus. That's why
they are present, are coercing DMA caps from physical dev
representor, and why questinable dma_declare_coherent_memory()
is still here and doesn't allow to build rproc core as a module.
I agree that this is not the best model obviously, and we should take
a look at it.

> OK but we do have a regression since 5.7 and this looks like
> a fix appropriate for e.g. stable, right?
>
>> Bjorn, I thought you were going to look into this a while ago?
>>
>>
>> On Wed, Nov 04, 2020 at 03:31:36PM +0000, Alexander Lobakin wrote:
>>> Since commit 086d08725d34 ("remoteproc: create vdev subdevice with
>>> specific dma memory pool"), every remoteproc has a DMA subdevice
>>> ("remoteprocX#vdevYbuffer") for each virtio device, which inherits
>>> DMA capabilities from the corresponding platform device. This allowed
>>> to associate different DMA pools with each vdev, and required from
>>> virtio drivers to perform DMA operations with the parent device
>>> (vdev->dev.parent) instead of grandparent (vdev->dev.parent->parent).
>>>
>>> virtio_rpmsg_bus was already changed in the same merge cycle with
>>> commit d999b622fcfb ("rpmsg: virtio: allocate buffer from parent"),
>>> but virtio_console did not. In fact, operations using the grandparent
>>> worked fine while the grandparent was the platform device, but since
>>> commit c774ad010873 ("remoteproc: Fix and restore the parenting
>>> hierarchy for vdev") this was changed, and now the grandparent device
>>> is the remoteproc device without any DMA capabilities.
>>> So, starting v5.8-rc1 the following warning is observed:
>>>
>>> [    2.483925] ------------[ cut here ]------------
>>> [    2.489148] WARNING: CPU: 3 PID: 101 at kernel/dma/mapping.c:427 0x80e7eee8
>>> [    2.489152] Modules linked in: virtio_console(+)
>>> [    2.503737]  virtio_rpmsg_bus rpmsg_core
>>> [    2.508903]
>>> [    2.528898] <Other modules, stack and call trace here>
>>> [    2.913043]
>>> [    2.914907] ---[ end trace 93ac8746beab612c ]---
>>> [    2.920102] virtio-ports vport1p0: Error allocating inbufs
>>>
>>> kernel/dma/mapping.c:427 is:
>>>
>>> WARN_ON_ONCE(!dev->coherent_dma_mask);
>>>
>>> obviously because the grandparent now is remoteproc dev without any
>>> DMA caps:
>>>
>>> [    3.104943] Parent: remoteproc0#vdev1buffer, grandparent: remoteproc0
>>>
>>> Fix this the same way as it was for virtio_rpmsg_bus, using just the
>>> parent device (vdev->dev.parent, "remoteprocX#vdevYbuffer") for DMA
>>> operations.
>>> This also allows now to reserve DMA pools/buffers for rproc serial
>>> via Device Tree.
>>>
>>> Fixes: c774ad010873 ("remoteproc: Fix and restore the parenting hierarchy for vdev")
>>> Cc: stable@xxxxxxxxxxxxxxx # 5.1+
>>> Signed-off-by: Alexander Lobakin <alobakin@xxxxx>
>>> ---
>>>  drivers/char/virtio_console.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>>> index a2da8f768b94..1836cc56e357 100644
>>> --- a/drivers/char/virtio_console.c
>>> +++ b/drivers/char/virtio_console.c
>>> @@ -435,12 +435,12 @@ static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t buf_size
>>>  		/*
>>>  		 * Allocate DMA memory from ancestor. When a virtio
>>>  		 * device is created by remoteproc, the DMA memory is
>>> -		 * associated with the grandparent device:
>>> -		 * vdev => rproc => platform-dev.
>>> +		 * associated with the parent device:
>>> +		 * virtioY => remoteprocX#vdevYbuffer.
>>>  		 */
>>> -		if (!vdev->dev.parent || !vdev->dev.parent->parent)
>>> +		buf->dev = vdev->dev.parent;
>>> +		if (!buf->dev)
>>>  			goto free_buf;
>>> -		buf->dev = vdev->dev.parent->parent;
>>>
>>>  		/* Increase device refcnt to avoid freeing it */
>>>  		get_device(buf->dev);
>>> --
>>> 2.29.2
>>>
>>>
>> ---end quoted text---

Thanks,
Al