From: Yunfei Wang <yf.wang@mediatek.com>

The same physical address can be mapped multiple times, and it is not
required to have attributes such as DMA_ATTR_SKIP_CPU_SYNC, but
dma_debug_entry will report an warning if active_cacheline_insert
returns -EEXIST, which can cause a lot of trouble.

For example, if two dma engine share the same dma-buf, the two dma
engine drivers will call the DMA API to map their own dma address
respectively, which is normal follow, but dma_debug_entry will
report an warning.

In addition, if active_cacheline_insert returns -EEXIST, the
active_cacheline_inc_overlap function already has the logic to
check overlapping mappings counter, if it exceeded
ACTIVE_CACHELINE_MAX_OVERLAP overlapping mappings, it already
has a warning.
And also, if warning as panic is turned on, it will cause KE.

So, dma_debug_entry does not need report warning.

Signed-off-by: Yunfei Wang <yf.wang@mediatek.com>
---
 kernel/dma/debug.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 2caafd13f8aa..ad731f7858c9 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -566,9 +566,6 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)
 	if (rc == -ENOMEM) {
 		pr_err_once("cacheline tracking ENOMEM, dma-debug disabled\n");
 		global_disable = true;
-	} else if (rc == -EEXIST && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
-		err_printk(entry->dev, entry,
-			"cacheline tracking EEXIST, overlapping mappings aren't supported\n");
 	}
 }
 
-- 
2.18.0

On 2022-07-30 09:56, yf.wang@mediatek.com wrote:
> From: Yunfei Wang <yf.wang@mediatek.com>
> 
> The same physical address can be mapped multiple times, and it is not
> required to have attributes such as DMA_ATTR_SKIP_CPU_SYNC, but
> dma_debug_entry will report an warning if active_cacheline_insert
> returns -EEXIST, which can cause a lot of trouble.
> 
> For example, if two dma engine share the same dma-buf, the two dma
> engine drivers will call the DMA API to map their own dma address
> respectively, which is normal follow, but dma_debug_entry will
> report an warning.
> 
> In addition, if active_cacheline_insert returns -EEXIST, the
> active_cacheline_inc_overlap function already has the logic to
> check overlapping mappings counter, if it exceeded
> ACTIVE_CACHELINE_MAX_OVERLAP overlapping mappings, it already
> has a warning.
> And also, if warning as panic is turned on, it will cause KE.
> 
> So, dma_debug_entry does not need report warning.

I'm sure this has come up before, but I can't remember enough of the 
context to easily search for the previous thread.

In short, this check is not entirely correct, but removing it isn't 
right either, since that would be too much of a step backwards in terms 
of debug coverage IMO. What we need here is to do a bit more work to 
differentiate between mappings of the exact same buffer, which are 
allowed, vs. any other kind of partial overlaps which are still illegal 
and definitely deserve a warning.

Furthermore, even for the valid dma-buf import case I'm not convinced 
that the SKIP_CPU_SYNC check can be dropped either. If one device can 
import a buffer while another device is already accessing it, there are 
definitely combinations of parameters which could lead to potential data 
loss (e.g. a non-coherent DMA_TO_DEVICE mapping during a non-coherent 
DMA_FROM_DEVICE access, a or non-coherent DMA_FROM_DEVICE mapping during 
a coherent DMA_TO_DEVICE access).

Thanks,
Robin.

> Signed-off-by: Yunfei Wang <yf.wang@mediatek.com>
> ---
>   kernel/dma/debug.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 2caafd13f8aa..ad731f7858c9 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -566,9 +566,6 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)
>   	if (rc == -ENOMEM) {
>   		pr_err_once("cacheline tracking ENOMEM, dma-debug disabled\n");
>   		global_disable = true;
> -	} else if (rc == -EEXIST && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
> -		err_printk(entry->dev, entry,
> -			"cacheline tracking EEXIST, overlapping mappings aren't supported\n");
>   	}
>   }
>

On Mon, Aug 01, 2022 at 01:07:10PM +0100, Robin Murphy wrote:
> Furthermore, even for the valid dma-buf import case I'm not convinced that 
> the SKIP_CPU_SYNC check can be dropped either. If one device can import a 
> buffer while another device is already accessing it, there are definitely 
> combinations of parameters which could lead to potential data loss (e.g. a 
> non-coherent DMA_TO_DEVICE mapping during a non-coherent DMA_FROM_DEVICE 
> access, a or non-coherent DMA_FROM_DEVICE mapping during a coherent 
> DMA_TO_DEVICE access).

Yes.  While there is a rare case where double mapping actually works
(exactly the same region, no access to the data between the invidual
mapping and unmapping calls), there is absolutely no good reason to
support it.  The additional cache maintainance operations will still
take some overhead that can be skipped, and it makes the rules very
confusing.  It is much better to fix the upper layers to use
the SKIP_CPU_SYNC flag in case of multiple mappings, which also
clearly documents what is being done there.