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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me --- 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);
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:
You are correct.
[ 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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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;
Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org
/* Increase device refcnt to avoid freeing it */ get_device(buf->dev); -- 2.29.2
On 2020/11/4 下午11:31, 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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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;
I wonder it could be the right time to introduce dma_dev for virtio instead of depending on something magic via parent.
(Btw I don't even notice that there's transport specific code in virtio console, it's better to avoid it)
Thanks
/* Increase device refcnt to avoid freeing it */ get_device(buf->dev);
From: Jason Wang jasowang@redhat.com Date: Thu, 5 Nov 2020 11:10:24 +0800
Hi Jason,
On 2020/11/4 下午11:31, 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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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;I wonder it could be the right time to introduce dma_dev for virtio instead of depending on something magic via parent.
This patch are meant to hit RC window and stable trees as a fix of the bug that is present since v5.8-rc1. So any new features are out of scope of this particular fix.
The idea of DMAing through "dev->parent" is that "virtioX" itself is a logical dev, not the real one, but its parent *is*. This logic is used across the whole tree -- every subsystem creates its own logical device, but drivers should always use the backing PCI/platform/etc. devices for DMA operations, which represent the real hardware.
(Btw I don't even notice that there's transport specific code in virtio console, it's better to avoid it)
Thanks
Thanks, Al
/* Increase device refcnt to avoid freeing it */ get_device(buf->dev);
On 2020/11/5 下午8:22, Alexander Lobakin wrote:
From: Jason Wang jasowang@redhat.com Date: Thu, 5 Nov 2020 11:10:24 +0800
Hi Jason,
On 2020/11/4 下午11:31, 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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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;I wonder it could be the right time to introduce dma_dev for virtio instead of depending on something magic via parent.
This patch are meant to hit RC window and stable trees as a fix of the bug that is present since v5.8-rc1. So any new features are out of scope of this particular fix.
Right.
The idea of DMAing through "dev->parent" is that "virtioX" itself is a logical dev, not the real one, but its parent *is*. This logic is used across the whole tree -- every subsystem creates its own logical device, but drivers should always use the backing PCI/platform/etc. devices for DMA operations, which represent the real hardware.
Yes, so what I meant is to use different variables for DMA and hierarchy. So it's the responsibility of the lower layer to pass a correct "dma_dev" to the upper layer instead of depending parent.
Anyway for this patch.
Acked-by: Jason Wang jasowang@redhat.com
Thanks
(Btw I don't even notice that there's transport specific code in virtio console, it's better to avoid it)
Thanks
Thanks, Al
/* Increase device refcnt to avoid freeing it */ get_device(buf->dev);
I just noticed this showing up in Linus' tree and I'm not happy.
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.
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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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---
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.
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),
Could you point me at an example or two please?
Thanks!
instead of faking up devices with broken adhoc inheritance of DMA properties and magic poking into device parent relationships.
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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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---
On Mon, Nov 16, 2020 at 04:51:49AM -0500, Michael S. Tsirkin wrote:
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.
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),
Could you point me at an example or two please?
Take a look at the ib_dma_* helper in include/rdma/ib_verbs.h and dma_device member in struct ib_device for the best example.
From: Christoph Hellwig hch@infradead.org Date: Mon, 16 Nov 2020 16:27:44 +0000
On Mon, Nov 16, 2020 at 04:51:49AM -0500, Michael S. Tsirkin wrote:
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.
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),
Could you point me at an example or two please?
Take a look at the ib_dma_* helper in include/rdma/ib_verbs.h and dma_device member in struct ib_device for the best example.
Oh, best example indeed. I did really love these helpers and kinda wish there were such for Ethernet and wireless networking. They'd allow to keep the code more readable and clean and prevent from several sorts of silly mistakes.
This could be done in e.g. 4 steps: - introduce such helpers for netdev/mac80211; - add checkpatch warnings to discourage usage of old methods like SET_NETDEV_DEV() and direct dereferencing of netdev->dev.parent; - slowly convert existing drivers to the new model; - remove the old way entirely along with checkpatch remnants.
I could take this if there'll be enough votes :)
Al
Hi all,
On 11/16/20 10:19 AM, Christoph Hellwig wrote:
I just noticed this showing up in Linus' tree and I'm not happy.
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.
For your formation I started some stuff on my side to be able to declare the virtio device in DT as a remoteproc child node.
https://lkml.org/lkml/2020/4/16/1817
Quite big refactoring, but could be a way to answer...
Regards, Arnaud
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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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---
On Mon, Nov 16, 2020 at 11:46:59AM +0100, Arnaud POULIQUEN wrote:
Hi all,
On 11/16/20 10:19 AM, Christoph Hellwig wrote:
I just noticed this showing up in Linus' tree and I'm not happy.
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.
For your formation I started some stuff on my side to be able to declare the virtio device in DT as a remoteproc child node.
https://lkml.org/lkml/2020/4/16/1817
Quite big refactoring, but could be a way to answer...
Yes, that series is exactly what we need to do conceptually (can't comment on all the nitty grity details as I'm not too familiar with the remoteproc code).
Btw, I also still don't understand why remoteproc is using dma_declare_coherent_memory to start with. The virtio code has exactly one call to dma_alloc_coherent vring_alloc_queue, a function that already switches between two different allocators. Why can't we just add a third allocator specifically for these remoteproc memory carveouts and bypass dma_declare_coherent_memory entirely?
On 11/16/20 5:39 PM, Christoph Hellwig wrote:
Btw, I also still don't understand why remoteproc is using dma_declare_coherent_memory to start with. The virtio code has exactly one call to dma_alloc_coherent vring_alloc_queue, a function that already switches between two different allocators. Why can't we just add a third allocator specifically for these remoteproc memory carveouts and bypass dma_declare_coherent_memory entirely?
The dma_declare_coherent_memory allows to associate vdev0buffer memory region to the remoteproc virtio device (vdev parent). This region is used to allocated the rpmsg buffers. The memory for the rpmsg buffer is allocated by the rpmsg_virtio device in rpmsg_virtio_bus[1]. The size depends on the total size needed for the rpmsg buffers.
The vrings are allocated directly by the remoteproc device.
[1] https://elixir.bootlin.com/linux/v5.10-rc3/source/drivers/rpmsg/virtio_rpmsg...
On Tue, Nov 17, 2020 at 03:00:32PM +0100, Arnaud POULIQUEN wrote:
The dma_declare_coherent_memory allows to associate vdev0buffer memory region to the remoteproc virtio device (vdev parent). This region is used to allocated the rpmsg buffers. The memory for the rpmsg buffer is allocated by the rpmsg_virtio device in rpmsg_virtio_bus[1]. The size depends on the total size needed for the rpmsg buffers.
The vrings are allocated directly by the remoteproc device.
Weird. I thought virtio was pretty strict in not allowing diret DMA API usage in drivers to support the legacy no-mapping case.
Either way, the point stands: if you want these magic buffers handed out to specific rpmsg instances I think not having to detour through the DMA API is going to make everyones life easier.
On Tue, Nov 17, 2020 at 02:02:30PM +0000, Christoph Hellwig wrote:
On Tue, Nov 17, 2020 at 03:00:32PM +0100, Arnaud POULIQUEN wrote:
The dma_declare_coherent_memory allows to associate vdev0buffer memory region to the remoteproc virtio device (vdev parent). This region is used to allocated the rpmsg buffers. The memory for the rpmsg buffer is allocated by the rpmsg_virtio device in rpmsg_virtio_bus[1]. The size depends on the total size needed for the rpmsg buffers.
The vrings are allocated directly by the remoteproc device.
Weird. I thought virtio was pretty strict in not allowing diret DMA API usage in drivers to support the legacy no-mapping case.
Well remoteproc is weird in that it's use of DMA API precedes standartization efforts, and it was never standardized in the virtio spec ..
Either way, the point stands: if you want these magic buffers handed out to specific rpmsg instances I think not having to detour through the DMA API is going to make everyones life easier.
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.
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.
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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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---
From: "Michael S. Tsirkin" mst@redhat.com 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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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
On Mon, Nov 16, 2020 at 01:07:28PM +0000, Alexander Lobakin wrote:
From: "Michael S. Tsirkin" mst@redhat.com 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@vger.kernel.org # 5.1+ Signed-off-by: Alexander Lobakin alobakin@pm.me
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
On Mon, Nov 16, 2020 at 01:07:28PM +0000, Alexander Lobakin wrote:
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.
Oh, every drivers is perfectly fine to use ->parent as it suits. The problem is when we have layered architectures, where this pokes a massive hole into the layering.
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.
As far as I can tell the series from Arnaud does the right thing here.
linux-stable-mirror@lists.linaro.org
-
Alexander Lobakin -
Arnaud POULIQUEN -
Christoph Hellwig -
Jason Wang -
Mathieu Poirier -
Michael S. Tsirkin