On 2022-03-25 15:25, Halil Pasic wrote:
On Thu, 24 Mar 2022 19:02:16 +0100 Halil Pasic pasic@linux.ibm.com wrote:
I'll admit I still never quite grasped the reason for also adding the override to swiotlb_sync_single_for_device() in aa6f8dcbab47, but I think by that point we were increasingly tired and confused and starting to second-guess ourselves (well, I was, at least).
I raised the question, do we need to do the same for swiotlb_sync_single_for_device(). Did that based on my understanding of the DMA API documentation. I had the following scenario in mind
SWIOTLB without the snyc_single: Memory Bounce buffer Owner
start 12345678 xxxxxxxx C dma_map(DMA_FROM_DEVICE) 12345678 -> 12345678 C->D device writes partial data 12345678 12ABC678 <- ABC D sync_for_cpu(DMA_FROM_DEVICE) 12ABC678 <- 12ABC678 D->C cpu modifies buffer 66666666 12ABC678 C sync_for_device(DMA_FROM_DEVICE) 66666666 12ABC678 C->D device writes partial data 66666666 1EFGC678 <-EFG D dma_unmap(DMA_FROM_DEVICE) 1EFGC678 <- 1EFGC678 D->C
Legend: in Owner column C stands for cpu and D for device.
Without swiotlb, I believe we should have arrived at 6EFG6666. To get the same result, IMHO, we need to do a sync in sync_for_device(). And aa6f8dcbab47 is an imperfect solution to that (because of size).
@Robin, Christoph: Do we consider this a valid scenario?
Aha, I see it now (turns out diagrams really do help!) - so essentially the original situation but with buffer recycling thrown into the mix as well... I think it's technically valid, but do we know if anything's actually doing that in a way which ends up affected? For sure it would be nice to know that we had all bases covered without having to audit whether we need to, but if it's fundamentally incompatible with what other code expects, that we know *is* being widely used, and however questionable it may be we don't have an easy fix for, then we're in a bit of a tough spot :(
Thanks, Robin.