Re: dma-coherent: fix dma_declare_coherent_memory() logic error

[Roy Pledge]

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@xxxxxxxx>
> ---
>   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;
>