Re: [PATCH] dma: debug: detect repeated DMA sync

[Rouven Czerwinski <r.czerwinski@pengutronix.de>]

Hi Ahmad,

On Thu, 2024-03-07 at 12:14 +0100, Ahmad Fatoum wrote:
> dma_map_single will do any necessary cache maintenance to make a buffer
> available to a device. Calling debug_dma_sync_single_for_device on such
> a buffer is unnecessary, so flag when this happens.


AFAIUI It is only incorrect if the buffer is handed of to the device
and never touched by the CPU again. If you want to modify a buffer
after the device has modified it to let the device work on it again,
dma_sync_single_for_device is the correct function.

In Essence:

Device Access -> dma_sync_single_for_cpu -> CPU modification ->
dma_sync_single_for_device -> Device Access

The buffer stays mapped to the deivce the whole time. Please correct me
if my understanding is wrong.

> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/dma/debug.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/debug.c b/drivers/dma/debug.c
> index b3bfbff9b2f5..b80e35ff5092 100644
> --- a/drivers/dma/debug.c
> +++ b/drivers/dma/debug.c
> @@ -12,6 +12,7 @@ struct dma_debug_entry {
>  	dma_addr_t       dev_addr;
>  	size_t           size;
>  	int              direction;
> +	bool             dev_mapped;
>  };
>  
>  static const char *dir2name[] = {
> @@ -121,6 +122,7 @@ void debug_dma_map(struct device *dev, void
> *addr,
>  	entry->dev_addr = dev_addr;
>  	entry->size = size;
>  	entry->direction = direction;
> +	entry->dev_mapped = true;
>  
>  	list_add(&entry->list, &dma_mappings);
>  
> @@ -159,9 +161,17 @@ void debug_dma_sync_single_for_cpu(struct device
> *dev,
>  	struct dma_debug_entry *entry;
>  
>  	entry = dma_debug_entry_find(dev, dma_handle, size);
> -	if (!entry)
> +	if (!entry) {
>  		dma_dev_warn(dev, "sync for CPU of never-mapped %s
> buffer 0x%llx+0x%zx!\n",
>  			     dir2name[direction], (u64)dma_handle,
> size);
> +		return;
> +	}
> +
> +	if (!entry->dev_mapped)
> +		dma_dev_warn(dev, "unexpected sync for CPU of
> already CPU-mapped %s buffer 0x%llx+0x%zx!\n",
> +			     dir2name[direction], (u64)dma_handle,
> size);
> +
> +	entry->dev_mapped = false;
>  }
>  
>  void debug_dma_sync_single_for_device(struct device *dev,
> @@ -177,7 +187,15 @@ void debug_dma_sync_single_for_device(struct
> device *dev,
>  	 * corruption
>  	 */
>  	entry = dma_debug_entry_find(dev, dma_handle, size);
> -	if (!entry)
> +	if (!entry) {
>  		dma_dev_warn(dev, "Syncing for device of never-
> mapped %s buffer 0x%llx+0x%zx!\n",
>  			     dir2name[direction], (u64)dma_handle,
> size);
> +		return;
> +	}
> +
> +	if (entry->dev_mapped)
> +		dma_dev_warn(dev, "unexpected sync for device of
> already device-mapped %s buffer 0x%llx+0x%zx!\n",
> +			     dir2name[direction], (u64)dma_handle,
> size);
> +
> +	entry->dev_mapped = true;
>  }