| Message ID | 20170329163335.GA7909@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Russell, On Wed, Mar 29, 2017 at 10:33 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > Okay, I'm about to merge the following patch for -rc, since refusing > to create a scattertable for non-page backed memory is the only valid > solution for that case. I'm intending to queue this for -rc this > evening. I was bit sidetracked and getting back to this today. I am just about to test this change on my system. I will test your patch and send you results. This might not help my use-case - I suspect it will trip this check. That said I will send you results very soon. > > 8<==== > ARM: dma-mapping: disallow dma_get_sgtable() for non-kernel managed memory > > dma_get_sgtable() tries to create a scatterlist table containing valid > struct page pointers for the coherent memory allocation passed in to it. > > However, memory can be declared via dma_declare_coherent_memory(), or > via other reservation schemes which means that coherent memory is not > guaranteed to be backed by struct pages. This means it is not possible > to create a valid scatterlist via this mechanism. I have been thinking about this some. I would like to see if we can provide a way forward to address the cases where coherent memory is not guaranteed to be backed by struct pages. We know the memory isn't backed at alloc time in dma_alloc_coherent(). Can we key off of that maybe add a new attr flag to avoid page lookups. I am willing to work on the fixing it. Let me first send you the results after testing your patch. Maybe we can explore ways to fix the problem. thanks, -- Shuah > > This patch adds detection of such memory, and refuses to create a > scatterlist table for such memory. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > arch/arm/mm/dma-mapping.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 63eabb06f9f1..475811f5383a 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -935,13 +935,31 @@ static void arm_coherent_dma_free(struct device *dev, size_t size, void *cpu_add > __arm_dma_free(dev, size, cpu_addr, handle, attrs, true); > } > > +/* > + * The whole dma_get_sgtable() idea is fundamentally unsafe - it seems > + * that the intention is to allow exporting memory allocated via the > + * coherent DMA APIs through the dma_buf API, which only accepts a > + * scattertable. This presents a couple of problems: > + * 1. Not all memory allocated via the coherent DMA APIs is backed by > + * a struct page > + * 2. Passing coherent DMA memory into the streaming APIs is not allowed > + * as we will try to flush the memory through a different alias to that > + * actually being used (and the flushes are redundant.) > + */ > int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt, > void *cpu_addr, dma_addr_t handle, size_t size, > unsigned long attrs) > { > - struct page *page = pfn_to_page(dma_to_pfn(dev, handle)); > + unsigned long pfn = dma_to_pfn(dev, handle); > + struct page *page; > int ret; > > + /* If the PFN is not valid, we do not have a struct page */ > + if (!pfn_valid(pfn)) > + return -ENXIO; > + > + page = pfn_to_page(pfn); > + > ret = sg_alloc_table(sgt, 1, GFP_KERNEL); > if (unlikely(ret)) > return ret; > -- > 2.7.4 > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Wed, Mar 29, 2017 at 10:51:05AM -0600, Shuah Khan wrote: > Hi Russell, > > On Wed, Mar 29, 2017 at 10:33 AM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > Okay, I'm about to merge the following patch for -rc, since refusing > > to create a scattertable for non-page backed memory is the only valid > > solution for that case. I'm intending to queue this for -rc this > > evening. > > I was bit sidetracked and getting back to this today. I am just about > to test this change on my system. I will test your patch and send you > results. > > This might not help my use-case - I suspect it will trip this check. > That said I will send you results very soon. I do hope that it _does_ trip this check - the result should be that you get an error without the kernel oopsing. > I have been thinking about this some. I would like to see if we can > provide a way forward to address the cases where coherent memory is > not guaranteed to be backed by struct pages. We know the memory isn't > backed at alloc time in dma_alloc_coherent(). Can we key off of that > maybe add a new attr flag to avoid page lookups. I am willing to work > on the fixing it. What we need is to revise the dma-buf API and DMA APIs to allow passing coherent memory through it - there is no current provision in the DMA APIs to have coherent memory mapped for two peripherals.
Hi Russell, On Wed, Mar 29, 2017 at 11:03 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Wed, Mar 29, 2017 at 10:51:05AM -0600, Shuah Khan wrote: >> Hi Russell, >> >> On Wed, Mar 29, 2017 at 10:33 AM, Russell King - ARM Linux >> <linux@armlinux.org.uk> wrote: >> > Okay, I'm about to merge the following patch for -rc, since refusing >> > to create a scattertable for non-page backed memory is the only valid >> > solution for that case. I'm intending to queue this for -rc this >> > evening. >> >> I was bit sidetracked and getting back to this today. I am just about >> to test this change on my system. I will test your patch and send you >> results. >> >> This might not help my use-case - I suspect it will trip this check. >> That said I will send you results very soon. > > I do hope that it _does_ trip this check - the result should be that you > get an error without the kernel oopsing. Agreed. I would rather have it fail gracefully earlier than limp along and die later. I would make a suggestion based on these results. Please consider adding a dev_err() to if (!pfn_valid(pfn)) condition to indicate why get_sgtable failed including __func__ I worry about adding dev_err() to a core function, but in this case, it would be helpful to do to make it very clear that the requested use-case can't be supported. Results: [ 235.706048] platform 11000000.codec:left: failed to get scatterlist from DMA API The following is a WARN_ON from vb2_dc_get_dmabuf() - probably should go away. I will send a patch removing the WARN_ON to linux-media [ 235.711985] ------------[ cut here ]------------ [ 235.712001] WARNING: CPU: 6 PID: 1987 at drivers/media/v4l2-core/videobuf2-dma-contig.c:402 vb2_dc_get_dmabuf+0x130/0x168 [videobuf2_dma_contig] [ 235.712005] Modules linked in: cpufreq_userspace cpufreq_powersave cpufreq_conservative s5p_jpeg s5p_mfc exynos_gsc v4l2_mem2mem videobuf2_dma_contig v4l2_common videobuf2_memops videobuf2_v4l2 videobuf2_core videodev media [ 235.712063] CPU: 6 PID: 1987 Comm: qtdemux0:sink Not tainted 4.11.0-rc2-00001-g49214d0-dirty #11 [ 235.712068] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [ 235.712085] [<c010e244>] (unwind_backtrace) from [<c010aea0>] (show_stack+0x10/0x14) [ 235.712096] [<c010aea0>] (show_stack) from [<c033ef50>] (dump_stack+0x78/0x8c) [ 235.712106] [<c033ef50>] (dump_stack) from [<c011b3c8>] (__warn+0xe8/0x100) [ 235.712113] [<c011b3c8>] (__warn) from [<c011b490>] (warn_slowpath_null+0x20/0x28) [ 235.712122] [<c011b490>] (warn_slowpath_null) from [<bf20d8b8>] (vb2_dc_get_dmabuf+0x130/0x168 [videobuf2_dma_contig]) [ 235.712141] [<bf20d8b8>] (vb2_dc_get_dmabuf [videobuf2_dma_contig]) from [<bf151ec4>] (vb2_core_expbuf+0x1d4/0x2d8 [videobuf2_core]) [ 235.712159] [<bf151ec4>] (vb2_core_expbuf [videobuf2_core]) from [<bf19c198>] (vb2_expbuf+0x2c/0x34 [videobuf2_v4l2]) [ 235.712195] [<bf19c198>] (vb2_expbuf [videobuf2_v4l2]) from [<bf04b834>] (__video_do_ioctl+0x204/0x2fc [videodev]) [ 235.712241] [<bf04b834>] (__video_do_ioctl [videodev]) from [<bf04b390>] (video_usercopy+0x24c/0x4e0 [videodev]) [ 235.712285] [<bf04b390>] (video_usercopy [videodev]) from [<bf0465a4>] (v4l2_ioctl+0xa0/0xd8 [videodev]) [ 235.712316] [<bf0465a4>] (v4l2_ioctl [videodev]) from [<c01fe920>] (do_vfs_ioctl+0x9c/0x8e0) [ 235.712325] [<c01fe920>] (do_vfs_ioctl) from [<c01ff198>] (SyS_ioctl+0x34/0x5c) [ 235.712334] [<c01ff198>] (SyS_ioctl) from [<c0107740>] (ret_fast_syscall+0x0/0x3c) [ 235.712339] ---[ end trace cdfda037de46497c ]--- > >> I have been thinking about this some. I would like to see if we can >> provide a way forward to address the cases where coherent memory is >> not guaranteed to be backed by struct pages. We know the memory isn't >> backed at alloc time in dma_alloc_coherent(). Can we key off of that >> maybe add a new attr flag to avoid page lookups. I am willing to work >> on the fixing it. > > What we need is to revise the dma-buf API and DMA APIs to allow passing > coherent memory through it - there is no current provision in the DMA APIs > to have coherent memory mapped for two peripherals. I will explore adding the support. I do think there is value in being able to use coherent memory buffers for DMA. thanks, -- Shuah > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 63eabb06f9f1..475811f5383a 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -935,13 +935,31 @@ static void arm_coherent_dma_free(struct device *dev, size_t size, void *cpu_add __arm_dma_free(dev, size, cpu_addr, handle, attrs, true); } +/* + * The whole dma_get_sgtable() idea is fundamentally unsafe - it seems + * that the intention is to allow exporting memory allocated via the + * coherent DMA APIs through the dma_buf API, which only accepts a + * scattertable. This presents a couple of problems: + * 1. Not all memory allocated via the coherent DMA APIs is backed by + * a struct page + * 2. Passing coherent DMA memory into the streaming APIs is not allowed + * as we will try to flush the memory through a different alias to that + * actually being used (and the flushes are redundant.) + */ int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt, void *cpu_addr, dma_addr_t handle, size_t size, unsigned long attrs) { - struct page *page = pfn_to_page(dma_to_pfn(dev, handle)); + unsigned long pfn = dma_to_pfn(dev, handle); + struct page *page; int ret; + /* If the PFN is not valid, we do not have a struct page */ + if (!pfn_valid(pfn)) + return -ENXIO; + + page = pfn_to_page(pfn); + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); if (unlikely(ret)) return ret;
Okay, I'm about to merge the following patch for -rc, since refusing to create a scattertable for non-page backed memory is the only valid solution for that case. I'm intending to queue this for -rc this evening. 8<==== ARM: dma-mapping: disallow dma_get_sgtable() for non-kernel managed memory dma_get_sgtable() tries to create a scatterlist table containing valid struct page pointers for the coherent memory allocation passed in to it. However, memory can be declared via dma_declare_coherent_memory(), or via other reservation schemes which means that coherent memory is not guaranteed to be backed by struct pages. This means it is not possible to create a valid scatterlist via this mechanism. This patch adds detection of such memory, and refuses to create a scatterlist table for such memory. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- arch/arm/mm/dma-mapping.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)