qcom_scm: Incompatible pointer type build failure

qcom_scm: Incompatible pointer type build failure

[Julien Grall]

Hi Ian,

Thank you for the report.

On 30/04/2019 13:44, Ian Jackson wrote:
> osstest service owner writes ("[linux-4.19 test] 135420: regressions - FAIL"):
>> flight 135420 linux-4.19 real [real]
>> http://logs.test-lab.xenproject.org/osstest/logs/135420/
>>
>> Regressions :-(
>>
>> Tests which did not succeed and are blocking,
>> including tests which could not be run:
>>   build-armhf-pvops             6 kernel-build             fail REGR. vs. 129313
> 
> http://logs.test-lab.xenproject.org/osstest/logs/135420/build-armhf-pvops/6.ts-kernel-build.log
> 
>    drivers/firmware/qcom_scm.c: In function ‘qcom_scm_assign_mem’:
>    drivers/firmware/qcom_scm.c:469:47: error: passing argument 3 of ‘dma_alloc_coherent’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>      ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL);
>                                                   ^
>    In file included from drivers/firmware/qcom_scm.c:21:0:
>    ./include/linux/dma-mapping.h:560:21: note: expected ‘dma_addr_t * {aka long long unsigned int *}’ but argument is of type ‘phys_addr_t * {aka unsigned int *}’
>     static inline void *dma_alloc_coherent(struct device *dev, size_t size,
>                         ^~~~~~~~~~~~~~~~~~
>    cc1: some warnings being treated as errors
>    scripts/Makefile.build:303: recipe for target 'drivers/firmware/qcom_scm.o' failed
>    make[2]: *** [drivers/firmware/qcom_scm.o] Error 1
>    scripts/Makefile.build:544: recipe for target 'drivers/firmware' failed
>    make[1]: *** [drivers/firmware] Error 2
>    make[1]: *** Waiting for unfinished jobs....
> 
> I think this build failure is probably a regression; rather it is due
> to the stretch update which brings in a new compiler.

The bug has always been present (and still present in master), it is possible 
the compiler became smarter with the upgrade to stretch.

The problem is similar to [1] and happen when the size of phys_addr_t is 
different to dma_addr_t.

I have CCed the maintainers of this file.

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-04/msg00940.html

-- 
Julien Grall

qcom_scm: Incompatible pointer type build failure

[Julien Grall]

Hi Ian,

Thank you for the report.

On 30/04/2019 13:44, Ian Jackson wrote:
> osstest service owner writes ("[linux-4.19 test] 135420: regressions - FAIL"):
>> flight 135420 linux-4.19 real [real]
>> http://logs.test-lab.xenproject.org/osstest/logs/135420/
>>
>> Regressions :-(
>>
>> Tests which did not succeed and are blocking,
>> including tests which could not be run:
>>   build-armhf-pvops             6 kernel-build             fail REGR. vs. 129313
> 
> http://logs.test-lab.xenproject.org/osstest/logs/135420/build-armhf-pvops/6.ts-kernel-build.log
> 
>    drivers/firmware/qcom_scm.c: In function ‘qcom_scm_assign_mem’:
>    drivers/firmware/qcom_scm.c:469:47: error: passing argument 3 of ‘dma_alloc_coherent’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>      ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL);
>                                                   ^
>    In file included from drivers/firmware/qcom_scm.c:21:0:
>    ./include/linux/dma-mapping.h:560:21: note: expected ‘dma_addr_t * {aka long long unsigned int *}’ but argument is of type ‘phys_addr_t * {aka unsigned int *}’
>     static inline void *dma_alloc_coherent(struct device *dev, size_t size,
>                         ^~~~~~~~~~~~~~~~~~
>    cc1: some warnings being treated as errors
>    scripts/Makefile.build:303: recipe for target 'drivers/firmware/qcom_scm.o' failed
>    make[2]: *** [drivers/firmware/qcom_scm.o] Error 1
>    scripts/Makefile.build:544: recipe for target 'drivers/firmware' failed
>    make[1]: *** [drivers/firmware] Error 2
>    make[1]: *** Waiting for unfinished jobs....
> 
> I think this build failure is probably a regression; rather it is due
> to the stretch update which brings in a new compiler.

The bug has always been present (and still present in master), it is possible 
the compiler became smarter with the upgrade to stretch.

The problem is similar to [1] and happen when the size of phys_addr_t is 
different to dma_addr_t.

I have CCed the maintainers of this file.

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-04/msg00940.html

-- 
Julien Grall

Re: qcom_scm: Incompatible pointer type build failure

[Ian Jackson]

Julien Grall writes ("qcom_scm: Incompatible pointer type build failure"):
> Thank you for the report.
...> 
> On 30/04/2019 13:44, Ian Jackson wrote:
> > osstest service owner writes ("[linux-4.19 test] 135420: regressions - FAIL"):
> >    drivers/firmware/qcom_scm.c: In function ‘qcom_scm_assign_mem’:
> >    drivers/firmware/qcom_scm.c:469:47: error: passing argument 3 of ‘dma_alloc_coherent’ from incompatible pointer type [-Werror=incompatible-pointer-types]
...
> > I think this build failure is probably a regression; rather it is due
> > to the stretch update which brings in a new compiler.
> 
> The bug has always been present (and still present in master), it is possible 
> the compiler became smarter with the upgrade to stretch.
> 
> The problem is similar to [1] and happen when the size of phys_addr_t is 
> different to dma_addr_t.
> 
> I have CCed the maintainers of this file.

That was several weeks ago and osstest is still blocked on this.
Can you please advise what CONFIG_* to disable to work around this ?

Ian.

[PATCH 0/3] qcom_scm: Fix some dma mapping things

[Stephen Boyd]

This patch series fixes some DMA mapping problems reported
in the qcom SCM driver. I haven't tested these patches at all
so it could be totally broken. If someone can test them for
me I'd appreciate it. Otherwise, I'll spend some time dusting
off modem loading code to see if it works.

Stephen Boyd (3):
  firmware: qcom_scm: Use proper types for dma mappings
  firmware: qcom_scm: Cleanup code in qcom_scm_assign_mem()
  firmware: qcom_scm: Fix some typos in docs and printks

 drivers/firmware/qcom_scm.c | 47 +++++++++++++++++++------------------
 include/linux/qcom_scm.h    |  9 +++----
 2 files changed, 29 insertions(+), 27 deletions(-)


base-commit: e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd
-- 
Sent by a computer through tubes

[PATCH 1/3] firmware: qcom_scm: Use proper types for dma mappings

[Stephen Boyd]

We need to use the proper types and convert between physical addresses
and dma addresses here to avoid mismatch warnings. This is especially
important on systems with a different size for dma addresses and
physical addresses. Otherwise, we get the following warning:

  drivers/firmware/qcom_scm.c: In function "qcom_scm_assign_mem":
  drivers/firmware/qcom_scm.c:469:47: error: passing argument 3 of "dma_alloc_coherent" from incompatible pointer type [-Werror=incompatible-pointer-types]

We also fix the size argument to dma_free_coherent() because that size
doesn't need to be aligned after it's already aligned on the allocation
size. In fact, dma debugging expects the same arguments to be passed to
both the allocation and freeing sides of the functions so changing the
size is incorrect regardless.

Reported-by: Ian Jackson <ian.jackson@citrix.com>
Cc: Ian Jackson <ian.jackson@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/qcom_scm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index af4eee86919d..0c63495cf269 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/cpumask.h>
 #include <linux/export.h>
+#include <linux/dma-direct.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -449,6 +450,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	phys_addr_t mem_to_map_phys;
 	phys_addr_t dest_phys;
 	phys_addr_t ptr_phys;
+	dma_addr_t ptr_dma;
 	size_t mem_to_map_sz;
 	size_t dest_sz;
 	size_t src_sz;
@@ -466,9 +468,10 @@ 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, &ptr_dma, GFP_KERNEL);
 	if (!ptr)
 		return -ENOMEM;
+	ptr_phys = dma_to_phys(__scm->dev, ptr_dma);
 
 	/* Fill source vmid detail */
 	src = ptr;
@@ -498,7 +501,7 @@ 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);
+	dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_dma);
 	if (ret) {
 		dev_err(__scm->dev,
 			"Assign memory protection call failed %d.\n", ret);
-- 
Sent by a computer through tubes

[PATCH 2/3] firmware: qcom_scm: Cleanup code in qcom_scm_assign_mem()

[Stephen Boyd]

There are some questionable coding styles in this function. It looks
quite odd to deref a pointer with array indexing that only uses the
first element. Also, destroying an input/output variable halfway through
the function and then overwriting it on success is not clear. It's
better to use a local variable and the kernel macros to step through
each bit set in a bitmask and clearly show where outputs are set.

Cc: Ian Jackson <ian.jackson@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/qcom_scm.c | 34 ++++++++++++++++------------------
 include/linux/qcom_scm.h    |  9 +++++----
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 0c63495cf269..153f13f72bac 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -443,7 +443,8 @@ EXPORT_SYMBOL(qcom_scm_set_remote_state);
  */
 int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 			unsigned int *srcvm,
-			struct qcom_scm_vmperm *newvm, int dest_cnt)
+			const struct qcom_scm_vmperm *newvm,
+			unsigned int dest_cnt)
 {
 	struct qcom_scm_current_perm_info *destvm;
 	struct qcom_scm_mem_map_info *mem_to_map;
@@ -458,11 +459,10 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	int next_vm;
 	__le32 *src;
 	void *ptr;
-	int ret;
-	int len;
-	int i;
+	int ret, i, b;
+	unsigned long srcvm_bits = *srcvm;
 
-	src_sz = hweight_long(*srcvm) * sizeof(*src);
+	src_sz = hweight_long(srcvm_bits) * sizeof(*src);
 	mem_to_map_sz = sizeof(*mem_to_map);
 	dest_sz = dest_cnt * sizeof(*destvm);
 	ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) +
@@ -475,28 +475,26 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 
 	/* Fill source vmid detail */
 	src = ptr;
-	len = hweight_long(*srcvm);
-	for (i = 0; i < len; i++) {
-		src[i] = cpu_to_le32(ffs(*srcvm) - 1);
-		*srcvm ^= 1 << (ffs(*srcvm) - 1);
-	}
+	i = 0;
+	for_each_set_bit(b, &srcvm_bits, sizeof(srcvm_bits))
+		src[i++] = cpu_to_le32(b);
 
 	/* 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[0].mem_addr = cpu_to_le64(mem_addr);
-	mem_to_map[0].mem_size = cpu_to_le64(mem_sz);
+	mem_to_map->mem_addr = cpu_to_le64(mem_addr);
+	mem_to_map->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);
-	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);
-		destvm[i].ctx = 0;
-		destvm[i].ctx_size = 0;
-		next_vm |= BIT(newvm[i].vmid);
+	for (i = 0; i < dest_cnt; i++, destvm++, newvm++) {
+		destvm->vmid = cpu_to_le32(newvm->vmid);
+		destvm->perm = cpu_to_le32(newvm->perm);
+		destvm->ctx = 0;
+		destvm->ctx_size = 0;
+		next_vm |= BIT(newvm->vmid);
 	}
 
 	ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz,
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index d0aecc04c54b..1d406403c843 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -57,8 +57,9 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
 extern int qcom_scm_pas_auth_and_reset(u32 peripheral);
 extern int qcom_scm_pas_shutdown(u32 peripheral);
 extern int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
-			       unsigned int *src, struct qcom_scm_vmperm *newvm,
-			       int dest_cnt);
+			       unsigned int *src,
+			       const struct qcom_scm_vmperm *newvm,
+			       unsigned int dest_cnt);
 extern void qcom_scm_cpu_power_down(u32 flags);
 extern u32 qcom_scm_get_version(void);
 extern int qcom_scm_set_remote_state(u32 state, u32 id);
@@ -95,8 +96,8 @@ qcom_scm_pas_auth_and_reset(u32 peripheral) { return -ENODEV; }
 static inline int qcom_scm_pas_shutdown(u32 peripheral) { return -ENODEV; }
 static inline int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 				      unsigned int *src,
-				      struct qcom_scm_vmperm *newvm,
-				      int dest_cnt) { return -ENODEV; }
+				      const struct qcom_scm_vmperm *newvm,
+				      unsigned int dest_cnt) { return -ENODEV; }
 static inline void qcom_scm_cpu_power_down(u32 flags) {}
 static inline u32 qcom_scm_get_version(void) { return 0; }
 static inline u32
-- 
Sent by a computer through tubes

[PATCH 3/3] firmware: qcom_scm: Fix some typos in docs and printks

[Stephen Boyd]

Some words are misspelled and we put a full stop after a return value
integer. Fix these things up so it doesn't look so odd.

Cc: Ian Jackson <ian.jackson@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/qcom_scm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 153f13f72bac..e300cc272847 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -435,11 +435,11 @@ EXPORT_SYMBOL(qcom_scm_set_remote_state);
  * @mem_sz:   size of the region.
  * @srcvm:    vmid for current set of owners, each set bit in
  *            flag indicate a unique owner
- * @newvm:    array having new owners and corrsponding permission
+ * @newvm:    array having new owners and corresponding permission
  *            flags
  * @dest_cnt: number of owners in next set.
  *
- * Return negative errno on failure, 0 on success, with @srcvm updated.
+ * Return negative errno on failure or 0 on success with @srcvm updated.
  */
 int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 			unsigned int *srcvm,
@@ -502,7 +502,7 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_dma);
 	if (ret) {
 		dev_err(__scm->dev,
-			"Assign memory protection call failed %d.\n", ret);
+			"Assign memory protection call failed %d\n", ret);
 		return -EINVAL;
 	}
 
-- 
Sent by a computer through tubes

[PATCH 1/3] firmware: qcom_scm: Use proper types for dma mappings

[Ian Jackson]

Stephen Boyd writes ("[PATCH 1/3] firmware: qcom_scm: Use proper types for dma mappings"):
> We need to use the proper types and convert between physical addresses
> and dma addresses here to avoid mismatch warnings. This is especially
> important on systems with a different size for dma addresses and
> physical addresses. Otherwise, we get the following warning:

Thanks.  Do you expect this to be a backport candidate and if so how
far back do you think it will go ?  To be honest, I am not really
convinced that backporting this would be a service to users.  The
situation I have, where I changed the compiler but kept the old kernel
code and old configuration, is going to be fairly rare.

I think I should probably therefore disable this driver in the config
on stable branches of Linux, at least.

Thanks,
Ian.

Re: [PATCH 1/3] firmware: qcom_scm: Use proper types for dma mappings

[Julien Grall]

Hi Ian,

On 20/05/2019 10:41, Ian Jackson wrote:
> Stephen Boyd writes ("[PATCH 1/3] firmware: qcom_scm: Use proper types for dma mappings"):
>> We need to use the proper types and convert between physical addresses
>> and dma addresses here to avoid mismatch warnings. This is especially
>> important on systems with a different size for dma addresses and
>> physical addresses. Otherwise, we get the following warning:
> 
> Thanks.  Do you expect this to be a backport candidate and if so how
> far back do you think it will go ?  To be honest, I am not really
> convinced that backporting this would be a service to users.  The
> situation I have, where I changed the compiler but kept the old kernel
> code and old configuration, is going to be fairly rare.

I will leave the maintainers answering to that.

> 
> I think I should probably therefore disable this driver in the config
> on stable branches of Linux, at least.

If we decide to disable the driver, then we would need to add in our .config, 
then we would need to disable completely the support for Qualcomm (i.e 
CONFIG_ARCH_QCOM=n) on Arm32.

This should not be an issue in osstest as we don't test any qualcomm board so far.

Cheers,

-- 
Julien Grall

[OSSTEST PATCH] ts-kernel-build: Disable CONFIG_ARCH_QCOM in Xen Project CI

[Ian Jackson]

  drivers/firmware/qcom_scm.c:469:47: error: passing argument 3 of `dma_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]

This is fixed by

  firmware: qcom_scm: Use proper types for dma mappings

but this is not present in all relevant stable branches.

We currently have no Qualcomm hardware in the Xen Project test lab so
we do not need this enabled.

CC: Julien Grall <julien.grall@arm.com
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: linux-arm-msm@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Stephen Boyd <swboyd@chromium.org>
CC: Andy Gross <agross@kernel.org>
CC: Bjorn Andersson <bjorn.andersson@linaro.org>
CC: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 ts-kernel-build | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ts-kernel-build b/ts-kernel-build
index f7d059b0..5536586f 100755
--- a/ts-kernel-build
+++ b/ts-kernel-build
@@ -274,6 +274,10 @@ setopt CONFIG_MDIO_THUNDER=m
 setopt CONFIG_I2C_THUNDERX=m
 setopt CONFIG_SPI_THUNDERX=m
 
+# Some Linux branches we care about, including 4.19, still lack
+# firmware: qcom_scm: Use proper types for dma mappings
+CONFIG_ARCH_QCOM=n
+
 ####
 
 END
-- 
2.11.0

Re: [OSSTEST PATCH] ts-kernel-build: Disable CONFIG_ARCH_QCOM in Xen Project CI

[Julien Grall]

Hi Ian,

On 30/05/2019 17:51, Ian Jackson wrote:
>    drivers/firmware/qcom_scm.c:469:47: error: passing argument 3 of `dma_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 
> This is fixed by
> 
>    firmware: qcom_scm: Use proper types for dma mappings
> 
> but this is not present in all relevant stable branches.
> 
> We currently have no Qualcomm hardware in the Xen Project test lab so
> we do not need this enabled.
> 
> CC: Julien Grall <julien.grall@arm.com
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: linux-arm-msm@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: Stephen Boyd <swboyd@chromium.org>
> CC: Andy Gross <agross@kernel.org>
> CC: Bjorn Andersson <bjorn.andersson@linaro.org>
> CC: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

FWIW:

Acked-by: Julien Grall <julien.grall@arm.com>

> ---
>   ts-kernel-build | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/ts-kernel-build b/ts-kernel-build
> index f7d059b0..5536586f 100755
> --- a/ts-kernel-build
> +++ b/ts-kernel-build
> @@ -274,6 +274,10 @@ setopt CONFIG_MDIO_THUNDER=m
>   setopt CONFIG_I2C_THUNDERX=m
>   setopt CONFIG_SPI_THUNDERX=m
>   
> +# Some Linux branches we care about, including 4.19, still lack
> +# firmware: qcom_scm: Use proper types for dma mappings
> +CONFIG_ARCH_QCOM=n
> +
>   ####
>   
>   END
> 

-- 
Julien Grall

Re: [PATCH 2/3] firmware: qcom_scm: Cleanup code in qcom_scm_assign_mem()

[Bjorn Andersson]

On Fri 17 May 14:09 PDT 2019, Stephen Boyd wrote:

> There are some questionable coding styles in this function. It looks
> quite odd to deref a pointer with array indexing that only uses the
> first element. Also, destroying an input/output variable halfway through
> the function and then overwriting it on success is not clear. It's
> better to use a local variable and the kernel macros to step through
> each bit set in a bitmask and clearly show where outputs are set.
> 
> Cc: Ian Jackson <ian.jackson@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/qcom_scm.c | 34 ++++++++++++++++------------------
>  include/linux/qcom_scm.h    |  9 +++++----
>  2 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 0c63495cf269..153f13f72bac 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -443,7 +443,8 @@ EXPORT_SYMBOL(qcom_scm_set_remote_state);
>   */
>  int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  			unsigned int *srcvm,
> -			struct qcom_scm_vmperm *newvm, int dest_cnt)
> +			const struct qcom_scm_vmperm *newvm,
> +			unsigned int dest_cnt)
>  {
>  	struct qcom_scm_current_perm_info *destvm;
>  	struct qcom_scm_mem_map_info *mem_to_map;
> @@ -458,11 +459,10 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  	int next_vm;
>  	__le32 *src;
>  	void *ptr;
> -	int ret;
> -	int len;
> -	int i;
> +	int ret, i, b;
> +	unsigned long srcvm_bits = *srcvm;
>  
> -	src_sz = hweight_long(*srcvm) * sizeof(*src);
> +	src_sz = hweight_long(srcvm_bits) * sizeof(*src);
>  	mem_to_map_sz = sizeof(*mem_to_map);
>  	dest_sz = dest_cnt * sizeof(*destvm);
>  	ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) +
> @@ -475,28 +475,26 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  
>  	/* Fill source vmid detail */
>  	src = ptr;
> -	len = hweight_long(*srcvm);
> -	for (i = 0; i < len; i++) {
> -		src[i] = cpu_to_le32(ffs(*srcvm) - 1);
> -		*srcvm ^= 1 << (ffs(*srcvm) - 1);
> -	}
> +	i = 0;
> +	for_each_set_bit(b, &srcvm_bits, sizeof(srcvm_bits))

The modem is sad that you only pass 8 here. Changed it to BITS_PER_LONG
to include the modem's permission bit and applied all three patches.

Thanks,
Bjorn

> +		src[i++] = cpu_to_le32(b);
>  
>  	/* 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[0].mem_addr = cpu_to_le64(mem_addr);
> -	mem_to_map[0].mem_size = cpu_to_le64(mem_sz);
> +	mem_to_map->mem_addr = cpu_to_le64(mem_addr);
> +	mem_to_map->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);
> -	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);
> -		destvm[i].ctx = 0;
> -		destvm[i].ctx_size = 0;
> -		next_vm |= BIT(newvm[i].vmid);
> +	for (i = 0; i < dest_cnt; i++, destvm++, newvm++) {
> +		destvm->vmid = cpu_to_le32(newvm->vmid);
> +		destvm->perm = cpu_to_le32(newvm->perm);
> +		destvm->ctx = 0;
> +		destvm->ctx_size = 0;
> +		next_vm |= BIT(newvm->vmid);
>  	}
>  
>  	ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz,
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index d0aecc04c54b..1d406403c843 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -57,8 +57,9 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
>  extern int qcom_scm_pas_auth_and_reset(u32 peripheral);
>  extern int qcom_scm_pas_shutdown(u32 peripheral);
>  extern int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> -			       unsigned int *src, struct qcom_scm_vmperm *newvm,
> -			       int dest_cnt);
> +			       unsigned int *src,
> +			       const struct qcom_scm_vmperm *newvm,
> +			       unsigned int dest_cnt);
>  extern void qcom_scm_cpu_power_down(u32 flags);
>  extern u32 qcom_scm_get_version(void);
>  extern int qcom_scm_set_remote_state(u32 state, u32 id);
> @@ -95,8 +96,8 @@ qcom_scm_pas_auth_and_reset(u32 peripheral) { return -ENODEV; }
>  static inline int qcom_scm_pas_shutdown(u32 peripheral) { return -ENODEV; }
>  static inline int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  				      unsigned int *src,
> -				      struct qcom_scm_vmperm *newvm,
> -				      int dest_cnt) { return -ENODEV; }
> +				      const struct qcom_scm_vmperm *newvm,
> +				      unsigned int dest_cnt) { return -ENODEV; }
>  static inline void qcom_scm_cpu_power_down(u32 flags) {}
>  static inline u32 qcom_scm_get_version(void) { return 0; }
>  static inline u32
> -- 
> Sent by a computer through tubes
> 

Re: [PATCH 2/3] firmware: qcom_scm: Cleanup code in qcom_scm_assign_mem()

[Stephen Boyd]

Quoting Bjorn Andersson (2019-07-22 16:27:19)
> On Fri 17 May 14:09 PDT 2019, Stephen Boyd wrote:
> 
> > There are some questionable coding styles in this function. It looks
> > quite odd to deref a pointer with array indexing that only uses the
> > first element. Also, destroying an input/output variable halfway through
> > the function and then overwriting it on success is not clear. It's
> > better to use a local variable and the kernel macros to step through
> > each bit set in a bitmask and clearly show where outputs are set.
> > 
> > Cc: Ian Jackson <ian.jackson@citrix.com>
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/firmware/qcom_scm.c | 34 ++++++++++++++++------------------
> >  include/linux/qcom_scm.h    |  9 +++++----
> >  2 files changed, 21 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 0c63495cf269..153f13f72bac 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -443,7 +443,8 @@ EXPORT_SYMBOL(qcom_scm_set_remote_state);
> >   */
> >  int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> >                       unsigned int *srcvm,
> > -                     struct qcom_scm_vmperm *newvm, int dest_cnt)
> > +                     const struct qcom_scm_vmperm *newvm,
> > +                     unsigned int dest_cnt)
> >  {
> >       struct qcom_scm_current_perm_info *destvm;
> >       struct qcom_scm_mem_map_info *mem_to_map;
> > @@ -458,11 +459,10 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> >       int next_vm;
> >       __le32 *src;
> >       void *ptr;
> > -     int ret;
> > -     int len;
> > -     int i;
> > +     int ret, i, b;
> > +     unsigned long srcvm_bits = *srcvm;
> >  
> > -     src_sz = hweight_long(*srcvm) * sizeof(*src);
> > +     src_sz = hweight_long(srcvm_bits) * sizeof(*src);
> >       mem_to_map_sz = sizeof(*mem_to_map);
> >       dest_sz = dest_cnt * sizeof(*destvm);
> >       ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) +
> > @@ -475,28 +475,26 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> >  
> >       /* Fill source vmid detail */
> >       src = ptr;
> > -     len = hweight_long(*srcvm);
> > -     for (i = 0; i < len; i++) {
> > -             src[i] = cpu_to_le32(ffs(*srcvm) - 1);
> > -             *srcvm ^= 1 << (ffs(*srcvm) - 1);
> > -     }
> > +     i = 0;
> > +     for_each_set_bit(b, &srcvm_bits, sizeof(srcvm_bits))
> 
> The modem is sad that you only pass 8 here. Changed it to BITS_PER_LONG
> to include the modem's permission bit and applied all three patches.
> 

Ah of course. Thanks.

BTW, srcvm is an unsigned int, but then we do a bunch of unsigned long
operations on them. Maybe the whole API should be changed to be more
explicit about the size of the type, i.e. u64?

Re: [PATCH 2/3] firmware: qcom_scm: Cleanup code in qcom_scm_assign_mem()

[Bjorn Andersson]

On Mon 22 Jul 17:04 PDT 2019, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2019-07-22 16:27:19)
> > On Fri 17 May 14:09 PDT 2019, Stephen Boyd wrote:
> > 
> > > There are some questionable coding styles in this function. It looks
> > > quite odd to deref a pointer with array indexing that only uses the
> > > first element. Also, destroying an input/output variable halfway through
> > > the function and then overwriting it on success is not clear. It's
> > > better to use a local variable and the kernel macros to step through
> > > each bit set in a bitmask and clearly show where outputs are set.
> > > 
> > > Cc: Ian Jackson <ian.jackson@citrix.com>
> > > Cc: Julien Grall <julien.grall@arm.com>
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Cc: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  drivers/firmware/qcom_scm.c | 34 ++++++++++++++++------------------
> > >  include/linux/qcom_scm.h    |  9 +++++----
> > >  2 files changed, 21 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index 0c63495cf269..153f13f72bac 100644
> > > --- a/drivers/firmware/qcom_scm.c
> > > +++ b/drivers/firmware/qcom_scm.c
> > > @@ -443,7 +443,8 @@ EXPORT_SYMBOL(qcom_scm_set_remote_state);
> > >   */
> > >  int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> > >                       unsigned int *srcvm,
> > > -                     struct qcom_scm_vmperm *newvm, int dest_cnt)
> > > +                     const struct qcom_scm_vmperm *newvm,
> > > +                     unsigned int dest_cnt)
> > >  {
> > >       struct qcom_scm_current_perm_info *destvm;
> > >       struct qcom_scm_mem_map_info *mem_to_map;
> > > @@ -458,11 +459,10 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> > >       int next_vm;
> > >       __le32 *src;
> > >       void *ptr;
> > > -     int ret;
> > > -     int len;
> > > -     int i;
> > > +     int ret, i, b;
> > > +     unsigned long srcvm_bits = *srcvm;
> > >  
> > > -     src_sz = hweight_long(*srcvm) * sizeof(*src);
> > > +     src_sz = hweight_long(srcvm_bits) * sizeof(*src);
> > >       mem_to_map_sz = sizeof(*mem_to_map);
> > >       dest_sz = dest_cnt * sizeof(*destvm);
> > >       ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) +
> > > @@ -475,28 +475,26 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> > >  
> > >       /* Fill source vmid detail */
> > >       src = ptr;
> > > -     len = hweight_long(*srcvm);
> > > -     for (i = 0; i < len; i++) {
> > > -             src[i] = cpu_to_le32(ffs(*srcvm) - 1);
> > > -             *srcvm ^= 1 << (ffs(*srcvm) - 1);
> > > -     }
> > > +     i = 0;
> > > +     for_each_set_bit(b, &srcvm_bits, sizeof(srcvm_bits))
> > 
> > The modem is sad that you only pass 8 here. Changed it to BITS_PER_LONG
> > to include the modem's permission bit and applied all three patches.
> > 
> 
> Ah of course. Thanks.
> 
> BTW, srcvm is an unsigned int, but then we do a bunch of unsigned long
> operations on them. Maybe the whole API should be changed to be more
> explicit about the size of the type, i.e. u64?
> 

It's a bitmap of vmids currently with access to the region and the space
has expanded since I looked at this list time, so the now highest
defined id in the downstream kernel is 42.

So it sounds very reasonable to expand this to a u64.

Regards,
Bjorn