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

[Tom Rini <trini@konsulko.com>]

On Wed, Jan 16, 2019 at 10:44:16PM +0100, Simon Goldschmidt wrote:
> Am Mi., 16. Jan. 2019, 22:34 hat Simon Glass <sjg@chromium.org> geschrieben:
> 
> > Hi Simon,
> >
> > On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
> > <simon.k.r.goldschmidt@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@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@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...

Do it as a follow-up please, I'm testing v10 atm.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190116/cf65b368/attachment.sig>