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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 07.03.24 12:20, Rouven Czerwinski wrote:
> 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.

Your understanding is correct and my patch shouldn't preclude using
the DMA API that way. What it flags is doing a sync for CPU or a sync
for device twice in a row without an intervening sync into the
inverse direction.

Cheers,
Ahmad

> 
>> 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;
>>  }
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |