[U-Boot] [PATCH v10 05/10] lib: lmb: extend lmb for checks at load time

[Simon Goldschmidt]

Am Mi., 16. Jan. 2019, 22:34 hat Simon Glass <sjg at chromium.org> geschrieben:

> Hi Simon,
>
> On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com> wrote:
> >
> > This adds two new functions, lmb_alloc_addr and
> > lmb_get_unreserved_size.
> >
> > lmb_alloc_addr behaves like lmb_alloc, but it tries to allocate a
> > pre-specified address range. Unlike lmb_reserve, this address range
> > must be inside one of the memory ranges that has been set up with
> > lmb_add.
> >
> > lmb_get_unreserved_size returns the number of bytes that can be
> > used up to the next reserved region or the end of valid ram. This
> > can be 0 if the address passed is reserved.
> >
> > Added test for these new functions.
> >
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> > ---
> >
> > Changes in v10: None
> > Changes in v9: None
> > Changes in v8: None
> > Changes in v7: None
> > Changes in v6: None
> > Changes in v5:
> > - fixed lmb_alloc_addr when resulting reserved ranges get combined
> > - added test for these new functions
> >
> > Changes in v4: None
> > Changes in v2:
> > - added lmb_get_unreserved_size() for tftp
> >
> >  include/lmb.h  |   3 +
> >  lib/lmb.c      |  53 +++++++++++++
> >  test/lib/lmb.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 258 insertions(+)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> But please see suggestions/nits below.
>
> >
> > diff --git a/include/lmb.h b/include/lmb.h
> > index f04d058093..7d7e2a78dc 100644
> > --- a/include/lmb.h
> > +++ b/include/lmb.h
> > @@ -38,6 +38,9 @@ extern phys_addr_t lmb_alloc_base(struct lmb *lmb,
> phys_size_t size, ulong align
> >                             phys_addr_t max_addr);
> >  extern phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size,
> ulong align,
> >                               phys_addr_t max_addr);
> > +extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
> > +                                 phys_size_t size);
>
> Can you please add full comments in the header for new functions.
>

Sure I can but wouldn't it look odd to have one function documented in the
header but not the rest?


> > +extern phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t
> addr);
> >  extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr);
> >  extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t
> size);
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index cd297f8202..e380a0a722 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -313,6 +313,59 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb,
> phys_size_t size, ulong align, phy
> >         return 0;
> >  }
> >
> > +/*
> > + * Try to allocate a specific address range: must be in defined memory
> but not
> > + * reserved
> > + */
> > +phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
> phys_size_t size)
> > +{
> > +       long j;
>
> How about addr instead of j? I think single-char vars are OK for loop
> counters, etc. but this is not that.
>

Sure.


> > +
> > +       /* Check if the requested address is in one of the memory
> regions */
> > +       j = lmb_overlaps_region(&lmb->memory, base, size);
> > +       if (j >= 0) {
> > +               /*
> > +                * Check if the requested end address is in the same
> memory
> > +                * region we found.
> > +                */
> > +               if (lmb_addrs_overlap(lmb->memory.region[j].base,
> > +                                     lmb->memory.region[j].size, base +
> size -
> > +                                     1, 1)) {
> > +                       /* ok, reserve the memory */
> > +                       if (lmb_reserve(lmb, base, size) >= 0)
> > +                               return base;
> > +               }
> > +       }
> > +       return 0;
> > +}
> > +
> > +/* Return number of bytes from a given address that are free */
> > +phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr)
>
> I support you use 'unreserved' instead of 'free' due to some subtle
> difference in meaning? Can you add a comment somewhere about this?
>

Actually no, the name could be changed to 'lmb_get_free_size' if you like.


> > +{
> > +       int i;
> > +       long j;
>
> Here too - addr?
>

Yes.


> > +
> > +       /* check if the requested address is in the memory regions */
> > +       j = lmb_overlaps_region(&lmb->memory, addr, 1);
> > +       if (j >= 0) {
> > +               for (i = 0; i < lmb->reserved.cnt; i++) {
> > +                       if (addr < lmb->reserved.region[i].base) {
> > +                               /* first reserved range > requested
> address */
> > +                               return lmb->reserved.region[i].base -
> addr;
> > +                       }
> > +                       if (lmb->reserved.region[i].base +
> > +                           lmb->reserved.region[i].size > addr) {
> > +                               /* requested addr is in this reserved
> range */
> > +                               return 0;
> > +                       }
> > +               }
> > +               /* if we come here: no reserved ranges above requested
> addr */
> > +               return lmb->memory.region[lmb->memory.cnt - 1].base +
> > +                      lmb->memory.region[lmb->memory.cnt - 1].size -
> addr;
> > +       }
> > +       return 0;
> > +}
> > +
>

Sigh, I'll re-spin again, but I won't find the time to do so before next
week...

Regards,
Simon

>