Subject: Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool
To: Christoph Hellwig <hch@infradead.org>
References: <1495621472-9323-1-git-send-email-vladimir.murzin@arm.com>
 <1495621472-9323-5-git-send-email-vladimir.murzin@arm.com>
 <20170620134932.GB31496@infradead.org>
Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux@armlinux.org.uk, sza@esh.hu, arnd@arndb.de,
        gregkh@linuxfoundation.org, akpm@linux-foundation.org,
        alexandre.torgue@st.com, robin.murphy@arm.com,
        benjamin.gaignard@linaro.org, kbuild-all@01.org,
        Michal Nazarewicz <mina86@mina86.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>
From: Vladimir Murzin <vladimir.murzin@arm.com>
Message-ID: <94e57caa-e7b9-d740-bca3-89950c10033e@arm.com>
Date: Thu, 22 Jun 2017 14:18:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.0
MIME-Version: 1.0
In-Reply-To: <20170620134932.GB31496@infradead.org>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1905
Lines: 48

On 20/06/17 14:49, Christoph Hellwig wrote:
> On Wed, May 24, 2017 at 11:24:29AM +0100, Vladimir Murzin wrote:
>> This patch introduces default coherent DMA pool similar to default CMA
>> area concept. To keep other users safe code kept under CONFIG_ARM.
> 
> I don't see a CONFIG_ARM in the code, although parts of it are added
> under CONFIG_OF_RESERVED_MEM.

It should look like that:

#ifdef CONFIG_ARM
	if (!of_get_flat_dt_prop(node, "no-map", NULL)) {
		pr_err("Reserved memory: regions without no-map are not yet supported\n");
		return -EINVAL;
	}
+
+	if (of_get_flat_dt_prop(node, "linux,dma-default", NULL)) {
+		WARN(dma_reserved_default_memory,
+		     "Reserved memory: region for default DMA coherent area is redefined\n");
+		dma_reserved_default_memory = rmem;
+	}
#endif


> 
> But overall this code look a bit odd to me.  As far as I can tell
> the dma-coherent.c code is for the case where we have a special
> piece of coherent memory close to a device.
> 
> If you're allocating out of the global allocator the memory should
> come from the normal dma_ops ->alloc allocator - and also take
> the attrs into account (e.g. for DMA_ATTR_NON_CONSISTENT or
> DMA_ATTR_NO_KERNEL_MAPPING requests you don't need coherent memory)
> 

It is how it has been started [1] - defining memory which is not cacheable
(i.e. suitable for coherent allocations) and building custom allocator on top
of it, like it was done for c6x and blackfin. The annoying thing was that we
needed to advertise such memory via command line parameter plus some "mem="
adjustment to hide coherent memory from buddy allocator. So it was suggested
to use reserved memory and this makes things look much better, but on the
other hand require changes on dts side to "bind" devices with reserved memory
- default DMA pool removes such drawback.

[1] https://marc.info/?l=linux-arm-kernel&m=148163694824475&w=2

Cheers
Vladimir