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 <[email protected]>
> ---
> 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;
>
>> @@ -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;