On Tue, 30 Jul 2024 at 20:10, Simon Glass <s...@chromium.org> wrote:
>
> This function has more special cases than it needs. Simplify it to
> reduce code size and complexity.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
> ---
>
> lib/lmb.c | 57 +++++++++++++++++++------------------------------------
> 1 file changed, 19 insertions(+), 38 deletions(-)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index c11ce308c5b..83b060a2f4d 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -396,14 +396,6 @@ static long lmb_add_region_flags(struct alist
> *lmb_rgn_lst, phys_addr_t base,
> if (alist_err(lmb_rgn_lst))
> return -1;
>
> - if (alist_empty(lmb_rgn_lst)) {
> - rgn[0].base = base;
> - rgn[0].size = size;
> - rgn[0].flags = flags;
> - lmb_rgn_lst->count = 1;
> - return 0;
> - }
> -
> /* First try and coalesce this LMB with another. */
> for (i = 0; i < lmb_rgn_lst->count; i++) {
> phys_addr_t rgnbase = rgn[i].base;
> @@ -448,50 +440,39 @@ static long lmb_add_region_flags(struct alist
> *lmb_rgn_lst, phys_addr_t base,
> }
> }
>
> - if (i < lmb_rgn_lst->count - 1 &&
> - rgn[i].flags == rgn[i + 1].flags) {
> - if (lmb_regions_adjacent(lmb_rgn_lst, i, i + 1)) {
> - lmb_coalesce_regions(lmb_rgn_lst, i, i + 1);
> - coalesced++;
> - } else if (lmb_regions_overlap(lmb_rgn_lst, i, i + 1)) {
> - /* fix overlapping area */
> - lmb_fix_over_lap_regions(lmb_rgn_lst, i, i + 1);
> - coalesced++;
> + if (lmb_rgn_lst->count && i < lmb_rgn_lst->count - 1) {
> + rgn = lmb_rgn_lst->data;
> + if (rgn[i].flags == rgn[i + 1].flags) {
> + if (lmb_regions_adjacent(lmb_rgn_lst, i, i + 1)) {
> + lmb_coalesce_regions(lmb_rgn_lst, i, i + 1);
> + coalesced++;
> + } else if (lmb_regions_overlap(lmb_rgn_lst, i, i +
> 1)) {
> + /* fix overlapping area */
> + lmb_fix_over_lap_regions(lmb_rgn_lst, i, i +
> 1);
> + coalesced++;
> + }
> }
> }
>
> if (coalesced)
> return coalesced;
>
> - if (alist_full(lmb_rgn_lst)) {
> - if (!alist_expand_by(lmb_rgn_lst, lmb_rgn_lst->alloc * 2))
> - return -1;
> - else
> - rgn = lmb_rgn_lst->data;
> - }
> + if (!alist_add_placeholder(lmb_rgn_lst))
> + return -1;
> + rgn = lmb_rgn_lst->data;
I think the above should have a check for alist_full(), and then add
to the list if full. Else we simply go on adding to the list on every
new node that gets added.
>
> /* Couldn't coalesce the LMB, so add it to the sorted table. */
> for (i = lmb_rgn_lst->count - 1; i >= 0; i--) {
> - if (base < rgn[i].base) {
> - rgn[i + 1].base = rgn[i].base;
> - rgn[i + 1].size = rgn[i].size;
> - rgn[i + 1].flags = rgn[i].flags;
> + if (i && base < rgn[i - 1].base) {
> + rgn[i] = rgn[i - 1];
> } else {
> - rgn[i + 1].base = base;
> - rgn[i + 1].size = size;
> - rgn[i + 1].flags = flags;
> + rgn[i].base = base;
> + rgn[i].size = size;
> + rgn[i].flags = flags;
> break;
> }
> }
With the logic that you put above, should the loop not have 'i'
initialised to lmb_rgn_lst-count? I mean
for (i = lmb_rgn_lst->count; i >= 0; i--)
Else we are overwriting one region?
-sughosh
>
> - if (base < rgn[0].base) {
> - rgn[0].base = base;
> - rgn[0].size = size;
> - rgn[0].flags = flags;
> - }
> -
> - lmb_rgn_lst->count++;
> -
> return 0;
> }
>
> --
> 2.34.1
>
