[PATCH] dma-coherent: fix dma_declare_coherent_memory() logic error

[PATCH] dma-coherent: fix dma_declare_coherent_memory() logic error

[Arnd Bergmann]

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;
 
-	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;
-- 
2.9.0

Re: [PATCH] dma-coherent: fix dma_declare_coherent_memory() logic error

[Christoph Hellwig]

Thanks Arnd,

applied.

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

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

[Arnd Bergmann]

>> @@ -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.

Yes, you are right, I misread that code.  I think we need this fixup on top
to restore the previous behavior:

diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
index a39b2166b145..744f64f43454 100644
--- a/drivers/base/dma-coherent.c
+++ b/drivers/base/dma-coherent.c
@@ -348,16 +348,15 @@ 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;
-
-       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 ret;
+       if (!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 ret;
+               }
        }
        mem->use_dev_dma_pfn_offset = true;
        rmem->priv = mem;

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

[Arnd Bergmann]

>> @@ -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.

Yes, you are right, I misread that code.  I think we need this fixup on top
to restore the previous behavior:

diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
index a39b2166b145..744f64f43454 100644
--- a/drivers/base/dma-coherent.c
+++ b/drivers/base/dma-coherent.c
@@ -348,16 +348,15 @@ 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;
-
-       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 ret;
+       if (!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 ret;
+               }
        }
        mem->use_dev_dma_pfn_offset = true;
        rmem->priv = mem;