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

[Sergei Miroshnichenko]

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.

Serge

> 
> Jess
> 
> > [1] dma_sync_*_for_cpu and direction=TO_DEVICE
> > Link: https://lkml.org/lkml/2018/5/18/979
> > 
> > [2] Cache maintenance for non-coherent DMA in
> > arch_sync_dma_for_device()
> > Link:
> > https://lore.kernel.org/all/20220606152150.GA31568@willie-the-truck/
> > 
> > [3] commit c50f11c6196f ("arm64: mm: Don't invalidate FROM_DEVICE
> > buffers at start of DMA transfer")
> > 
> > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices
> > using zicbom extension")
> > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko at yadro.com>
> > ---
> > arch/riscv/mm/dma-noncoherent.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-
> > noncoherent.c
> > index cd2225304c82..2a8f6124021f 100644
> > --- a/arch/riscv/mm/dma-noncoherent.c
> > +++ b/arch/riscv/mm/dma-noncoherent.c
> > @@ -45,7 +45,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr,
> > size_t size,
> >               break;
> >       case DMA_FROM_DEVICE:
> >       case DMA_BIDIRECTIONAL:
> > -             ALT_CMO_OP(flush, vaddr, size,
> > riscv_cbom_block_size);
> > +             ALT_CMO_OP(inval, vaddr, size,
> > riscv_cbom_block_size);
> >               break;
> >       default:
> >               break;
> > --
> > 2.37.2
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>