Re: [PATCH 1/1] riscv: set ARCH_DMA_DEFAULT_COHERENT if RISCV_DMA_NONCOHERENT is not set

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]

On Fri, Dec 22, 2023 at 05:39:34PM +0300, Maxim Kochetkov wrote:
> 
> 
> On 22.12.2023 07:14, Christoph Hellwig wrote:
> > On Thu, Dec 21, 2023 at 10:27:33PM +0000, Jiaxun Yang wrote:
> > > 
> > > 
> > > 在 2023/12/21 20:29, Conor Dooley 写道:
> > > > + Christoph
> > > > 
> > > > I don't think this patch is correct. Regardless of whether we support
> > > > cache management operations, DMA is assumed to be coherent unless
> > > > peripherals etc are specified to otherwise in DT (or however ACPI deals
> > > > with that kind of thing).
> > > > 
> > > > What problem are you trying to solve here?
> > > > 
> > > > On Thu, Dec 21, 2023 at 09:51:52PM +0300, Maxim Kochetkov wrote:
> > > > > Not all the RISCV are DMA coherent by default.
> > > 
> > > Sorry for chime in here.
> > > IMO if your platform is not coherent by default, just insert
> > > "dma-noncoherent"
> > > at devicetree root node.
> > 
> > Exactly.  ARCH_DMA_DEFAULT_COHERENTis a setting that just says for
> > a given architecture assumes coherent unless otherwise specified,
> > which has historically been the case for mips.  Not setting it means
> > non-coherent unless specified, which has historially been the case
> > for arm.
> > 
> > RISC-V starte out without support for non-coherent DMA, and high ups
> > in RISCV still told me in 2019 that RISC-V doesn't need cache
> > management instructions because no new hardware would ever not be
> > dma coherent.  Yeah, right..
> > 
> > Anyay, Linux for RISC-V has historically been coherent only and then
> > coherent default, so this option is wrong, and you need to mark
> > you platform as non-coherent by inserting dma-noncoherent somewhere.
> > 
> Conor, Christoph, Jiaxun, thanks for quick feedback!
> 
> The problem is very simple:
> For non mips platforms dma_default_coherent is used at of_dma_is_coherent()
> and device_initialize().
> of_dma_is_coherent() affects only DT devices. And we can override it with
> "dma-coherent"/"dma-noncoherent". ACPI devices can specify by
> "attr == DEV_DMA_COHERENT". But all other devices (platform_device, usb,

I would have expected that usb devices "inherit" the value from the usb
controller whose bus they are on. Similarly, platform devices are on a
bus that should be marked as non-coherent if that is the case.
Christoph certainly knows better how things operate here however.

> etc..) do not have this feature. These devices will use value from
> device_initialize(). And we have no possibility to change
> dma_default_coherent value by disabling ARCH_DMA_DEFAULT_COHERENT.
> Moreover, changing dma_default_coherent from false to true may cause
> regression for other devices.

How can there be a regression when dma has been coherent by default for
the RISC-V kernel from day 1?

> That is why I suggest possibility to disable
> ARCH_DMA_DEFAULT_COHERENT by RISCV_DMA_NONCOHERENT option.
> It will works like for PPC:
> .....
> config PPC
> 	bool
> 	default y
> 	#
> 	# Please keep this list sorted alphabetically.
> 	#
> 	select ARCH_32BIT_OFF_T if PPC32
> 	select ARCH_DISABLE_KASAN_INLINE	if PPC_RADIX_MMU
> 	select ARCH_DMA_DEFAULT_COHERENT	if !NOT_COHERENT_CACHE
> .....
> Doesn't the option RISCV_DMA_NONCOHERENT say the DMA should be non-coherent
> by default?

No. That option only controls whether or not support for cache
management operations is built into the kernel. It is expected that this
option can be enabled for multiplatform kernels, like those used by
distributions, that will run on systems that are entirely coherent as
well as those that are not.
It does not work the same was as this PPC option appears to.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]