[PATCH] remoteproc: recopy parent dma_pfn_offset for vdev

[PATCH] remoteproc: recopy parent dma_pfn_offset for vdev

[Clement Leger]

When preparing the subdevice for the vdev, also copy dma_pfn_offset
since this is used for sub device dma allocations. Without that, there
is incoherency between the parent dma settings and the childs one,
potentially leading to dma_alloc_coherent failure (due to phys_to_dma
using dma_pfn_offset for translation).
---
 drivers/remoteproc/remoteproc_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 263e9c9614a8..3b56ca043231 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -520,6 +520,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.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);
 	dev_set_drvdata(&rvdev->dev, rvdev);
-- 
2.15.0.276.g89ea799

RE: [PATCH] remoteproc: recopy parent dma_pfn_offset for vdev

[Loic PALLARDY]

Hi Clement,

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> owner@vger.kernel.org> On Behalf Of Clement Leger
> Sent: mercredi 12 juin 2019 11:55
> To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>; linux-remoteproc <linux-
> remoteproc@vger.kernel.org>
> Cc: Clement Leger <cleger@kalray.eu>
> Subject: [PATCH] remoteproc: recopy parent dma_pfn_offset for vdev
> 
> When preparing the subdevice for the vdev, also copy dma_pfn_offset
> since this is used for sub device dma allocations. Without that, there
> is incoherency between the parent dma settings and the childs one,
> potentially leading to dma_alloc_coherent failure (due to phys_to_dma
> using dma_pfn_offset for translation).

Thanks for this correction.
Could you please add fixes in message commit
Fixes: 086d08725d34 ("remoteproc: create vdev subdevice with specific dma memory pool")

Else
Acked-by: Loic Pallardy <loic.pallardy@st.com>

Regards,
Loic

> ---
>  drivers/remoteproc/remoteproc_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 263e9c9614a8..3b56ca043231 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -520,6 +520,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.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);
>  	dev_set_drvdata(&rvdev->dev, rvdev);
> --
> 2.15.0.276.g89ea799

Re: [PATCH] remoteproc: recopy parent dma_pfn_offset for vdev

[Bjorn Andersson]

On Wed 12 Jun 02:55 PDT 2019, Clement Leger wrote:

> When preparing the subdevice for the vdev, also copy dma_pfn_offset
> since this is used for sub device dma allocations. Without that, there
> is incoherency between the parent dma settings and the childs one,
> potentially leading to dma_alloc_coherent failure (due to phys_to_dma
> using dma_pfn_offset for translation).

Hi Clement,

This looks reasonable, but I can't merge it without your Signed-off-by.

Can you please resubmit the change, with the Fixes: and Loic's ack?

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 263e9c9614a8..3b56ca043231 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -520,6 +520,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.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);
>  	dev_set_drvdata(&rvdev->dev, rvdev);
> -- 
> 2.15.0.276.g89ea799
> 

[PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Clement Leger]

When preparing the subdevice for the vdev, also copy dma_pfn_offset
since this is used for sub device dma allocations. Without that, there
is incoherency between the parent dma settings and the childs one,
potentially leading to dma_alloc_coherent failure (due to phys_to_dma
using dma_pfn_offset for translation).

Fixes: 086d08725d34 ("remoteproc: create vdev subdevice with specific dma memory pool")
Signed-off-by: Clement Leger <cleger@kalray.eu>
Acked-by: Loic Pallardy <loic.pallardy@st.com>
---
Changes in v2:
 - Fix typo in commit message
 - Add "Fixes" in commit message
 - Add Signed-off
 - Add Acked-by Loic Pallardy

---
 drivers/remoteproc/remoteproc_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 48feebd6d0a2..06837b1f2d60 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -520,6 +520,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.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);
 	dev_set_drvdata(&rvdev->dev, rvdev);
-- 
2.15.0.276.g89ea799

Re: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Bjorn Andersson]

On Mon 01 Jul 00:02 PDT 2019, Clement Leger wrote:

> When preparing the subdevice for the vdev, also copy dma_pfn_offset
> since this is used for sub device dma allocations. Without that, there
> is incoherency between the parent dma settings and the childs one,
> potentially leading to dma_alloc_coherent failure (due to phys_to_dma
> using dma_pfn_offset for translation).
> 
> Fixes: 086d08725d34 ("remoteproc: create vdev subdevice with specific dma memory pool")
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> Acked-by: Loic Pallardy <loic.pallardy@st.com>

Thanks for the update. Applied

Regards,
Bjorn

> ---
> Changes in v2:
>  - Fix typo in commit message
>  - Add "Fixes" in commit message
>  - Add Signed-off
>  - Add Acked-by Loic Pallardy
> 
> ---
>  drivers/remoteproc/remoteproc_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 48feebd6d0a2..06837b1f2d60 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -520,6 +520,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.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);
>  	dev_set_drvdata(&rvdev->dev, rvdev);
> -- 
> 2.15.0.276.g89ea799
> 

Re: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Christoph Hellwig]

This is just increasing the mess remoteproc has created with the vdev.
It is poking its nose way to deep into the DMA layer internals, and
creating massive problems that way.  Can we go back to the table
and figure out what the root problem even was?  To me it seems if you
clearly need separate devices they should be declared as such in the
device tree.

RE: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Loic PALLARDY]

Hi Christoph,

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: mardi 2 juillet 2019 15:22
> To: Clement Leger <cleger@kalray.eu>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Loic PALLARDY <loic.pallardy@st.com>
> Subject: Re: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev
> 
> This is just increasing the mess remoteproc has created with the vdev.
> It is poking its nose way to deep into the DMA layer internals, and
> creating massive problems that way.  Can we go back to the table
> and figure out what the root problem even was?  To me it seems if you
> clearly need separate devices they should be declared as such in the
> device tree.

Agree there is definitively an issue with the way virtio device are defined.
Today definition is based on rproc firmware ressource table and rproc 
framework is in charge of vdev creation.
Device tree definition was discarded as vdev is not HW but SW definition.
One solution would be to associate both resource table (which provides
Firmware capabilities) and some virtio device tree nodes (declared as sub nodes
of remote processor with associated resources like memory carveout).
When we have a match between resource table and rproc DT sub node, we
can register virtio device via of_platform_populate.
Then need to adapt virtio_rpmsg or to create a virtio_rproc to be DT probe compliant
like virtio_mmio is.

But that's breaking legacy as all platforms will have to add a virtio device node in
their DT file...

Is it aligned with your view ?

Regards,
Loic

Re: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Christoph Hellwig]

On Tue, Jul 02, 2019 at 03:36:56PM +0000, Loic PALLARDY wrote:
> Agree there is definitively an issue with the way virtio device are defined.
> Today definition is based on rproc firmware ressource table and rproc 
> framework is in charge of vdev creation.
> Device tree definition was discarded as vdev is not HW but SW definition.

Well, it appears to be a firmware interface description.

> One solution would be to associate both resource table (which provides
> Firmware capabilities) and some virtio device tree nodes (declared as sub nodes
> of remote processor with associated resources like memory carveout).
> When we have a match between resource table and rproc DT sub node, we
> can register virtio device via of_platform_populate.
> Then need to adapt virtio_rpmsg or to create a virtio_rproc to be DT probe compliant
> like virtio_mmio is.
> 
> But that's breaking legacy as all platforms will have to add a virtio device node in
> their DT file...
> 
> Is it aligned with your view ?

Yes, that is how I'd assume it works.  But given that until recently
you did now have these subdevices for dma coherent purposes we can't
really break anything older than that, so I might still be missing
something.

Re: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Christoph Hellwig]

On Mon, Jul 08, 2019 at 11:45:46AM -0700, Christoph Hellwig wrote:
> > But that's breaking legacy as all platforms will have to add a virtio device node in
> > their DT file...
> > 
> > Is it aligned with your view ?
> 
> Yes, that is how I'd assume it works.  But given that until recently
> you did now have these subdevices for dma coherent purposes we can't
> really break anything older than that, so I might still be missing
> something.

Any chance we could expedite this?  remoteproc is the only driver
inheriting dma ops to subdevices, and the only one using
dma_declare_coherent_memory.  I'd really like to clean this mess up
rather sooner than later.

RE: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Loic PALLARDY]

> -----Original Message-----
> From: Christoph Hellwig <hch@infradead.org>
> Sent: vendredi 19 juillet 2019 08:33
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: Christoph Hellwig <hch@infradead.org>; Clement Leger
> <cleger@kalray.eu>; Ohad Ben-Cohen <ohad@wizery.com>; Bjorn
> Andersson <bjorn.andersson@linaro.org>; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev
> 
> On Mon, Jul 08, 2019 at 11:45:46AM -0700, Christoph Hellwig wrote:
> > > But that's breaking legacy as all platforms will have to add a virtio device
> node in
> > > their DT file...
> > >
> > > Is it aligned with your view ?
> >
> > Yes, that is how I'd assume it works.  But given that until recently
> > you did now have these subdevices for dma coherent purposes we can't
> > really break anything older than that, so I might still be missing
> > something.
> 
> Any chance we could expedite this?  remoteproc is the only driver
> inheriting dma ops to subdevices, and the only one using
> dma_declare_coherent_memory.  I'd really like to clean this mess up
> rather sooner than later.

Ongoing...
Two topics to clean up:
- Sub device creation and DMA ops inheritance --> need to use platform_device or device tree
- dma_declare_coherent_memory use --> it has been introduced to support internal memories declared via reg field.
I propose to migrate existing drivers on reserved memory usage and so remove dma_declare_coherent call from remoteproc core.

Re: [PATCH v2] remoteproc: copy parent dma_pfn_offset for vdev

[Christoph Hellwig]

On Mon, Jul 22, 2019 at 08:41:32AM +0000, Loic PALLARDY wrote:
> Ongoing...
> Two topics to clean up:
> - Sub device creation and DMA ops inheritance --> need to use platform_device or device tree
> - dma_declare_coherent_memory use --> it has been introduced to support internal memories declared via reg field.
> I propose to migrate existing drivers on reserved memory usage and so remove dma_declare_coherent call from remoteproc core.

I thought this was inter-related in that you needed the additional
field with the manual memory region for the subdevs, but I guess I
misunderstood.  Anyway, thanks for doing the work.