Re: [PATCH] dma-debug: Remove warning in dma_debug_entry

[Robin Murphy]

On 2022-07-30 09:56, yf.wang@xxxxxxxxxxxx wrote:
> From: Yunfei Wang <yf.wang@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
>   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");
>   	}
>   }
>