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@xxxxxxxxxxxxxx>
>> ---
>> 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 |
