On Wed, 21 May 2025 at 14:48, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > 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.
Sorry, I replied here in haste. The reason I introduced lmb_can_reserve_region() is because there is a subtle difference between what lmb_overlaps_region() does. The lmb_overlaps_region() is used primarily to check if an address can be allocated. So it stops iterating through the regions as soon as it finds an overlap -- this makes sense as we cannot allocate memory over already reserved memory. The lmb_can_reserve_region() is used for reserving memory, which might already have been reserved. This function then checks if the requested region overlaps one or multiple regions, and if the flags of all the existing regions match (all regions have flag LMB_NONE) so that the reservation can go through. I have depicted the scenario in a diagram [1]. So, in case of code re-use, there will have to be some tweaks made to lmb_overlaps_region(), so that in case of a reservation request, it should not stop iterating through the loop in case of an overlap. I will check if it can be cleanly implemented. -sughosh [1] - https://gist.github.com/sughoshg/accfad55244fa0f3965e2137db8062b2 > > > > > [...] > > > > Overall this is moving towards thr gith direction > > Good to know :) > > -sughosh > > > > > Cheers > > /Ilias
