Re: [PATCH v2 02/11] firmware: qcom: scm: add a dedicated SCM memory allocator

[Bartosz Golaszewski]

On Thu, Sep 28, 2023 at 8:19 PM Jeff Johnson <quic_jjohnson@xxxxxxxxxxx> wrote:
>
> On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> > +void *qcom_scm_mem_alloc(size_t size, gfp_t gfp)
> > +{
> > +     struct qcom_scm_mem_chunk *chunk;
> > +     unsigned long vaddr;
>
> there are places below where you unnecessarily typecast this to its
> given type
>

Ah, it's a leftover from when this variable was of type void *. Thanks.

> > +     int ret;
> > +
> > +     if (!size)
> > +             return ZERO_SIZE_PTR;
> > +
> > +     size = roundup(size, 1 << PAGE_SHIFT);
> > +
> > +     chunk = kzalloc(sizeof(*chunk), gfp);
> > +     if (!chunk)
> > +             return NULL;
> > +
> > +     vaddr = gen_pool_alloc(qcom_scm_mem.pool, size);
> > +     if (!vaddr) {
> > +             kfree(chunk);
> > +             return NULL;
> > +     }
> > +
> > +     chunk->paddr = gen_pool_virt_to_phys(qcom_scm_mem.pool,
> > +                                          (unsigned long)vaddr);
>
> unnecessary typecast?
>
> > +     chunk->size = size;
> > +
> > +     scoped_guard(spinlock_irqsave, &qcom_scm_mem.lock) {
>
> my first exposure to this infrastructure..very cool now that I've
> wrapped my head around it! This helped for those also new to this:
> https://lwn.net/Articles/934679/
>

It's amazing but be careful with it. I used it wrong in one place and
got yelled at by Linus Torvalds personally already. :)

Bartosz

> > +             ret = radix_tree_insert(&qcom_scm_mem.chunks, vaddr, chunk);
> > +             if (ret) {
> > +                     gen_pool_free(qcom_scm_mem.pool, (unsigned long)vaddr,
>
> unnecessary typecast?
>
> > +                                   chunk->size);
> > +                     kfree(chunk);
> > +                     return NULL;
> > +             }
> > +     }
> > +
> > +     return (void *)vaddr;
> > +}
>