| Message ID | 20190701070245.32083-1-cleger@kalray.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] remoteproc: copy parent dma_pfn_offset for vdev | expand |
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 >
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.
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
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.
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.
> -----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.
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.
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);
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(+)