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 > + * 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? > +/** > + * 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. [...] Overall this is moving towards thr gith direction Cheers /Ilias
