On Wed, 21 May 2025 at 13:23, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Sughosh > > On Tue May 20, 2025 at 3:06 PM EEST, Sughosh Ganu wrote: > > There currently are multiple allocation API's in the LMB module. There > > are a couple of API's for allocating memory(lmb_alloc() and > > lmb_alloc_base()), and then there are two for requesting a reservation > > for a particular memory region (lmb_reserve() and > > lmb_alloc_addr()). Introduce a single API lmb_allocate_mem() which > > will cater to all types of allocation requests and replace > > lmb_reserve() and lmb_alloc_addr() with the new API. > > > > Moreover, the lmb_reserve() API is pretty similar to the > > lmb_alloc_addr() API, with the one difference being that the > > lmb_reserve() API allows for reserving any address passed to it -- > > the address need not be part of the LMB memory map. The > > lmb_alloc_addr() does check that the address being requested is > > actually part of the LMB memory map. > > > > There is no need to support reserving memory regions which are outside > > the LMB memory map. Remove the lmb_reserve() API functionality and use > > the functionality provided by lmb_alloc_addr() instead. The > > lmb_alloc_addr() will check if the requested address is part of the > > LMB memory map and return an error if not. > > > > * there's no need to check the result > > */ > > arm_smccc_smc(MTK_SIP_GET_BL31_REGION, 0, 0, 0, 0, 0, 0, 0, &res); > > - lmb_reserve(res.a1, res.a2, LMB_NOMAP); > > + addr = (phys_addr_t)res.a1; > > + lmb_allocate_mem(LMB_MEM_ALLOC_ADDR, 0, &addr, res.a2, LMB_NOMAP); > > In some case you check the return value and in others you ignore it. > Since you are changing this, check the return address everywhere
Yes, I mentioned this in the cover-letter. I have kept the code in arch-specific functions as is when it comes to error checking, as I do not have the associated boards with me. I am not sure what side effects erroring out would have. It would be better if the respective custodians/maintainers fix this, as it would ensure no breakage. > > > + * be -EINVAL if the requested memory region is not part of the LMB memory > > + * map, and -EEXIST if the requested region is already allocated. > > + */ > > +int lmb_allocate_mem(enum lmb_mem_type type, u64 align, phys_addr_t *addr, > > Nit, but can you rename it to lmb_alloc_mem() so it uses abbreviations > everywhere? You mean a shorter name? I had pondered over using lmb_alloc_mem() instead earlier, but thought about using the current one, as this is a globally visible API. > > > +/** > > + * lmb_can_reserve_region() - check if the region can be reserved > > + * @base: base address of region to be reserved > > + * @size: size of region to be reserved > > + * @flags: flag of the region to be reserved > > + * > > + * Go through all the reserved regions and ensure that the requested > > + * region does not overlap with any existing regions. An overlap is > > + * allowed only when the flag of the request region and the existing > > + * region is LMB_NONE. > > + * > > + * Return: true if region can be reserved, false otherwise > > + */ > > +static bool lmb_can_reserve_region(phys_addr_t base, phys_size_t size, > > + u32 flags) > > Overall this is a good cleanup. But I was hoping we can rid of this as well. > This is almost identical to lmb_overlaps_region() with very small differences. > One of these takes the lmb list as an argument and the other is also checking > some flags. > It's better if we keep one. I can get rid of the above function. It would require putting the checks for the flags in the calling function, but we can indeed use lmb_overlaps_region() instead. I will put this in a separate patch. Thanks. > > [...] > > Overall this is moving towards thr gith direction Good to know :) -sughosh > > Cheers > /Ilias
