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

[Simon Glass]

Hi Sughosh,

On Thu, 1 Aug 2024 at 10:19, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Thu, 1 Aug 2024 at 21:42, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 1 Aug 2024 at 08:58, 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.
> >
> > At this point in the function, we know that we are adding a new entry.
> > We could have an anlist_insert() perhaps?
> >
> > >
> > > >
> > > >         /* 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?
> >
> > It starts by doing the assignment rgn[count - 1 + 1] = rgn[count - 1],
> > which I believe is correct?
>
> That was the earlier logic, but now this is being replaced by
>
>  +               if (i && base < rgn[i - 1].base) {
>  +                       rgn[i] = rgn[i - 1];
>
> Which means 'i' should be initialised to lmb_rgn_cnt->count? If you
> see the patch, the for loop initialisation is not being changed.

OK, well I'll let you sort that out :-)

Regards,
Simon