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

[Sughosh Ganu]

On Wed, 4 Jun 2025 at 12:53, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> [...]
>
> > > > > >
> > > > > >
> > > > > > 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.
> > >
> > > what kind of code size increase are we talking about?
> >
> > Let me check the actual size impact with the changes and get back to you.
> >
> > > It's returning
> > > na int instead of a physical address so that should be close to zero?
> >
> > Sorry, I do not understand what you are suggesting, but even the above
> > static function is returning a phys_addr_t value, which is 0 in case
> > of an error. And I have kept this to mimic the existing behaviour.
> > Thanks.
>
> I am saying that changing the return type from phys_addr_t to an int
> should have a negligible size impact.

Yes, that will not have a size impact, but there would have to be a
check for whether the env variable has to be set [1], and that adds to
the size. That check might be superfluous if the address 0x0 is not
valid for the memory map of the platform. Fwiw, adding this check adds
about 256 bytes to the binary -- I can incorporate the check if an
addition of 256 bytes is fine.

-sughosh

[1] - https://gist.github.com/sughoshg/3ec45f2a8942db7bf5872efc8d373928

>
> Thanks
> /Ilias
> >
> > -sughosh
> >
> > >
> > > Thanks
> > > /Ilias
> > > > 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