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

[Lucas Stach <l.stach@pengutronix.de>]

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