| Message ID | 20190517210923.202131-3-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcom_scm: Fix some dma mapping things | expand |
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 >
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?
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
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
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(-)