On Fri, 25 Mar 2022 22:13:08 +0100 Johannes Berg johannes@sipsolutions.net wrote:
Then, however, we need to define what happens if you pass DMA_BIDIRECTIONAL to the sync_for_cpu() and sync_for_device() functions, which adds two more cases? Or maybe we eventually just think that's not valid at all, since you have to specify how you're (currently?) using the buffer, which can't be DMA_BIDIRECTIONAL?
Ugh. Do we actually have cases that do it?
Yes, a few.
That sounds really odd for a "sync" operation. It sounds very reasonable for _allocating_ DMA, but for syncing I'm left scratching my head what the semantics would be.
I agree.
But yes, if we do and people come up with semantics for it, those semantics should be clearly documented.
I'm not sure? I'm wondering if this isn't just because - like me initially - people misunderstood the direction argument, or didn't understand it well enough, and then just passed the same value as for the map()/unmap()?
I don't think you misunderstood the direction argument and its usage. I didn't finish thinking about the proposal of Linus, I'm pretty confident about my understanding of the current semantics of the direction. According to the documentation, you do have to pass in the very same direction, that was specified when the mapping was created. A qoute from the documentation.
""" void dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size, enum dma_data_direction direction)
void dma_sync_single_for_device(struct device *dev, dma_addr_t dma_handle, size_t size, enum dma_data_direction direction)
void dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction direction)
void dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction direction)
Synchronise a single contiguous or scatter/gather mapping for the CPU and device. With the sync_sg API, all the parameters must be the same as those passed into the single mapping API. With the sync_single API, you can use dma_handle and size parameters that aren't identical to those passed into the single mapping API to do a partial sync. """ (Documentation/core-api/dma-api.rst)
The key here is "sync_sg API, all the parameters must be the same as those passed into the single mapping API", but I have to admit, I don't understand the *single* in here. The intended meaning of the last sentence is that one can do partial sync by choose dma_hande_sync, size_sync such that dma_handle_mapping <= dma_handle_sync < dma_handle_mapping + size_mapping and dma_handle_sync + size_sync <= dma_handle_mapping + size_mapping. But the direction has to remain the same.
BTW, the current documented definition of the direction is about the data transfer direction between memory and the device, and how the CPU is interacting with the memory is not in scope. A quote form the documentation.
""" ======================= ============================================= DMA_NONE no direction (used for debugging) DMA_TO_DEVICE data is going from the memory to the device DMA_FROM_DEVICE data is coming from the device to the memory DMA_BIDIRECTIONAL direction isn't known ======================= ============================================= """ (Documentation/core-api/dma-api.rst)
My feeling is, that re-defining the dma direction is not a good idea. But I don't think my opinion has much weight here.
@Christoph, Robin: What do you think?
Regards, Halil