[PATCH] riscv: dma-mapping: Use conventional cache operations for dma_sync*()

[Jessica Clarke]

On 18 Aug 2022, at 21:52, Sergei Miroshnichenko <s.miroshnichenko at yadro.com> wrote:
> 
> Hi Jess,
> 
> On Thu, 2022-08-18 at 19:12 +0100, Jessica Clarke wrote:
>> On 18 Aug 2022, at 17:51, Sergei Miroshnichenko
>> <s.miroshnichenko at yadro.com> wrote:
>>> 
>>> Support of arch_sync_dma_for_{device,cpu}() for RISC-V was added
>>> recently.
>>> 
>>> Currently, for_cpu(FROM_DEVICE) uses the flush (writeback plus
>>> invalidate)
>>> cache operation, which can overwrite the data (received from a
>>> device via
>>> DMA) with dirty cache lines. Replace it with the inval to avoid
>>> data
>>> corruptions.
>>> 
>>> The same goes for for_cpu(BIDIRECTIONAL), it was also flushing the
>>> cache,
>>> but it is called after the device had owned the buffer. So in order
>>> to not
>>> loose the received data, stick to the inval here as well.
>>> 
>>> As of for_device(FROM_DEVICE), according to the inspirational
>>> discussion
>>> about matching the DMA API and cache operations [1], the inval or a
>>> no-op
>>> should be here, and this would resonate with most other arches. But
>>> in a
>>> later discussion [2] it was decided that the clean (which is used
>>> now) is
>>> indeed preferable and safer, though slower. The arm64 has been
>>> recently
>>> changed in the same way [3].
>> 
>> Flush is always a valid way to implement invalidate, and if it does
>> not
>> work then you have bugs elsewhere, since the CPU is free to write out
>> the dirty cache lines at any point before an invalidate. So whilst
>> it’s
>> more efficient to use invalidate and hint to the core that it does
>> not
>> need to do any writing, it is no more correct, and this this patch
>> description is misguided. You even might wish to use the fact that it
>> is currently a flush as a convenient sanitiser for whatever is
>> currently violating the non-coherent DMA rules and fix that before
>> it’s
>> switched to invalidate and papers over the issue (at least in
>> whatever
>> implementation you’re running on).
> 
> You are right, thank you, the description *is* misleading: cache lines
> can't get dirty before for_cpu(), as they are cleaned during
> for_device(), and are not written to by the CPU in between.
> 
> I gave it another thought: the reason to invalidate in for_cpu() is
> that the CPU can speculatively prefetch the buffer after
> map()/for_device() but before the DMA is finished.
> 
> Even if replace the clean in for_device(FROM_DEVICE) with the flush, a
> prefetch still may happen, requiring an invalidation.

In that case the line is still clean, not dirty, so a flush will behave
the same as an invalidate. So no, this is still wrong, flush is always
correct, even if invalidate is potentially faster.

Jess