[PATCH] dma: debug: detect repeated DMA sync

[PATCH] dma: debug: detect repeated DMA sync

[Ahmad Fatoum]

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.

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

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

[Rouven Czerwinski]

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

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

[Ahmad Fatoum]

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 |

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

[Lucas Stach]

Am Donnerstag, dem 07.03.2024 um 13:27 +0100 schrieb Ahmad Fatoum:
> 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.
> 
Yep, the name of the property is a bit confusing, as it's not really
telling if an entry is dma mapped, but rather in which domain CPU/DEV
the entry currently resides. All memory regions start out in the CPU
domain and dma_map/dma_sync_for_device push them into the device
domain, while dma_unmap/dma_sync_for_cpu pull them back into the CPU
domain.

So maybe make this a enum of domains or at least rename to dev_owned or
something along those lines.

Regards,
Lucas

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