On 2022-03-25 15:25, Halil Pasic wrote:
On Thu, 24 Mar 2022 19:02:16 +0100
Halil Pasic <pasic@xxxxxxxxxxxxx> 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.
