Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1733640 > unrolled thread

[PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

Back to article view | Back to linux.kernel

Contents

  [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode Huacai Chen <chenhc@lemote.com> - 2017-09-18 06:30 +0200
    Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in  non-coherent DMA mode Christoph Hellwig <hch@infradead.org> - 2017-09-18 07:30 +0200
      Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode "陈华才" <chenhc@lemote.com> - 2017-09-18 09:00 +0200
    Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent  DMA mode Robin Murphy <robin.murphy@arm.com> - 2017-09-18 11:50 +0200
      Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in  non-coherent DMA mode Christoph Hellwig <hch@infradead.org> - 2017-09-18 18:00 +0200
        Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode "陈华才" <chenhc@lemote.com> - 2017-09-19 04:30 +0200
    Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in  non-coherent DMA mode Christoph Hellwig <hch@infradead.org> - 2017-09-18 17:50 +0200

#1733640 — [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

In non-coherent DMA mode, kernel uses cache flushing operations to
maintain I/O coherency, so the dmapool objects should be aligned to
ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
on MIPS:

	Step 1, dma_map_single
	Step 2, cache_invalidate (no writeback)
	Step 3, dma_from_device
	Step 4, dma_unmap_single

If a DMA buffer and a kernel structure share a same cache line, and if
the kernel structure has dirty data, cache_invalidate (no writeback)
will cause data lost.

Cc: stable@vger.kernel.org
Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
 mm/dmapool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index 4d90a64..6263905 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -140,6 +140,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	else if (align & (align - 1))
 		return NULL;
 
+	if (!device_is_coherent(dev))
+		align = max_t(size_t, align, dma_get_cache_alignment());
+
 	if (size == 0)
 		return NULL;
 	else if (size < 4)
-- 
2.7.0

[toc] | [next] | [standalone]

#1733643 — Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

The dmapool code uses dma_alloc_coherent to allocate each element,
and dma_alloc_coherent must align to ARCH_DMA_MINALIGN already.
If you implementation doesn't do that it needs to be fixed.

[toc] | [prev] | [next] | [standalone]

#1733684 — Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode

Hi, Christoph,

Maybe you missed something.
1, pool_alloc_page() use dma_alloc_coherent() to allocate pool pages, and of course these pages are aligned to  ARCH_DMA_MINALIGN.
2, dma_pool_alloc() is the element allocator, but it doesn't use dma_alloc_coherent(). Elements only align to pool->size, but pool->size is usually less than ARCH_DMA_MINALIGN.
3, ARCH_DMA_MINALIGN is now only used in serveral drivers, no dma_ops use it.

Huacai
 
------------------ Original ------------------
From:  "Christoph Hellwig"<hch@infradead.org>;
Date:  Mon, Sep 18, 2017 01:22 PM
To:  "Huacai Chen"<chenhc@lemote.com>;
Cc:  "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-mm"<linux-mm@kvack.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "stable"<stable@vger.kernel.org>;
Subject:  Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode
 
The dmapool code uses dma_alloc_coherent to allocate each element,
and dma_alloc_coherent must align to ARCH_DMA_MINALIGN already.
If you implementation doesn't do that it needs to be fixed.

[toc] | [prev] | [next] | [standalone]

#1733941 — Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

On 18/09/17 05:22, Huacai Chen wrote:
> In non-coherent DMA mode, kernel uses cache flushing operations to
> maintain I/O coherency, so the dmapool objects should be aligned to
> ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> on MIPS:
> 
> 	Step 1, dma_map_single
> 	Step 2, cache_invalidate (no writeback)
> 	Step 3, dma_from_device
> 	Step 4, dma_unmap_single

This is a massive red warning flag for the whole series, because DMA
pools don't work like that. At best, this will do nothing, and at worst
it is papering over egregious bugs elsewhere. Streaming mappings of
coherent allocations means completely broken code.

> If a DMA buffer and a kernel structure share a same cache line, and if
> the kernel structure has dirty data, cache_invalidate (no writeback)
> will cause data lost.

DMA pools are backed by coherent allocations, and those should already
be at *page* granularity, so this doubly cannot happen for correct code.

More generally, the whole point of having the DMA APIs is that drivers
and subsystems should not have to be aware of details like hardware
coherency. Besides, cache line sharing that could pose a correctness
issue for non-hardware-coherent systems could still be a performance
issue in the presence of hardware coherency (due to unnecessary line
migration), so there's still an argument for not treating them differently.

Robin.

> Cc: stable@vger.kernel.org
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  mm/dmapool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index 4d90a64..6263905 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -140,6 +140,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  	else if (align & (align - 1))
>  		return NULL;
>  
> +	if (!device_is_coherent(dev))
> +		align = max_t(size_t, align, dma_get_cache_alignment());
> +
>  	if (size == 0)
>  		return NULL;
>  	else if (size < 4)
> 

[toc] | [prev] | [next] | [standalone]

#1734260 — Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

On Mon, Sep 18, 2017 at 10:44:54AM +0100, Robin Murphy wrote:
> On 18/09/17 05:22, Huacai Chen wrote:
> > In non-coherent DMA mode, kernel uses cache flushing operations to
> > maintain I/O coherency, so the dmapool objects should be aligned to
> > ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> > on MIPS:
> > 
> > 	Step 1, dma_map_single
> > 	Step 2, cache_invalidate (no writeback)
> > 	Step 3, dma_from_device
> > 	Step 4, dma_unmap_single
> 
> This is a massive red warning flag for the whole series, because DMA
> pools don't work like that. At best, this will do nothing, and at worst
> it is papering over egregious bugs elsewhere. Streaming mappings of
> coherent allocations means completely broken code.

Oh, I hadn't even seen that part.  Yes, dma coherent (and pool)
allocations must never be used for streaming mappings.  I wish we'd
have some debug infrastructure to warn on such uses.

[toc] | [prev] | [next] | [standalone]

#1734602 — Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode

Oh, I know, I've make a mistake, dmapool doesn't need to change.

Huacai
 
 
------------------ Original ------------------
From:  "Christoph Hellwig"<hch@infradead.org>;
Date:  Mon, Sep 18, 2017 11:51 PM
To:  "Robin Murphy"<robin.murphy@arm.com>; 
Cc:  "Huacai Chen"<chenhc@lemote.com>; "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-mm"<linux-mm@kvack.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "stable"<stable@vger.kernel.org>; 
Subject:  Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode

 
On Mon, Sep 18, 2017 at 10:44:54AM +0100, Robin Murphy wrote:
> On 18/09/17 05:22, Huacai Chen wrote:
> > In non-coherent DMA mode, kernel uses cache flushing operations to
> > maintain I/O coherency, so the dmapool objects should be aligned to
> > ARCH_DMA_MINALIGN. Otherwise, it will cause data corruption, at least
> > on MIPS:
> > 
> > 	Step 1, dma_map_single
> > 	Step 2, cache_invalidate (no writeback)
> > 	Step 3, dma_from_device
> > 	Step 4, dma_unmap_single
> 
> This is a massive red warning flag for the whole series, because DMA
> pools don't work like that. At best, this will do nothing, and at worst
> it is papering over egregious bugs elsewhere. Streaming mappings of
> coherent allocations means completely broken code.

Oh, I hadn't even seen that part.  Yes, dma coherent (and pool)
allocations must never be used for streaming mappings.  I wish we'd
have some debug infrastructure to warn on such uses.

[toc] | [prev] | [next] | [standalone]

#1734250 — Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode

Ok, it looks like adding the dma_get_cache_alignment constraint here
looks good - but again it should be unconditional.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web