* [PATCH] dma: debug: detect repeated DMA sync @ 2024-03-07 11:14 Ahmad Fatoum 2024-03-07 11:20 ` Rouven Czerwinski 0 siblings, 1 reply; 4+ messages in thread From: Ahmad Fatoum @ 2024-03-07 11:14 UTC (permalink / raw) To: barebox; +Cc: rcz, lst, 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dma: debug: detect repeated DMA sync 2024-03-07 11:14 [PATCH] dma: debug: detect repeated DMA sync Ahmad Fatoum @ 2024-03-07 11:20 ` Rouven Czerwinski 2024-03-07 12:27 ` Ahmad Fatoum 0 siblings, 1 reply; 4+ messages in thread From: Rouven Czerwinski @ 2024-03-07 11:20 UTC (permalink / raw) To: Ahmad Fatoum, barebox; +Cc: rcz, lst 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; > } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dma: debug: detect repeated DMA sync 2024-03-07 11:20 ` Rouven Czerwinski @ 2024-03-07 12:27 ` Ahmad Fatoum 2024-03-07 15:54 ` Lucas Stach 0 siblings, 1 reply; 4+ messages in thread From: Ahmad Fatoum @ 2024-03-07 12:27 UTC (permalink / raw) To: Rouven Czerwinski, barebox; +Cc: rcz, lst 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 | ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dma: debug: detect repeated DMA sync 2024-03-07 12:27 ` Ahmad Fatoum @ 2024-03-07 15:54 ` Lucas Stach 0 siblings, 0 replies; 4+ messages in thread From: Lucas Stach @ 2024-03-07 15:54 UTC (permalink / raw) To: Ahmad Fatoum, Rouven Czerwinski, barebox 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; > > > } > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-03-07 15:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-07 11:14 [PATCH] dma: debug: detect repeated DMA sync Ahmad Fatoum 2024-03-07 11:20 ` Rouven Czerwinski 2024-03-07 12:27 ` Ahmad Fatoum 2024-03-07 15:54 ` Lucas Stach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox