[PATCH] firmware: qcom_scm: use correct parameter type for dma_alloc_coherent

[PATCH] firmware: qcom_scm: use correct parameter type for dma_alloc_coherent

[Benjamin Gaignard]

dma_alloc_coherent expects it third argument type to be dma_addr_t and
not phys_addr_t.
When phys_addr_t is defined as u32 and dma_addr_t as u64 that generate
a compilation issue.
Change the variable name because dma_alloc_coherent returns a bus
address not a physical address.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/firmware/qcom_scm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index af4c75217ea6..a405a540c464 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -448,7 +448,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	struct qcom_scm_mem_map_info *mem_to_map;
 	phys_addr_t mem_to_map_phys;
 	phys_addr_t dest_phys;
-	phys_addr_t ptr_phys;
+	dma_addr_t handle;
 	size_t mem_to_map_sz;
 	size_t dest_sz;
 	size_t src_sz;
@@ -466,7 +466,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) +
 			ALIGN(dest_sz, SZ_64);
 
-	ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL);
+	ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &handle, GFP_KERNEL);
 	if (!ptr)
 		return -ENOMEM;
 
@@ -480,14 +480,14 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 
 	/* Fill details of mem buff to map */
 	mem_to_map = ptr + ALIGN(src_sz, SZ_64);
-	mem_to_map_phys = ptr_phys + ALIGN(src_sz, SZ_64);
+	mem_to_map_phys = handle + ALIGN(src_sz, SZ_64);
 	mem_to_map[0].mem_addr = cpu_to_le64(mem_addr);
 	mem_to_map[0].mem_size = cpu_to_le64(mem_sz);
 
 	next_vm = 0;
 	/* Fill details of next vmid detail */
 	destvm = ptr + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
-	dest_phys = ptr_phys + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
+	dest_phys = handle + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
 	for (i = 0; i < dest_cnt; i++) {
 		destvm[i].vmid = cpu_to_le32(newvm[i].vmid);
 		destvm[i].perm = cpu_to_le32(newvm[i].perm);
@@ -497,8 +497,8 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	}
 
 	ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz,
-				    ptr_phys, src_sz, dest_phys, dest_sz);
-	dma_free_coherent(__scm->dev, ALIGN(ptr_sz, SZ_64), ptr, ptr_phys);
+				    handle, src_sz, dest_phys, dest_sz);
+	dma_free_coherent(__scm->dev, ALIGN(ptr_sz, SZ_64), ptr, handle);
 	if (ret) {
 		dev_err(__scm->dev,
 			"Assign memory protection call failed %d.\n", ret);
-- 
2.15.0

Re: [PATCH] firmware: qcom_scm: use correct parameter type for dma_alloc_coherent

[Arnd Bergmann]

On Thu, Feb 8, 2018 at 1:21 PM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> dma_alloc_coherent expects it third argument type to be dma_addr_t and
> not phys_addr_t.
> When phys_addr_t is defined as u32 and dma_addr_t as u64 that generate
> a compilation issue.
> Change the variable name because dma_alloc_coherent returns a bus
> address not a physical address.
>

> @@ -480,14 +480,14 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>
>         /* Fill details of mem buff to map */
>         mem_to_map = ptr + ALIGN(src_sz, SZ_64);
> -       mem_to_map_phys = ptr_phys + ALIGN(src_sz, SZ_64);
> +       mem_to_map_phys = handle + ALIGN(src_sz, SZ_64);
>         mem_to_map[0].mem_addr = cpu_to_le64(mem_addr);
>         mem_to_map[0].mem_size = cpu_to_le64(mem_sz);
>
>         next_vm = 0;
>         /* Fill details of next vmid detail */
>         destvm = ptr + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
> -       dest_phys = ptr_phys + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
> +       dest_phys = handle + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);

This still assigns from a 'dma' address to a 'phys' address without a
long comment
explaining why you do it. There is also another instance of the naming confusion
in qcom_scm_pas_init_image and __qcom_scm_pas_init_image, so maybe they
should be addressed at the same time.

       Arnd

Re: [PATCH] firmware: qcom_scm: use correct parameter type for dma_alloc_coherent

[Benjamin Gaignard]

2018-02-08 15:09 GMT+01:00 Arnd Bergmann <arnd@arndb.de>:
> On Thu, Feb 8, 2018 at 1:21 PM, Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> dma_alloc_coherent expects it third argument type to be dma_addr_t and
>> not phys_addr_t.
>> When phys_addr_t is defined as u32 and dma_addr_t as u64 that generate
>> a compilation issue.
>> Change the variable name because dma_alloc_coherent returns a bus
>> address not a physical address.
>>
>
>> @@ -480,14 +480,14 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>>
>>         /* Fill details of mem buff to map */
>>         mem_to_map = ptr + ALIGN(src_sz, SZ_64);
>> -       mem_to_map_phys = ptr_phys + ALIGN(src_sz, SZ_64);
>> +       mem_to_map_phys = handle + ALIGN(src_sz, SZ_64);
>>         mem_to_map[0].mem_addr = cpu_to_le64(mem_addr);
>>         mem_to_map[0].mem_size = cpu_to_le64(mem_sz);
>>
>>         next_vm = 0;
>>         /* Fill details of next vmid detail */
>>         destvm = ptr + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
>> -       dest_phys = ptr_phys + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
>> +       dest_phys = handle + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
>
> This still assigns from a 'dma' address to a 'phys' address without a
> long comment
> explaining why you do it. There is also another instance of the naming confusion
> in qcom_scm_pas_init_image and __qcom_scm_pas_init_image, so maybe they
> should be addressed at the same time.

This patch just fix a compilation issue, nothing else.

>
>        Arnd

Re: [PATCH] firmware: qcom_scm: use correct parameter type for dma_alloc_coherent

[Benjamin Gaignard]

2018-02-08 15:26 GMT+01:00 Benjamin Gaignard <benjamin.gaignard@linaro.org>:
> 2018-02-08 15:09 GMT+01:00 Arnd Bergmann <arnd@arndb.de>:
>> On Thu, Feb 8, 2018 at 1:21 PM, Benjamin Gaignard
>> <benjamin.gaignard@linaro.org> wrote:
>>> dma_alloc_coherent expects it third argument type to be dma_addr_t and
>>> not phys_addr_t.
>>> When phys_addr_t is defined as u32 and dma_addr_t as u64 that generate
>>> a compilation issue.
>>> Change the variable name because dma_alloc_coherent returns a bus
>>> address not a physical address.
>>>
>>
>>> @@ -480,14 +480,14 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>>>
>>>         /* Fill details of mem buff to map */
>>>         mem_to_map = ptr + ALIGN(src_sz, SZ_64);
>>> -       mem_to_map_phys = ptr_phys + ALIGN(src_sz, SZ_64);
>>> +       mem_to_map_phys = handle + ALIGN(src_sz, SZ_64);
>>>         mem_to_map[0].mem_addr = cpu_to_le64(mem_addr);
>>>         mem_to_map[0].mem_size = cpu_to_le64(mem_sz);
>>>
>>>         next_vm = 0;
>>>         /* Fill details of next vmid detail */
>>>         destvm = ptr + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
>>> -       dest_phys = ptr_phys + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
>>> +       dest_phys = handle + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(src_sz, SZ_64);
>>
>> This still assigns from a 'dma' address to a 'phys' address without a
>> long comment
>> explaining why you do it. There is also another instance of the naming confusion
>> in qcom_scm_pas_init_image and __qcom_scm_pas_init_image, so maybe they
>> should be addressed at the same time.
>
> This patch just fix a compilation issue, nothing else.

Gentle ping on this patch since the problem still exist on 4.16-rc2

>
>>
>>        Arnd