[RFC PATCH 2/4] lmb: Tighten up the code in lmb_add_region_flags()

[Sughosh Ganu]

On Thu, 1 Aug 2024 at 20:28, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Tue, 30 Jul 2024 at 20:10, Simon Glass <sjg at 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 <sjg at 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?

To answer my question, yes we do need to change the loop start to
accommodate the new node. This code is better, thanks for the
suggestion!

-sughosh

>
> -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
> >