Re: [PATCH v3 07/15] firmware: qcom: scm: make qcom_scm_assign_mem() use the TZ allocator

[Andrew Halaney]

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

> 
> 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
> > >
> >
>