On Tue, Mar 28, 2023, at 00:25, Christoph Hellwig wrote:

quoted

+static inline void arch_dma_cache_wback(phys_addr_t paddr, size_t size)
 {
+	dma_cache_wback(paddr, size);
+}
 
+static inline void arch_dma_cache_inv(phys_addr_t paddr, size_t size)
+{
+	dma_cache_inv(paddr, size);
 }

quoted

+static inline void arch_dma_cache_wback_inv(phys_addr_t paddr, size_t size)
 {
+	dma_cache_wback_inv(paddr, size);
+}

There are the only calls for the three functions for each of the
involved functions.  So I'd rather rename the low-level symbols
(and drop the pointless exports for two of them) rather than adding
these wrapppers.

The same is probably true for many other architectures.

Ok, done that now.

quoted

+static inline bool arch_sync_dma_clean_before_fromdevice(void)
+{
+	return false;
+}
 
+static inline bool arch_sync_dma_cpu_needs_post_dma_flush(void)
+{
+	return true;
 }

Is there a way to cut down on this boilerplate code by just having
sane default, and Kconfig options to override them if they are not
runtime decisions?

I've changed arch_sync_dma_clean_before_fromdevice() to a
Kconfig symbol now, as this is never a runtime decision.

For arch_sync_dma_cpu_needs_post_dma_flush(), I have this
version now in common code, which lets mips and arm have
their own logic and has the same effect elsewhere:

+#ifndef arch_sync_dma_cpu_needs_post_dma_flush
+static inline bool arch_sync_dma_cpu_needs_post_dma_flush(void)
+{
+       return IS_ENABLED(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU);
+}
+#endif

quoted

+#include <linux/dma-sync.h>

I can't really say I like the #include version here despite your
rationale in the commit log.  I can probably live with it if you
think it is absolutely worth it, but I'm really not in favor of it.

quoted

+config ARCH_DMA_MARK_DCACHE_CLEAN
+	def_bool y

What do we need this symbol for?  Unless I'm missing something it is
always enable for arm32, and only used in arm32 code.

This was left over from an earlier draft and accidentally duplicates
the thing that I have in the Arm version for the existing
ARCH_HAS_DMA_MARK_CLEAN. I dropped this one and the
generic copy of the arch_dma_mark_dcache_clean() function
now, but still need to revisit the arm version, as it sounds
like it has slightly different semantics from the ia64 version.

     Arnd