On Wed, 4 Jun 2025 at 16:08, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > On Wed, 4 Jun 2025 at 12:53, Ilias Apalodimas > <ilias.apalodi...@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.
Can we arrive at a decision on this please. If I don't get any response by tomorrow, I will incorporate Ilias's comment, and will put a check for setting the env variable. Thanks. -sughosh > > -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
