Re: [RFC PATCH v6 1/6] riscv: mm: dma-noncoherent: Switch using function pointers for cache management

From: Christoph Hellwig <hch@lst.de>
To: Arnd Bergmann <arnd@arndb.de>
Cc: "Christoph Hellwig" <hch@lst.de>,
	Prabhakar <prabhakar.csengg@gmail.com>,
	"Conor.Dooley" <conor.dooley@microchip.com>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Heiko Stübner" <heiko@sntech.de>, guoren <guoren@kernel.org>,
	"Andrew Jones" <ajones@ventanamicro.com>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"open list:RISC-V ARCHITECTURE" <linux-riscv@lists.infradead.org>,
	"open list" <linux-kernel@vger.kernel.org>,
	devicetree@vger.kernel.org,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"Philipp Tomsich" <philipp.tomsich@vrull.eu>,
	"Nathan Chancellor" <nathan@kernel.org>,
	"Atish Patra" <atishp@rivosinc.com>,
	"Anup Patel" <apatel@ventanamicro.com>,
	"Tsukasa OI" <research_trasio@irq.a4lg.com>,
	"Jisheng Zhang" <jszhang@kernel.org>,
	"Mayuresh Chitale" <mchitale@ventanamicro.com>,
	"Will Deacon" <will@kernel.org>
Subject: Re: [RFC PATCH v6 1/6] riscv: mm: dma-noncoherent: Switch using function pointers for cache management
Date: Fri, 13 Jan 2023 06:48:07 +0100	[thread overview]
Message-ID: <20230113054807.GA23179@lst.de> (raw)
In-Reply-To: <02988e70-b099-46fd-b260-2d537c50543a@app.fastmail.com>

On Tue, Jan 10, 2023 at 04:03:06PM +0100, Arnd Bergmann wrote:
> To clarify: I was looking at arch_sync_dma_for_device(), which
> changed from 'invalidate' to 'clean' last June in commit
> c50f11c6196f ("arm64: mm: Don't invalidate FROM_DEVICE buffers
> at start of DMA transfer"). I don't see  a revert for that.
> 
> The one that was reverted recently is arch_dma_prep_coherent, which
> was changed and reverted in
> 
> c44094eee32 Aug 23 2022 flush->clean
> b7d9aae4048 Dec 6  2022 clean->flush
> 
> I'm primarily interested in the streaming mappings (arch_sync_*)
> at the moment.

Oh, true.  I've been confused about the two changes.

> Agreed, that's why I particularly don't like the idea of giving SoC
> specific L2$ drivers the option of making custom choices here.

Yes, moving this into individual drivers is an absolute no-go. 

> The arch_dma_prep_coherent() change is arguably arm64 specific
> because it is only needed for machines sharing memory with EL3
> firmware that enforces buffer ownership, but even for that it would
> be nice to have a central definition and some architecture specific
> flag that picks one behavior or the other. Same thing for the
> speculative access difference.

Yes.

> I looked at all the implementations now and put them in a table[1]
> to see what the differences are. The only bit that I think needs
> discussion is the dma_sync_single_for_device(DMA_FROM_DEVICE) op
> that I mentioned above. I see that arm64, csky, powerpc, riscv
> and parisc all write out at least partical cache lines first to
> avoid losing dirty data in the part that is not written by the
> device[2][3], while the other ones don't[4]. 

I'm tempted to declare [4] buggy until proof of the inverse.

> I also see that at least arc, arm, mips and riscv all want
> CPU specific cache management operations to be registered
> at boot time. While Russell had some concerns about your
> suggestion to generalize the arm version, we could start
> by moving the abstracted riscv version into
> kernel/dma/direct.c and make sure it can be shared with
> at least mips and arc, provided that we can agree on the
> DMA_FROM_DEVICE behavior.

Yes, I'd really like to start out with a common version and then
move bits over.  There's also some interesting bits about handling
highmem for architectures that needs virtual addresss for flushing
that might be worth sharing, too.

Thіs should be a new file in kernel/dma/ as it's not only used by
dma-direct but also by dma-iommu, and to keep the code nicely
separated.

Can you give it a go?