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

[Jessica Clarke]

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).

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