From: Roy Pledge <roy.pledge@nxp.com>
To: Arnd Bergmann <arnd@arndb.de>, Christoph Hellwig <hch@lst.de>,
        "Marek Szyprowski" <m.szyprowski@samsung.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Robin Murphy <robin.murphy@arm.com>,
        Vladimir Murzin <vladimir.murzin@arm.com>,
        Mark Craske <Mark_Craske@mentor.com>,
        "George G. Davis" <george_davis@mentor.com>,
        Bastian Hecht <bhecht@de.adit-jv.com>,
        "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: dma-coherent: fix dma_declare_coherent_memory() logic error
Thread-Topic: dma-coherent: fix dma_declare_coherent_memory() logic error
Thread-Index: AQHTLZxR9YYzCtVhcUawp57Wd1yR4Q==
Date: Thu, 14 Sep 2017 20:59:16 +0000
Message-ID: <DB6PR04MB2999F7968A4C1804E5C1EB80866F0@DB6PR04MB2999.eurprd04.prod.outlook.com>
References: <20170905081026.1198405-1-arnd@arndb.de>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Sep 2017 20:59:16.3491
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR04MB3125
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8EKxPST002614
Content-Length: 3906
Lines: 116

On 9/5/2017 4:10 AM, Arnd Bergmann wrote:
> A recent change interprets the return code of dma_init_coherent_memory
> as an error value, but it is instead a boolean, where 'true' indicates
> success. This leads causes the caller to always do the wrong thing,
> and also triggers a compile-time warning about it:
> 
> drivers/base/dma-coherent.c: In function 'dma_declare_coherent_memory':
> drivers/base/dma-coherent.c:99:15: error: 'mem' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> I ended up changing the code a little more, to give use the usual
> error handling, as this seemed the best way to fix up the warning
> and make the code look reasonable at the same time.
> 
> Fixes: 2436bdcda53f ("dma-coherent: remove the DMA_MEMORY_MAP and DMA_MEMORY_IO flags")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/base/dma-coherent.c | 38 +++++++++++++++++++++++++-------------
>   1 file changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
> index f82a504583d4..a39b2166b145 100644
> --- a/drivers/base/dma-coherent.c
> +++ b/drivers/base/dma-coherent.c
> @@ -37,7 +37,7 @@ static inline dma_addr_t dma_get_device_base(struct device *dev,
>   		return mem->device_base;
>   }
>   
> -static bool dma_init_coherent_memory(
> +static int dma_init_coherent_memory(
>   	phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags,
>   	struct dma_coherent_mem **mem)
>   {
> @@ -45,20 +45,28 @@ static bool dma_init_coherent_memory(
>   	void __iomem *mem_base = NULL;
>   	int pages = size >> PAGE_SHIFT;
>   	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> +	int ret;
>   
> -	if (!size)
> +	if (!size) {
> +		ret = -EINVAL;
>   		goto out;
> +	}
>   
>   	mem_base = memremap(phys_addr, size, MEMREMAP_WC);
> -	if (!mem_base)
> +	if (!mem_base) {
> +		ret = -EINVAL;
>   		goto out;
> -
> +	}
>   	dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL);
> -	if (!dma_mem)
> +	if (!dma_mem) {
> +		ret = -ENOMEM;
>   		goto out;
> +	}
>   	dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> -	if (!dma_mem->bitmap)
> +	if (!dma_mem->bitmap) {
> +		ret = -ENOMEM;
>   		goto out;
> +	}
>   
>   	dma_mem->virt_base = mem_base;
>   	dma_mem->device_base = device_addr;
> @@ -68,13 +76,13 @@ static bool dma_init_coherent_memory(
>   	spin_lock_init(&dma_mem->spinlock);
>   
>   	*mem = dma_mem;
> -	return true;
> +	return 0;
>   
>   out:
>   	kfree(dma_mem);
>   	if (mem_base)
>   		memunmap(mem_base);
> -	return false;
> +	return ret;
>   }
>   
>   static void dma_release_coherent_memory(struct dma_coherent_mem *mem)
> @@ -338,14 +346,18 @@ static struct reserved_mem *dma_reserved_default_memory __initdata;
>   static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev)
>   {
>   	struct dma_coherent_mem *mem = rmem->priv;
> +	int ret;
> +
> +	if (!mem)
> +		return -ENODEV;

When I picked up this change my use of of_reserved_mem_device_init() 
broke. The only place rmem->priv is set is in this function (bottom of 
the patch) so the !mem check above will always fail. Am I missing 
something? It seems to me the intent here was to only call 
dma_init_coherent_memory() once and use rmem->priv as a cache for future 
calls but I'm just looking at the implemation of this for the first time.

>   
> -	if (!mem &&
> -	    !dma_init_coherent_memory(rmem->base, rmem->base, rmem->size,
> -				      DMA_MEMORY_EXCLUSIVE,
> -				      &mem)) {
> +	ret = dma_init_coherent_memory(rmem->base, rmem->base, rmem->size,
> +				       DMA_MEMORY_EXCLUSIVE, &mem);
> +
> +	if (ret) {
>   		pr_err("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n",
>   			&rmem->base, (unsigned long)rmem->size / SZ_1M);
> -		return -ENODEV;
> +		return ret;
>   	}
>   	mem->use_dev_dma_pfn_offset = true;
>   	rmem->priv = mem;
>