Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP

[Robin Murphy <robin.murphy@arm.com>]

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.