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

[Simon Glass]

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.

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

> +
> +       /* 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?

> +{
> +       int i;
> +       long j;

Here too - addr?

> +
> +       /* 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;
> +}
> +

Regards,
Simon