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

[Michael S. Tsirkin]

On Mon, Nov 16, 2020 at 01:07:28PM +0000, Alexander Lobakin wrote:
> 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.

Oh you are right, Greg merged it and I didn't notice because I didn't
rebase my tree.

> 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