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.fat...@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;
> > > }
> >
> >
>
