[PATCH v3 2/6] lmb: replace the lmb_alloc() and lmb_alloc_base() API's

[Sughosh Ganu]

On Fri, 30 May 2025 at 15:41, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Fri, 30 May 2025 at 13:17, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > > +static phys_addr_t lmb_alloc(phys_size_t size)
> > > +{
> > > +       int ret;
> > > +       phys_addr_t addr;
> > > +
> > > +       /* All memory regions allocated with a 2MiB alignment */
> > > +       ret = lmb_alloc_mem(LMB_MEM_ALLOC_ANY, SZ_2M, &addr, size, LMB_NONE);
> > > +       if (ret)
> > > +               return 0;
> > > +
> > > +       return addr;
> > > +}
> > > +
> >
> > I think we  need a better naming for these. Right now we have
> > lmb_alloc() here and in tests, addr_alloc() in snapdragon code.
> > I'd say either export them as API if you think they would be useful,
> > or get rid of the wrappers.
>
> I kept these functions as is to reduce the amount of change resulting
> from introducing the API. Also, the newly introduced API has
> parameters which are common across all the callers, which also was why
> I kept a wrapper. This is especially true in the tests, where it would
> be required to change the function names in a bunch of places without
> much benefit.
>
> >
> > [...]
> >
> > > +static phys_addr_t lmb_alloc(phys_size_t size, ulong align)
> > > +{
> > > +       int err;
> > > +       phys_addr_t addr;
> > > +
> > > +       err = lmb_alloc_mem(LMB_MEM_ALLOC_ANY, align, &addr, size, LMB_NONE);
> > > +       if (err)
> > > +               return 0;
> >
> >
> > This tends to blow up in random ways. See
> > commit 67be24906fe. TL;DR 0 is a valid address in some systems.
>
> Yes, I see your point. I think the calling function will have to be
> re-written such that the env variables get stored only when the API
> returns successfully. Then at least the platform will not have an env
> variable with some junk value.

Thinking about this a bit, I think in these two instances, returning a
value of 0 might not be an issue if the DRAM memory does not start at
0x0. Don't get me wrong, what you are suggesting is definitely
correct. I am only thinking about increasing code size on these
platforms if 0x0 is not a valid address, and moreover since the
platforms were already setting 0x0 in case of an error. If it is okay
to increase code size on these platforms, I will change the calling
function, such that the variable does not get set in case of an error.
Maybe Casey and Mark can comment? Thanks.

-sughosh

>
> -sughosh
>
> >
> > > +
> > > +       return addr;
> > > +}
> > > +
> > > +static phys_addr_t lmb_alloc_base(phys_size_t size, ulong align,
> > > +                                 phys_addr_t max_addr, u32 flags)
> > > +{
> > > +       int err;
> > > +       phys_addr_t addr;
> > > +
> > > +       addr = max_addr;
> > > +       err = lmb_alloc_mem(LMB_MEM_ALLOC_MAX, align, &addr, size, flags);
> > > +       if (err)
> > > +               return 0;
> > > +
> > > +       return addr;
> > > +}
> > > +
> > >  #define lmb_alloc_addr(addr, size, flags) lmb_reserve(addr, size, flags)
> > >
> > >  static int test_multi_alloc(struct unit_test_state *uts, const phys_addr_t ram,
> > > --
> > > 2.34.1
> > >
> >
> > Cheers
> > /Ilias