On Fri, 30 May 2025 at 13:17, Ilias Apalodimas
<ilias.apalodi...@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.
-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
