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 >