[PATCH v2 02/14] lmb: Rename interior piece to area

[Simon Glass]

Hi Heinrich,

On Fri, 1 Sept 2023 at 00:16, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 9/1/23 03:13, Simon Glass wrote:
> > The lmb data structures use the word 'region' to describe the region in
> > which the allocations happen, as well as the individual allocations in
> > that region. The interior structure is called lmb_property but is
> > described as a region.
> >
> > This is quite confusing. One of the reviewers in v1 asked why we are using
> > a property to describe a region!
>
> We currently have:
>
> struct lmb_region - Description of a set of region.
> struct lmb_property - Description of one region.
>
> The word 'region' implies that it is contiguous, while 'region set'
> implies that its members might not be contiguous.

>From what I can tell, the areas inside a region are contiguous. The
code is so badly commented that it is hard to know what the intent
was.

Actually I just looked it up and found it came from Linux, so I
suppose that explains the lack of comments. There it has been renamed
memblock.

Could you take a look and see if we could adopt the same naming?

>
> Calling a set region is what starts the confusion.
>
> Your patch is creating new confusion by calling a member of "a set of
> regions" "area".
>
> Please, rename as follows:
>
> lmb_region -> lmb_region_set

That sounds like it sets a region

> lmb_property -> lmb_region
>
> The comments below are only valid if you stick with area which I
> strongly discourage.

Fair enough, I don't like it much either.

>
> >
> > It seems better to adopt the word 'area' for the internal pieces, since an
> > area is smaller than a region.
> >
> > As a first step to improve this, rename struct lmb_property to lmb_area.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >   include/lmb.h  | 12 ++++++------
> >   test/lib/lmb.c |  2 +-
> >   2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/lmb.h b/include/lmb.h
> > index 231b68b27d91..4b7664f22c1d 100644
> > --- a/include/lmb.h
> > +++ b/include/lmb.h
> > @@ -23,13 +23,13 @@ enum lmb_flags {
> >   };
> >
> >   /**
> > - * struct lmb_property - Description of one region.
> > + * struct lmb_area - Description of one region.
>
> %s/region/memory area/
>
> For struct lmb_area we need a long text explaining what it describes.
>
> >    *
> >    * @base:   Base address of the region.
> >    * @size:   Size of the region
> >    * @flags:  memory region attributes
> >    */
> > -struct lmb_property {
> > +struct lmb_area {
> >       phys_addr_t base;
> >       phys_size_t size;
> >       enum lmb_flags flags;
> > @@ -64,9 +64,9 @@ struct lmb_region {
> >       unsigned long cnt;
> >       unsigned long max;
> >   #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
>
> %s/MAX_REGIONS/MAX_AREAS/
>
> > -     struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
> > +     struct lmb_area region[CONFIG_LMB_MAX_REGIONS];
>
> This is really confusing: An area called region and indexed by something
> related to regions not areas.
>
> >   #else
> > -     struct lmb_property *region;
> > +     struct lmb_area *region;
>
> ditto
>
> Best regards
>
> Heinrich
>
> >   #endif
> >   };
> >
> > @@ -87,8 +87,8 @@ struct lmb {
> >       struct lmb_region memory;
> >       struct lmb_region reserved;
> >   #if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > -     struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
> > -     struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
> > +     struct lmb_area memory_regions[CONFIG_LMB_MEMORY_REGIONS];
> > +     struct lmb_area reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
> >   #endif
> >   };
> >
> > diff --git a/test/lib/lmb.c b/test/lib/lmb.c
> > index 162887503588..59b140fde4ce 100644
> > --- a/test/lib/lmb.c
> > +++ b/test/lib/lmb.c
> > @@ -12,7 +12,7 @@
> >   #include <test/test.h>
> >   #include <test/ut.h>
> >
> > -static inline bool lmb_is_nomap(struct lmb_property *m)
> > +static inline bool lmb_is_nomap(struct lmb_area *m)
> >   {
> >       return m->flags & LMB_NOMAP;
> >   }
>

Regards,
Simon