On Wed, Oct 11, 2023 at 04:33:53PM +0200, Bartosz Golaszewski wrote: > On Wed, Oct 11, 2023 at 3:54 PM Andrew Halaney <ahalaney@xxxxxxxxxx> wrote: > > > > On Wed, Oct 11, 2023 at 09:41:49AM +0200, Bartosz Golaszewski wrote: > > > On Wed, Oct 11, 2023 at 12:19 AM Andrew Halaney <ahalaney@xxxxxxxxxx> wrote: > > > > > > > > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > > > > > > > > > Let's use the new TZ memory allocator to obtain a buffer for this call > > > > > instead of using dma_alloc_coherent(). > > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > > > > --- > > > > > drivers/firmware/qcom/qcom_scm.c | 10 ++++++---- > > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > > > index 71e98b666391..754f6056b99f 100644 > > > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > > > @@ -4,6 +4,7 @@ > > > > > */ > > > > > > > > > > #include <linux/arm-smccc.h> > > > > > +#include <linux/cleanup.h> > > > > > #include <linux/clk.h> > > > > > #include <linux/completion.h> > > > > > #include <linux/cpumask.h> > > > > > @@ -998,14 +999,13 @@ 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; > > > > > - dma_addr_t ptr_phys; > > > > > + phys_addr_t ptr_phys; > > > > > size_t mem_to_map_sz; > > > > > size_t dest_sz; > > > > > size_t src_sz; > > > > > size_t ptr_sz; > > > > > int next_vm; > > > > > __le32 *src; > > > > > - void *ptr; > > > > > > > > nit: couldn't you keep this up here? > > > > > > > > > > This still needs to make its way into the coding style guide but I got > > > yelled at by Linus Torvalds personally for not declaring the managed > > > variables where they are initialized. So this is the correct approach. > > > > I'm being a stick in the mud, but couldn't you initialize to NULL and > > keep them all up top? That seems more in line with the current "declare > > all variables at the start of function" guideline the kernel follows. > > > > Not a big deal... yours call! but /me shrugs > > > > I agree with you but it's not my call to make. Please see[1]. > Yeah, I see you're following the guidance there (declare + initialize together unless there's a conditional, etc, preventing that) in this series. Thanks for the pointer. > Bartosz > > [1] https://lore.kernel.org/lkml/20230919193516.GA20937@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/#m7f97e10dbfde777f58493398a77933e6a2f3c15d > > > > > > > Bart > > > > > > > Otherwise, > > > > > > > > Reviewed-by: Andrew Halaney <ahalaney@xxxxxxxxxx> > > > > > > > > > int ret, i, b; > > > > > u64 srcvm_bits = *srcvm; > > > > > > > > > > @@ -1015,10 +1015,13 @@ 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); > > > > > + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > > > > > + ptr_sz, GFP_KERNEL); > > > > > if (!ptr) > > > > > return -ENOMEM; > > > > > > > > > > + ptr_phys = qcom_tzmem_to_phys(ptr); > > > > > + > > > > > /* Fill source vmid detail */ > > > > > src = ptr; > > > > > i = 0; > > > > > @@ -1047,7 +1050,6 @@ 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, ptr_sz, ptr, ptr_phys); > > > > > if (ret) { > > > > > dev_err(__scm->dev, > > > > > "Assign memory protection call failed %d\n", ret); > > > > > -- > > > > > 2.39.2 > > > > > > > > > > > > > > >
