* [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
@ 2017-09-18 4:22 ` Huacai Chen
0 siblings, 0 replies; 12+ messages in thread
From: Huacai Chen @ 2017-09-18 4:22 UTC (permalink / raw)
To: Andrew Morton; +Cc: Fuxin Zhang, linux-mm, linux-kernel, Huacai Chen, stable
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
2017-09-18 4:22 ` Huacai Chen
@ 2017-09-18 5:22 ` Christoph Hellwig
-1 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2017-09-18 5:22 UTC (permalink / raw)
To: Huacai Chen; +Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
@ 2017-09-18 5:22 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2017-09-18 5:22 UTC (permalink / raw)
To: Huacai Chen; +Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode
2017-09-18 5:22 ` Christoph Hellwig
(?)
@ 2017-09-18 6:55 ` 陈华才
-1 siblings, 0 replies; 12+ messages in thread
From: 陈华才 @ 2017-09-18 6:55 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
2017-09-18 4:22 ` Huacai Chen
@ 2017-09-18 9:44 ` Robin Murphy
-1 siblings, 0 replies; 12+ messages in thread
From: Robin Murphy @ 2017-09-18 9:44 UTC (permalink / raw)
To: Huacai Chen; +Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
@ 2017-09-18 9:44 ` Robin Murphy
0 siblings, 0 replies; 12+ messages in thread
From: Robin Murphy @ 2017-09-18 9:44 UTC (permalink / raw)
To: Huacai Chen; +Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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)
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
2017-09-18 9:44 ` Robin Murphy
@ 2017-09-18 15:51 ` Christoph Hellwig
-1 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2017-09-18 15:51 UTC (permalink / raw)
To: Robin Murphy
Cc: Huacai Chen, Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
@ 2017-09-18 15:51 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2017-09-18 15:51 UTC (permalink / raw)
To: Robin Murphy
Cc: Huacai Chen, Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [V5, 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN innon-coherent DMA mode
2017-09-18 15:51 ` Christoph Hellwig
(?)
@ 2017-09-19 2:23 ` 陈华才
-1 siblings, 0 replies; 12+ messages in thread
From: 陈华才 @ 2017-09-19 2:23 UTC (permalink / raw)
To: Christoph Hellwig, Robin Murphy
Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
2017-09-18 4:22 ` Huacai Chen
@ 2017-09-18 15:45 ` Christoph Hellwig
-1 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2017-09-18 15:45 UTC (permalink / raw)
To: Huacai Chen; +Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
Ok, it looks like adding the dma_get_cache_alignment constraint here
looks good - but again it should be unconditional.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V5 2/3] mm: dmapool: Align to ARCH_DMA_MINALIGN in non-coherent DMA mode
@ 2017-09-18 15:45 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2017-09-18 15:45 UTC (permalink / raw)
To: Huacai Chen; +Cc: Andrew Morton, Fuxin Zhang, linux-mm, linux-kernel, stable
Ok, it looks like adding the dma_get_cache_alignment constraint here
looks good - but again it should be unconditional.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 12+ messages in thread