Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Marcin Wojtas <mw@semihalf.com>
Cc: "Marek Behún" <kabel@kernel.org>,
	pali@kernel.org, "Christoph Hellwig" <hch@lst.de>,
	"Robin Murphy" <robin.murphy@arm.com>,
	"Arnd Bergmann" <arnd@kernel.org>,
	"Andre Przywara" <andre.przywara@arm.com>,
	"Marc Zyngier" <maz@kernel.org>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Gregory Clement" <gregory.clement@bootlin.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	iommu@lists.linux-foundation.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
Date: Tue, 4 Oct 2022 08:25:09 +0100	[thread overview]
Message-ID: <Yzvf1TNJebkJic/s@shell.armlinux.org.uk> (raw)
In-Reply-To: <CAPv3WKcggJ3ki+8DQyA=P=+d714kYxA2-M5xtZf9fEENkk5osQ@mail.gmail.com>

On Mon, Oct 03, 2022 at 11:30:31PM +0200, Marcin Wojtas wrote:
> I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> the driver - the CPU doesn't access the payload afterward.

Please look at the bigger picture rather than concentrating on what
happens when a packet is received.

The issue is that the CPU writes to the buffer prior to handing the
buffer over to the hardware, and then the BM reads from the buffer.
This is DMA _to_ the device no matter how you look at it.

The BM later writes the received packet to the buffer. This is DMA
_from_ the device.

So, we have DMA in both directions, hence it really is bidirectional,
and using DMA_FROM_DEVICE in the RX path _is_ incorrect.

Architectures _can_ and _do_ invalidate the data cache when mapping a
buffer for DMA_FROM_DEVICE and any writes in the data cache for that
buffer will be discarded. If the writes don't hit the data cache, then
they will be unaffected by this.

IMHO, having read the docs on the buffer manager, the use of
DMA_FROM_DEVICE in mvneta in this path is a long-standing bug - it
should be bidirectional for the reason I state above - the hardware
both reads and writes the buffer that is passed to it, expecting to
read data written by the CPU initially.

> Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?

That will make no difference to this - this is not about barriers, it's
about caches.

> I have one overall concern here. On all kinds of A38x-based boards I
> worked on, by default, the firmware set all devices (e.g. network,
> AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> used). Can you please try adding 'dma-coherent;' property under the
> 'internal-regs' node?

I did notice that there was no dma-coherent markings in DT, which means
that the DMA API will assume the device is non-coherent, and cache
maintenance will be performed. If it is dma-coherent, then cache
maintenance won't be performed, and DT needs to be updated to indicate
this.

If firmware is making the devices DMA coherent, and it's under firmware
control, then shouldn't firmware also be updating the kernel's device
tree to indicate how it's configured the hardware coherency?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel